Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561493003:
Rework of ResourcePrefetchPredictor browser tests infrastructure. (Closed)
DescriptionSummary
-----
Allows for custom comparison of results and exotic navigation setup.
Details
-----
- New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate.
- To be able to add the test cases we need to have some more customization
than what the current test framework provides. Specifically, partial
comparison and new navigation types. Implemented that.
BUG=650253
Committed: https://crrev.com/23b1517fb1deb148503f67f1bc9f38283f4e89a0
Cr-Commit-Position: refs/heads/master@{#437877}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Using BROWSER_TEST_NONE #
Total comments: 10
Patch Set 3 : Post-Review Modifications #
Total comments: 7
Patch Set 4 : Post-Review Modifications 2 #Patch Set 5 : Post-Review Modifications 3 #Messages
Total messages: 36 (20 generated)
ahemery@chromium.org changed reviewers: + alexilin@chromium.org, lizeb@chromium.org
https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:131: bool match_navigation_id) New test observer parameter. If true, when comparing result with reference we match the navigation_id's https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:149: if (!match_navigation_id_) { If we are not matching the navigation_id we empty all the id's. This is because we cannot pass a custom comparison function to the GTEST matcher. Note the copy of refs to avoid messing with the constness. https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:527: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInPopup) { New tests. Do the same as simple learning but in a popup or new tab https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:217: (disposition == WindowOpenDisposition::NEW_POPUP)) { I believe this one was just forgotten. Behavior was buggy with NEW_POPUP+NAVIGATION.
Description was changed from ========== Summary ----- Adding new browser tests related to ResourcePrefetchPredictor Details ----- This patch is about two things: - Adding two new browser tests to the RPP. Aiming at verifying behaviour during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG= ========== to ========== Summary ----- Adding new browser tests related to ResourcePrefetchPredictor Details ----- This patch is about two things: - Adding two new browser tests to the RPP. Aiming at verifying behaviour during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG= ==========
Patchset #2 (id:20001) has been deleted
Looks good after the second patch. I also have a couple of questions. Please add the bug number to the CL description. 650253 will be fine. https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:224: ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION | wait_flags); From offline discussion: we don't have to wait for loading completed in this call because we have our own observer. So we can just pass ui_test_utils::BROWSER_TEST_NONE flag. https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:217: (disposition == WindowOpenDisposition::NEW_POPUP)) { On 2016/12/07 14:13:02, ahemery wrote: > I believe this one was just forgotten. Behavior was buggy with > NEW_POPUP+NAVIGATION. We don't need it for this CL anymore. https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:145: std::vector<ResourcePrefetchPredictor::URLRequestSummary> Probably, it could be better to extract the code related to subresources comparison to the separate method and put it in anonymous namespace in this file. https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:146: subresources_results = GetUniqueSubresources(summary); nit: I'd prefer different naming. For example, actual_subresources/expected_subresources. https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: for (auto& subresource : subresources_ref) { nit: Remove curly brace. https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:153: for (auto& subresource : subresources_results) { ditto https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:532: AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE, Why font resource became first in the list?
Description was changed from ========== Summary ----- Adding new browser tests related to ResourcePrefetchPredictor Details ----- This patch is about two things: - Adding two new browser tests to the RPP. Aiming at verifying behaviour during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG= ========== to ========== Summary ----- Adding new browser tests related to ResourcePrefetchPredictor Details ----- This patch is about two things: - Adding two new browser tests to the RPP. Aiming at verifying behaviour during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 ==========
Patchset #3 (id:60001) has been deleted
On 2016/12/07 16:57:12, alexilin wrote: > Looks good after the second patch. I also have a couple of questions. > > Please add the bug number to the CL description. 650253 will be fine. > > https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2561493003/diff/1/chrome/browser/predictors/r... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:224: > ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION | wait_flags); > From offline discussion: we don't have to wait for loading completed in this > call because we have our own observer. > So we can just pass ui_test_utils::BROWSER_TEST_NONE flag. > > https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... > File chrome/test/base/ui_test_utils.cc (right): > > https://codereview.chromium.org/2561493003/diff/1/chrome/test/base/ui_test_ut... > chrome/test/base/ui_test_utils.cc:217: (disposition == > WindowOpenDisposition::NEW_POPUP)) { > On 2016/12/07 14:13:02, ahemery wrote: > > I believe this one was just forgotten. Behavior was buggy with > > NEW_POPUP+NAVIGATION. > > We don't need it for this CL anymore. > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:145: > std::vector<ResourcePrefetchPredictor::URLRequestSummary> > Probably, it could be better to extract the code related to subresources > comparison to the separate method and put it in anonymous namespace in this > file. > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:146: > subresources_results = GetUniqueSubresources(summary); > nit: I'd prefer different naming. For example, > actual_subresources/expected_subresources. > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: for > (auto& subresource : subresources_ref) { > nit: Remove curly brace. > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:153: for > (auto& subresource : subresources_results) { > ditto > > https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:532: > AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE, > Why font resource became first in the list? This was simply a mistake. Also reordered new tab to be before new popup, as it makes more sense to have a progression.
https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:145: std::vector<ResourcePrefetchPredictor::URLRequestSummary> On 2016/12/07 16:57:12, alexilin wrote: > Probably, it could be better to extract the code related to subresources > comparison to the separate method and put it in anonymous namespace in this > file. Extracted in CompareSubresources(...) https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:146: subresources_results = GetUniqueSubresources(summary); On 2016/12/07 16:57:11, alexilin wrote: > nit: I'd prefer different naming. For example, > actual_subresources/expected_subresources. Renamed https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: for (auto& subresource : subresources_ref) { On 2016/12/07 16:57:11, alexilin wrote: > nit: Remove curly brace. Done https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:532: AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE, On 2016/12/07 16:57:11, alexilin wrote: > Why font resource became first in the list? Corrected, see main comment
Lgtm with tiny nit, thanks! https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:532: AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE, On 2016/12/08 11:08:44, ahemery wrote: > On 2016/12/07 16:57:11, alexilin wrote: > > Why font resource became first in the list? > > Corrected, see main comment There wasn't any logical mistake it was just a little bit confusing :) Thanks for fixing this. https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:164: CompareSubresources(summary, summary_, match_navigation_id_); nit: I found a little be confusing that the function CompareSubresources accepts summaries. It would be probably better to pass there two vectors of resources explicitly. (Feel free to change GetUniqueSubresources function as well)
https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:112: void EmptyNavigationId(NavigationID* navigation_id) { nit: I think it would be clearer to change this to void SetValidNavigationID(URLRequestSummary* summary); https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:526: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInNewTab) { It's not entirely clear to me that these tests do something different from the existing ones, from looking at the code in ui_test_utils. What's the aim here?
https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:112: void EmptyNavigationId(NavigationID* navigation_id) { On 2016/12/08 15:07:44, Benoit L wrote: > nit: I think it would be clearer to change this to > > void SetValidNavigationID(URLRequestSummary* summary); Changed https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:164: CompareSubresources(summary, summary_, match_navigation_id_); On 2016/12/08 12:57:25, alexilin wrote: > nit: I found a little be confusing that the function CompareSubresources accepts > summaries. > It would be probably better to pass there two vectors of resources explicitly. > (Feel free to change GetUniqueSubresources function as well) Modified. The copy is now done by parameters and duplicate removal happens in place. https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:526: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInNewTab) { On 2016/12/08 15:07:44, Benoit L wrote: > It's not entirely clear to me that these tests do something different from the > existing ones, from looking at the code in ui_test_utils. > > What's the aim here? It does not test any specific behavior but is more about making sure nothing crazy happens for edge cases. In particular that navigating to a new tab or a new popup does not crash. Should help specifically when moving to tab_ids.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:526: IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInNewTab) { On 2016/12/09 14:19:16, ahemery wrote: > On 2016/12/08 15:07:44, Benoit L wrote: > > It's not entirely clear to me that these tests do something different from the > > existing ones, from looking at the code in ui_test_utils. > > > > What's the aim here? > It does not test any specific behavior but is more about making sure nothing > crazy happens for edge cases. In particular that navigating to a new tab or a > new popup does not crash. Should help specifically when moving to tab_ids. Ok, reading through the code in ui_test_utils seems to confirm that this indeed does something different. Can you hold off landing this CL until there is the actual codefor the tab id stuff in? The reasoning being that introducing tests that are not (yet) useful is not (yet) useful.
Actually that part with partial comparison would be useful for my oncoming CL (one of the navigations will create new renderer process). How to handle this better? a) ahemery land this CL without adding new tests. b) I just copy these changes to my CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Summary ----- Adding new browser tests related to ResourcePrefetchPredictor Details ----- This patch is about two things: - Adding two new browser tests to the RPP. Aiming at verifying behaviour during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 ========== to ========== Summary ----- Allows for custom comparison of results and exotic navigation setup. Details ----- - New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 ==========
On 2016/12/09 14:52:09, alexilin wrote: > Actually that part with partial comparison would be useful for my oncoming CL > (one of the navigations will create new renderer process). > How to handle this better? > a) ahemery land this CL without adding new tests. > b) I just copy these changes to my CL. I have removed the tests while retaining the improvement in testing fexibility. Updated description.
Thanks! Let's land the partial comparison only, so that alexilin@ can use it in the tests. https://codereview.chromium.org/2561493003/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2561493003/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:117: void CompareSubresources(std::vector<URLRequestSummary> actual_subresources, Passing vectors by value is generally discouraged, but in this case, I think it's fine. https://codereview.chromium.org/2561493003/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:210: bool match_navigation_id = Can you add a comment to explain this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:120001) has been deleted
As advised by Benoit, removed the navigation_id matching according to disposition for now
lgtm, thanks.
The CQ bit was checked by ahemery@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexilin@chromium.org Link to the patchset: https://codereview.chromium.org/2561493003/#ps140001 (title: "Post-Review Modifications 3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481553895375940,
"parent_rev": "468b6b979de218297cc093dce640cb0c16359f44", "commit_rev":
"63d0a01eecba09de2fe050a150ecfb405cf309ff"}
Message was sent while issue was closed.
Description was changed from ========== Summary ----- Allows for custom comparison of results and exotic navigation setup. Details ----- - New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 ========== to ========== Summary ----- Allows for custom comparison of results and exotic navigation setup. Details ----- - New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 Review-Url: https://codereview.chromium.org/2561493003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Summary ----- Allows for custom comparison of results and exotic navigation setup. Details ----- - New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 Review-Url: https://codereview.chromium.org/2561493003 ========== to ========== Summary ----- Allows for custom comparison of results and exotic navigation setup. Details ----- - New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate. - To be able to add the test cases we need to have some more customization than what the current test framework provides. Specifically, partial comparison and new navigation types. Implemented that. BUG=650253 Committed: https://crrev.com/23b1517fb1deb148503f67f1bc9f38283f4e89a0 Cr-Commit-Position: refs/heads/master@{#437877} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/23b1517fb1deb148503f67f1bc9f38283f4e89a0 Cr-Commit-Position: refs/heads/master@{#437877} |
