Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110653002:
[tools/perf] Add system_health_smoke_test that run all system health stories (Closed)
|
Created:
4 years, 5 months ago by nednguyen Modified:
4 years, 5 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, kouhei (in TOK), Kunihiko Sakamoto, charliea (OOO until 10-5), benjhayden Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tools/perf] Add system_health_smoke_test that run all system health stories
BUG=615382
Committed: https://crrev.com/8095da730db4f644cc7691acca63f32b85536bce
Cr-Commit-Position: refs/heads/master@{#403163}
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : Disable failing tests #
Total comments: 23
Patch Set 4 : Address Petr's comment #
Total comments: 26
Patch Set 5 : Address Petr's comments #Patch Set 6 : Disable search:ebay & news:cnn #Patch Set 7 : Disable load:media:dailymotion #Patch Set 8 : Disable even more stories :-( #
Total comments: 3
Messages
Total messages: 81 (47 generated)
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all the system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all the system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all the system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all the system health stories BUG=615382 ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all the system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
nednguyen@google.com changed reviewers: + perezju@chromium.org, petrcermak@chromium.org
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 ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for doing this :-) Here are my initial comments. Petr https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:5: """Run the first page of one benchmark for every module. I don't think that this is the case. You seem to be running ALL stories for SH benchmarks. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:7: Only benchmarks that have a composable measurement are included. Not sure what this means. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:25: # benchmarks should be using the same stories as memory's ones, only with less nit: s/less/fewer/ ("actions" are countable) https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:34: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.load:tools:dropbox', # pylint: disable=line-too-long Could you please add a bug for each of these? https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:42: # This smoke test dynamically tests all benchmarks. So disabling it for one s/all benchmarks/all system health user stories/ ? https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:50: benchmark_class.options['pageset_repeat'] = 1 Isn't this modifying the top-level benchmark? Why not set this on the SinglePageBenchmark options? https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:99: assert sys.modules[benchmark_class.__module__] is system_health This seems unnecessary https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:103: # HACK: this options should be derived from options_for_unittests which is nit: s/this options/these options/ https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:106: # Since none of our system health benchmark create stories based on This comment is not complete. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:110: benchmark_class().CreateStorySet(options).stories): Why do you name |options| when you could just pass None here? https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:114: setattr(SystemHealthBenchmarkSmokeTest, test_method_name, method) This seems unnecessarily complicated. Why don't you just define the whole class in SmokeTestGenerator (as opposed to defining just the test method)? You could do something like: for benchmark_class in _SH_BENCHMARKS_TO_SMOKE_TEST: for story_to_smoke_test in benchmark_class().CreateStorySet(None).stories: _AddSmokeTestForStory(suite, story_to_smoke_test) https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:116: suite.addTest(SystemHealthBenchmarkSmokeTest(test_method_name)) I suggest you name the parameter: SystemHealthBenchmarkSmokeTest(methodName=test_method_name) ^^^^^^^^^^^ to make the code more readable
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Thanks for your comments. PTAL again https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:5: """Run the first page of one benchmark for every module. On 2016/06/29 06:53:27, petrcermak wrote: > I don't think that this is the case. You seem to be running ALL stories for SH > benchmarks. Update the description completely. I was just copying & pasting :P https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:7: Only benchmarks that have a composable measurement are included. On 2016/06/29 06:53:27, petrcermak wrote: > Not sure what this means. Removed this part. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:25: # benchmarks should be using the same stories as memory's ones, only with less On 2016/06/29 06:53:27, petrcermak wrote: > nit: s/less/fewer/ ("actions" are countable) Done. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:34: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.load:tools:dropbox', # pylint: disable=line-too-long On 2016/06/29 06:53:28, petrcermak wrote: > Could you please add a bug for each of these? Done. Looks like memory_mobile.load:games:bubbles is no longer failing. But we need to keep an eye on it though https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:42: # This smoke test dynamically tests all benchmarks. So disabling it for one On 2016/06/29 06:53:27, petrcermak wrote: > s/all benchmarks/all system health user stories/ ? Done. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:50: benchmark_class.options['pageset_repeat'] = 1 On 2016/06/29 06:53:27, petrcermak wrote: > Isn't this modifying the top-level benchmark? Why not set this on the > SinglePageBenchmark options? Done. Set these in options to make sure instead. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:99: assert sys.modules[benchmark_class.__module__] is system_health On 2016/06/29 06:53:27, petrcermak wrote: > This seems unnecessary I was wanting to make sure that people don't add non sh benchmark by accident. But with the new method of gathering benchmark to smoke test, it's no longer necessary. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:103: # HACK: this options should be derived from options_for_unittests which is On 2016/06/29 06:53:28, petrcermak wrote: > nit: s/this options/these options/ Done. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:110: benchmark_class().CreateStorySet(options).stories): On 2016/06/29 06:53:27, petrcermak wrote: > Why do you name |options| when you could just pass None here? I thought it was easier to add documentation there. Anyway, just use named param. https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:114: setattr(SystemHealthBenchmarkSmokeTest, test_method_name, method) On 2016/06/29 06:53:27, petrcermak wrote: > This seems unnecessarily complicated. Why don't you just define the whole class > in SmokeTestGenerator (as opposed to defining just the test method)? You could > do something like: > > for benchmark_class in _SH_BENCHMARKS_TO_SMOKE_TEST: > for story_to_smoke_test in benchmark_class().CreateStorySet(None).stories: > _AddSmokeTestForStory(suite, story_to_smoke_test) Because we need to make the test method name contains "<benchmark name>.<story name>". Even if we define the whole class with the test method directly, we still need to do some name manipulating logic there. Anyway, I think it's a good idea to move this to _AddSmokeTestForStory https://codereview.chromium.org/2110653002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:116: suite.addTest(SystemHealthBenchmarkSmokeTest(test_method_name)) On 2016/06/29 06:53:28, petrcermak wrote: > I suggest you name the parameter: > > SystemHealthBenchmarkSmokeTest(methodName=test_method_name) > ^^^^^^^^^^^ > > to make the code more readable Done.
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...
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
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_...)
LGTM with a few more comments. Thanks, Petr https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:5: """Run all the system health stories used by system health benchmarks. nit: I'd move "the" in front of "system health benchmarks" instead https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:9: stories as memory's ones, only with fewer actions (no memory dumping). nitL s/memory's ones/the memory ones/ https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:49: Please remove at least one blank line here (or both) https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:53: # disabling it for one failing or flaky benchmark would disable a much you have a double space here after "one" and after "coverage" on the line below. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:55: # failing, disable it by putting it in _DISABLED_TESTS list above. nit: s/in/into the/ https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:57: def runTest(self): method names should use CamelCase (not camelCase) https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:60: nit: I think that this would be more readable with this blank line removed https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:70: options.pageset_repeat = 1 Why don't you put these two lines inside GenerateBenchmarkOptions? https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:99: def GenerateBenchmarkOptions(benchmark_class): I suppose this should be private (since it's only called by a private function) https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:101: options = options_for_unittests.GetCopy() This whole method is super-complicated and I have absolutely no idea what it means. Is it copied from somewhere else (duplication)? It seems like the smoke tests shouldn't have to know about all the ordering of parsing etc. If you want to leave this for some other time, please at least add a TODO. Thanks :-) https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:122: # HACK: these options should be derived from options_for_unittests which is nit: s/is/are/ https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:123: # the resolved options from run_tests' arguments. However, options is only nit: s/is/are/ https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:125: # Since none of our system health benchmark create stories based on command nit: s/benchmark/benchmarks/ and s/create/creates/
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:5: """Run all the system health stories used by system health benchmarks. On 2016/06/29 21:32:52, petrcermak wrote: > nit: I'd move "the" in front of "system health benchmarks" instead Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:9: stories as memory's ones, only with fewer actions (no memory dumping). On 2016/06/29 21:32:53, petrcermak wrote: > nitL s/memory's ones/the memory ones/ Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:49: On 2016/06/29 21:32:52, petrcermak wrote: > Please remove at least one blank line here (or both) Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:53: # disabling it for one failing or flaky benchmark would disable a much On 2016/06/29 21:32:52, petrcermak wrote: > you have a double space here after "one" and after "coverage" on the line below. Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:55: # failing, disable it by putting it in _DISABLED_TESTS list above. On 2016/06/29 21:32:53, petrcermak wrote: > nit: s/in/into the/ Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:57: def runTest(self): On 2016/06/29 21:32:52, petrcermak wrote: > method names should use CamelCase (not camelCase) Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:60: On 2016/06/29 21:32:53, petrcermak wrote: > nit: I think that this would be more readable with this blank line removed Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:70: options.pageset_repeat = 1 On 2016/06/29 21:32:52, petrcermak wrote: > Why don't you put these two lines inside GenerateBenchmarkOptions? Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:99: def GenerateBenchmarkOptions(benchmark_class): On 2016/06/29 21:32:53, petrcermak wrote: > I suppose this should be private (since it's only called by a private function) Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:101: options = options_for_unittests.GetCopy() On 2016/06/29 21:32:52, petrcermak wrote: > This whole method is super-complicated and I have absolutely no idea what it > means. Is it copied from somewhere else (duplication)? It seems like the smoke > tests shouldn't have to know about all the ordering of parsing etc. If you want > to leave this for some other time, please at least add a TODO. Thanks :-) Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:122: # HACK: these options should be derived from options_for_unittests which is On 2016/06/29 21:32:53, petrcermak wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:123: # the resolved options from run_tests' arguments. However, options is only On 2016/06/29 21:32:52, petrcermak wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2110653002/diff/60001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:125: # Since none of our system health benchmark create stories based on command On 2016/06/29 21:32:52, petrcermak wrote: > nit: s/benchmark/benchmarks/ and s/create/creates/ Done.
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110653002/#ps80001 (title: "Address Petr's comments")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110653002/#ps100001 (title: "Disable search:ebay & news:cnn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/29 22:50:19, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... From patch set 4 to the time of landing patch set 5, we already have 2 more pages failing on linux :-/ Oh well...
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110653002/#ps120001 (title: "Disable ")
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 nednguyen@google.com
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110653002/#ps140001 (title: "Disable load:media:dailymotion")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nednguyen@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nednguyen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110653002/#ps160001 (title: "Disable even more stories :-(")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Once again, thanks for doing this :-) https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health_smoke_test.py:42: # crbug.com/624474 Wow, this is a *lot* of failing tests. I guess it's time we started burning this list down :-)
https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health_smoke_test.py:43: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.load:tools:dropbox', # pylint: disable=line-too-long not sure if this would work, but if we put the "# pylint: disable=line-too-long" on it's own line within the [ ... ], would it be scoped to apply to the contents of the array but not outside? Alternatively, all benchmarks seem to start with "benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest", can be factor out that common part?
https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2110653002/diff/160001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health_smoke_test.py:41: _DISABLED_TESTS = [ This should be a frozenset: _DISABLED_TESTS = frozenset({ ... })
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: winx64_10_perf_cq on tryserver.chromium.perf (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_c...)
The CQ bit was checked by nednguyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
On 2016/06/30 13:02:17, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... folks, I will try to land this to lock down what we have a soon as possible before making further improvement ~_~ The "# pylint:disable=line-too-long...# pylint:enable=line-too-long" works for suppressing python linter but not depot_tools long line check
SGTM On Thu, Jun 30, 2016 at 2:13 PM <nednguyen@google.com> wrote: > On 2016/06/30 13:02:17, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > folks, I will try to land this to lock down what we have a soon as possible > before making further improvement ~_~ > > The "# pylint:disable=line-too-long...# pylint:enable=line-too-long" works > for > suppressing python linter but not depot_tools long line check > > https://codereview.chromium.org/2110653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by nednguyen@google.com
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...
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 ========== to ========== [tools/perf] Add system_health_smoke_test that run all system health stories BUG=615382 Committed: https://crrev.com/8095da730db4f644cc7691acca63f32b85536bce Cr-Commit-Position: refs/heads/master@{#403163} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8095da730db4f644cc7691acca63f32b85536bce Cr-Commit-Position: refs/heads/master@{#403163}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:160001) has been created in https://codereview.chromium.org/2117533002/ by kelvinp@chromium.org. The reason for reverting is: This change is failing on Android Tests bots. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg....
Message was sent while issue was closed.
On 2016/06/30 16:50:37, kelvinp wrote: > A revert of this CL (patchset #8 id:160001) has been created in > https://codereview.chromium.org/2117533002/ by mailto:kelvinp@chromium.org. > > The reason for reverting is: This change is failing on Android Tests bots. > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg.... Reland in https://codereview.chromium.org/2115623002/ |
