Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
603753003:
Page runner clean-up. Page runner tests still pass. (Closed)
DescriptionPage runner clean-up. Page runner tests still pass.
BUG=
Committed: https://crrev.com/a43bba0c4f1b42e0cab1322abd501a7aba2b85d5
Cr-Commit-Position: refs/heads/master@{#296746}
Patch Set 1 #Patch Set 2 : Add xrange tweak too. #Patch Set 3 : Update _CheckArchives #
Total comments: 6
Patch Set 4 : Handle review comments. #Patch Set 5 : Rebase. #
Total comments: 1
Patch Set 6 : Fix counter bug. #Messages
Total messages: 19 (6 generated)
slamm@chromium.org changed reviewers: + chrishenry@google.com, nednguyen@google.com
Changes I had accumulated in my try-finally update of page_runner.
https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:405: pages_with_runs = set() There should be a better name for this. https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:426: page not in pages_with_runs and test.discard_first_result)) Sorry, this is hard to read and can be error-prone imo. Can you define discard_run in the line above? https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:465: pathless_pages = [] I think pathless_pages is not as descriptive as pages_missing_archive_path. Similar for dataless_pages
https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:405: pages_with_runs = set() On 2014/09/25 04:36:43, nednguyen wrote: > There should be a better name for this. I changed it to pages_with_discarded_first_result. However, could it simply check if the page_repeat index is zero? for page_repeat_index in xrange(finder_options.page_repeat): results.WillRunPage(page) try: _PrepareAndRunPage(...) finally: discard_run = test.discard_first_result and page_repeat_index == 0 ... https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:426: page not in pages_with_runs and test.discard_first_result)) On 2014/09/25 04:36:43, nednguyen wrote: > Sorry, this is hard to read and can be error-prone imo. Can you define > discard_run in the line above? Done. https://codereview.chromium.org/603753003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:465: pathless_pages = [] On 2014/09/25 04:36:43, nednguyen wrote: > I think pathless_pages is not as descriptive as pages_missing_archive_path. > Similar for dataless_pages Okay, I can see how the original names were better. I switched back. I also dropped calling itertools.chain() when adding FailureValue's to the results since the two lists are typically empty.
The CQ bit was checked by nednguyen@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603753003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/65589) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603753003/80001
https://codereview.chromium.org/603753003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/603753003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:247: attempt_num += attempt_num + 1 Is this really right?
The CQ bit was unchecked by chrishenry@google.com
(After fixing my one comment, this is lgtm, so don't block on my further lgtm.)
On 2014/09/25 16:57:14, chrishenry wrote: > (After fixing my one comment, this is lgtm, so don't block on my further lgtm.) OMG. Thank you for spotting that mistake.
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603753003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as e9c2d1ce8396f80be24d828316b646dd91039fa2
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a43bba0c4f1b42e0cab1322abd501a7aba2b85d5 Cr-Commit-Position: refs/heads/master@{#296746} |
