Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552303002:
[tools/perf] Add google search browsing story (Closed)
Description[tools/perf] Add google search browsing story
BUG=669582
Committed: https://crrev.com/36f870923f3bda18482660d445c3c6695c6e7ceb
Cr-Commit-Position: refs/heads/master@{#437231}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address Juan's comment & add WPR recording #
Total comments: 2
Messages
Total messages: 28 (18 generated)
nednguyen@google.com changed reviewers: + hjd@chromium.org, perezju@chromium.org, rnephew@chromium.org
For first pass review, you can patch this CL & run: "./tools/perf/run_benchmark --browser=system system_health.memory_desktop --story-filter="browse:search" --use-live-sites" Once the content of the story is agreed-upon, I will add the record WPR. Note that this story is created by combining the two different search stories from Kenji's spreadsheet. I hope this makes the story is more realistic & we have less stories to maintain overall.
This user story looking good to me. Higher level question about user stories organization: How does this fit with the existing search:portal:google story in searching_stories.py? Should this one replace that? Do we want to keep both of the stories? https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:263: text='Flower - Wikipedia') nit: should fit in one line? https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:278: action_runner.Wait(1) remove this and make the previous a Wait(2) ? https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:285: action_runner.ScrollPageToElement(selector='a[aria-label=\'Page 2\']') 1) I think you should be able to write this as selector='a[aria-label="Page 2"]' to keep consistent with _SEARCH_BOX_SELECTOR above. (And, as far as I can tell, should work even before landing https://codereview.chromium.org/2559503002/) 2) Maybe also introduce a _PAGE_2_SELECTOR?
The CQ bit was checked by nednguyen@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:263: text='Flower - Wikipedia') On 2016/12/07 11:12:06, perezju wrote: > nit: should fit in one line? Done. https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:278: action_runner.Wait(1) On 2016/12/07 11:12:06, perezju wrote: > remove this and make the previous a Wait(2) ? Done.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
The CQ bit was unchecked by nednguyen@google.com
The CQ bit was checked by nednguyen@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nednguyen@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/07 11:12:06, perezju wrote: > This user story looking good to me. > > Higher level question about user stories organization: > > How does this fit with the existing search:portal:google story in > searching_stories.py? Should this one replace that? Do we want to keep both of > the stories? Yes! I am thinking about removing search:portal:google as well since this seems to cover all the actions in that story. For now let us have both & then remove search:portal:google later > > https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:263: text='Flower - > Wikipedia') > nit: should fit in one line? > > https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:278: > action_runner.Wait(1) > remove this and make the previous a Wait(2) ? > > https://codereview.chromium.org/2552303002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:285: > action_runner.ScrollPageToElement(selector='a[aria-label=\'Page 2\']') > 1) I think you should be able to write this as selector='a[aria-label="Page 2"]' > to keep consistent with _SEARCH_BOX_SELECTOR above. (And, as far as I can tell, > should work even before landing https://codereview.chromium.org/2559503002/) > > 2) Maybe also introduce a _PAGE_2_SELECTOR?
On 2016/12/07 20:29:50, nednguyen wrote: > On 2016/12/07 11:12:06, perezju wrote: > > This user story looking good to me. > > > > Higher level question about user stories organization: > > > > How does this fit with the existing search:portal:google story in > > searching_stories.py? Should this one replace that? Do we want to keep both of > > the stories? > > Yes! I am thinking about removing search:portal:google as well since this seems > to cover all the actions in that story. For now let us have both & then remove > search:portal:google later sgtm, and this CL lgtm w/nit
https://codereview.chromium.org/2552303002/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2552303002/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:257: _SEARCH_PAGE_2_SELECTOR = 'a[aria-label=\'Page 2\']' nit: for consistency, can this be 'a[aria-label="Page 2"]'?
https://codereview.chromium.org/2552303002/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2552303002/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:257: _SEARCH_PAGE_2_SELECTOR = 'a[aria-label=\'Page 2\']' On 2016/12/08 10:01:28, perezju wrote: > nit: for consistency, can this be 'a[aria-label="Page 2"]'? No, the escaping is not right. If I use '"', it throws error
The CQ bit was checked by nednguyen@google.com
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": 20001, "attempt_start_ts": 1481200224578320,
"parent_rev": "d7385891b138c1f7973b3c6bd72372bfd39393d8", "commit_rev":
"0a19583595a9c45331499caaa65f5c872393eb28"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add google search browsing story BUG=669582 ========== to ========== [tools/perf] Add google search browsing story BUG=669582 Committed: https://crrev.com/36f870923f3bda18482660d445c3c6695c6e7ceb Cr-Commit-Position: refs/heads/master@{#437231} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/36f870923f3bda18482660d445c3c6695c6e7ceb Cr-Commit-Position: refs/heads/master@{#437231} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
