|
|
Descriptioncontent_browsertests for stale-while-revalidate
Validate correct end-to-end behaviour for stale-while-revalidate.
BUG=348877
TEST=content_browsertests
Committed: https://crrev.com/0f39f4a074e5bb473ec97e5adf50e772c769e773
Cr-Commit-Position: refs/heads/master@{#369116}
Patch Set 1 #Patch Set 2 : Add CookieSetAsynchronously test. #
Total comments: 2
Patch Set 3 : Clarify that the RunLoop is only used in one test. #Patch Set 4 : Rebase. #Patch Set 5 : Move tests to async_revalidation_manager_browsertest.cc #Patch Set 6 : Rebase. #
Total comments: 17
Patch Set 7 : Fixes from kinuko review. #Patch Set 8 : Remove excess blank line. #
Total comments: 2
Patch Set 9 : Flake protection for CacheIsUpdated test. #Patch Set 10 : Make CacheIsUpdated test deterministic. Also modify CheckTestTest() to TitleBecomes() for better di… #Patch Set 11 : Add missing base:: prefix to UTF16ToUTF8 function. #
Total comments: 4
Patch Set 12 : Remove unreachable failure diagnostics from TitleBecomes(). #
Messages
Total messages: 32 (8 generated)
ricea@chromium.org changed reviewers: + tyoshino@chromium.org
lgtm https://codereview.chromium.org/1308203003/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_browsertest.cc:562: scoped_ptr<HttpResponse> CountingRequestHandler(const HttpRequest& request) { this is special to StaleWhileRevalidateIsApplied for now. please explain that in the comment, or make the condition for running run_loop_.Quit() configurable and document that in the comment.
https://codereview.chromium.org/1308203003/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_browsertest.cc:562: scoped_ptr<HttpResponse> CountingRequestHandler(const HttpRequest& request) { On 2015/08/31 09:37:46, tyoshino wrote: > this is special to StaleWhileRevalidateIsApplied for now. please explain that in > the comment, or make the condition for running run_loop_.Quit() configurable and > document that in the comment. I have just documented it in comments for now. If we need a more general exit condition in future, we can change it then.
thanks. lgtm again in case you're waiting for it
ricea@chromium.org changed reviewers: + davidben@chromium.org
+davidben
On 2015/09/08 18:43:12, Adam Rice wrote: > +davidben Is this still active?
On 2015/11/10 05:43:23, cbentzel wrote: > On 2015/09/08 18:43:12, Adam Rice wrote: > > +davidben > > Is this still active? Yes. I still want to land this after the //content CL has landed.
Description was changed from ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests ========== to ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests ==========
ricea@chromium.org changed reviewers: + kinuko@chromium.org - davidben@chromium.org
-davidben, +kinuko Reducing davidben's review load.
https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:50: int requests_counted() { return requests_counted_; } nit: can be a const method https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:69: void SetUpCommandLine(base::CommandLine* command_line) override { nit: please place override methods from the same class next to each other https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:89: // TODO(ricea): Support a general condition if other tests need it. nit: I don't think these TODO comments add some value in testing code, please just clarify why this condition is appropriate in the current test code https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:92: return http_response.Pass(); nit: move? https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:134: return http_response.Pass(); std::move https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:149: GURL url(embedded_test_server()->GetURL("/counted.html")); nit: should these URL be a static const variable if we refer this URL at multiple places https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:162: EXPECT_EQ(2, requests_counted()); Also check EXPECT_EQ(1, requests_counted()) before we make async revalidation? https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:195: DVLOG(1) << "Number of loads: " << actual_loads; Having DVLOG(1) in tests... what usage are you imagining? This doesn't seem a part of test if we're just printing.
https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:50: int requests_counted() { return requests_counted_; } On 2015/12/15 15:28:54, kinuko wrote: > nit: can be a const method Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:69: void SetUpCommandLine(base::CommandLine* command_line) override { On 2015/12/15 15:28:54, kinuko wrote: > nit: please place override methods from the same class next to each other Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:89: // TODO(ricea): Support a general condition if other tests need it. On 2015/12/15 15:28:54, kinuko wrote: > nit: I don't think these TODO comments add some value in testing code, please > just clarify why this condition is appropriate in the current test code Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:92: return http_response.Pass(); On 2015/12/15 15:28:54, kinuko wrote: > nit: move? Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:134: return http_response.Pass(); On 2015/12/15 15:28:54, kinuko wrote: > std::move Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:149: GURL url(embedded_test_server()->GetURL("/counted.html")); On 2015/12/15 15:28:54, kinuko wrote: > nit: should these URL be a static const variable if we refer this URL at > multiple places I used namespace-scoped constants to avoid needing separate definition and declarations. PTAL. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:162: EXPECT_EQ(2, requests_counted()); On 2015/12/15 15:28:54, kinuko wrote: > Also check EXPECT_EQ(1, requests_counted()) before we make async revalidation? Done. https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:195: DVLOG(1) << "Number of loads: " << actual_loads; On 2015/12/15 15:28:54, kinuko wrote: > Having DVLOG(1) in tests... what usage are you imagining? This doesn't seem a > part of test if we're just printing. If the test became flaky, the log could be used to adjust the value of kMaxLoads. However, that doesn't seem likely to happen in practice. As you say, it isn't actually part of the test, so I have removed it.
https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/100001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:134: return http_response.Pass(); On 2016/01/05 09:21:44, Adam Rice (ooo until 5 Jan) wrote: > On 2015/12/15 15:28:54, kinuko wrote: > > std::move > > Done. Actually, this caused a compiler error about std::move preventing copy constructor elision. It appears to work fine without the std::move, however.
Mostly looking good, I wonder if there's a way to make the test with kMaxLoads really deterministic? https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:188: for (int loads = 0; loads < kMaxLoads && !matched; ++loads) { Is the reload number (100) chosen arbitrary? It feels that there still be the possibility that this test could be flaky?
https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:188: for (int loads = 0; loads < kMaxLoads && !matched; ++loads) { On 2016/01/05 13:43:53, kinuko wrote: > Is the reload number (100) chosen arbitrary? It feels that there still be the > possibility that this test could be flaky? Yes. I've only tested on my workstation, but I've never seen it take more than one load in practice. We could make kMaxLoads bigger, but even 100 loads should be easily enough to see the updated cache entry. In the latest patch set I've added code to wait until the embedded test server has handled the async revalidation. This should ensure that the request has a chance to be serviced even in CPU-starved environments. But this can only guarantee the handler has run and not necessarily that the data has been sent or received. We could use the same trick as the CookieSetAsynchronously test to guarantee that the response has been received by the browser before trying to load the page, but I'm concerned that that would be too much complexity.
On 2016/01/05 14:38:46, Adam Rice (ooo until 5 Jan) wrote: > https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... > File content/browser/loader/async_revalidation_manager_browsertest.cc (right): > > https://codereview.chromium.org/1308203003/diff/140001/content/browser/loader... > content/browser/loader/async_revalidation_manager_browsertest.cc:188: for (int > loads = 0; loads < kMaxLoads && !matched; ++loads) { > On 2016/01/05 13:43:53, kinuko wrote: > > Is the reload number (100) chosen arbitrary? It feels that there still be the > > possibility that this test could be flaky? > > Yes. I've only tested on my workstation, but I've never seen it take more than > one load in practice. We could make kMaxLoads bigger, but even 100 loads should > be easily enough to see the updated cache entry. > > In the latest patch set I've added code to wait until the embedded test server > has handled the async revalidation. This should ensure that the request has a > chance to be serviced even in CPU-starved environments. But this can only > guarantee the handler has run and not necessarily that the data has been sent or > received. > > We could use the same trick as the CookieSetAsynchronously test to guarantee > that the response has been received by the browser before trying to load the > page, but I'm concerned that that would be too much complexity. Ok... do you know if we have similar tests (I think we should have some?) and what they are doing? I'm still feeling a bit uneasy. Also could we have a reasonably understandable comment (for sherrifs) so that sheriffs can properly handle when they find this test is flaky?
On 2016/01/06 03:44:05, kinuko wrote: > Ok... do you know if we have similar tests (I think we should have some?) and > what they are doing? I'm still feeling a bit uneasy. > > Also could we have a reasonably understandable comment (for sherrifs) so that > sheriffs can properly handle when they find this test is flaky? I realised that the test can be deterministic because once the async revalidation request is sent the cache lock ensures that the old cache entry can no longer be retrieved. So I have removed the loop. If the cache lock did not exist then this would depend on the semantics that we chose for stale-while-revalidate. In other words, it would be possible to choose semantics where the stale result was returned by the cache while the browser waited for the response to the async revalidation request. However, if that happens it will be many months in the future and this code will be significantly different anyway.
I'm really glad that we no longer need to loop! https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:72: << actual_title << "'"; I wonder it'd be probably more readable if this method just returns a new title and we check the result at the callsite? EXPECT_EQ(expected_title, NavigateAndGetTitle(url));
https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:72: << actual_title << "'"; On 2016/01/06 08:17:13, kinuko wrote: > I wonder it'd be probably more readable if this method just returns a new title > and we check the result at the callsite? > > EXPECT_EQ(expected_title, NavigateAndGetTitle(url)); Unfortunately content::TitleWatcher can only wait for a specific list of titles. One result of this is that if the test fails, it will actually timeout rather than printing a useful diagnostic. This is not great, but I don't think it's worth writing a special version of TitleWatcher for this test.
https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:72: << actual_title << "'"; On 2016/01/06 08:32:26, Adam Rice wrote: > On 2016/01/06 08:17:13, kinuko wrote: > > I wonder it'd be probably more readable if this method just returns a new > title > > and we check the result at the callsite? > > > > EXPECT_EQ(expected_title, NavigateAndGetTitle(url)); > > Unfortunately content::TitleWatcher can only wait for a specific list of titles. > One result of this is that if the test fails, it will actually timeout rather > than printing a useful diagnostic. This is not great, but I don't think it's > worth writing a special version of TitleWatcher for this test. Hmm, but in either way aren't we calling title_watcher (hoping that it returns without timing out) and then comparing the returned title for post-diagnosis, right? If we can't get useful diagnostic in failure case anyways why are we making this method slightly more complex?
https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... File content/browser/loader/async_revalidation_manager_browsertest.cc (right): https://codereview.chromium.org/1308203003/diff/200001/content/browser/loader... content/browser/loader/async_revalidation_manager_browsertest.cc:72: << actual_title << "'"; On 2016/01/06 08:44:52, kinuko wrote: > On 2016/01/06 08:32:26, Adam Rice wrote: > > On 2016/01/06 08:17:13, kinuko wrote: > > > I wonder it'd be probably more readable if this method just returns a new > > title > > > and we check the result at the callsite? > > > > > > EXPECT_EQ(expected_title, NavigateAndGetTitle(url)); > > > > Unfortunately content::TitleWatcher can only wait for a specific list of > titles. > > One result of this is that if the test fails, it will actually timeout rather > > than printing a useful diagnostic. This is not great, but I don't think it's > > worth writing a special version of TitleWatcher for this test. > > Hmm, but in either way aren't we calling title_watcher (hoping that it returns > without timing out) and then comparing the returned title for post-diagnosis, > right? If we can't get useful diagnostic in failure case anyways why are we > making this method slightly more complex? Good point. I have simplified it and added a comment why it doesn't provide better diagnostics.
lgtm
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org Link to the patchset: https://codereview.chromium.org/1308203003/#ps220001 (title: "Remove unreachable failure diagnostics from TitleBecomes().")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308203003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308203003/220001
Message was sent while issue was closed.
Description was changed from ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests ========== to ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Failed to apply the patch.
Message was sent while issue was closed.
Description was changed from ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests ========== to ========== content_browsertests for stale-while-revalidate Validate correct end-to-end behaviour for stale-while-revalidate. BUG=348877 TEST=content_browsertests Committed: https://crrev.com/0f39f4a074e5bb473ec97e5adf50e772c769e773 Cr-Commit-Position: refs/heads/master@{#369116} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0f39f4a074e5bb473ec97e5adf50e772c769e773 Cr-Commit-Position: refs/heads/master@{#369116} |