Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199333005:
[tools/perf] Add unittests coverage for system health benchmarks (Closed)
|
Created:
4 years, 4 months ago by nednguyen Modified:
4 years, 4 months ago Reviewers:
petrcermak CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tools/perf] Add unittests coverage for system health benchmarks
These tests include:
+ Asserting that all system health benchmarks set
ShouldTearDownStateAfterEachStoryRun to True
+ Asserting that all system health benchmarks' names have
prefix "system_health."
+ Asserting that all system health benchmarks use system
health story sets.
BUG=589726
Committed: https://crrev.com/3ed59d7bc3c1d0bc7ce94df3fd7d0decf6ce061b
Cr-Commit-Position: refs/heads/master@{#409583}
Patch Set 1 #Patch Set 2 #
Total comments: 16
Patch Set 3 : Address Petr's nits #
Total comments: 6
Patch Set 4 : Address nits #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== [tools/perf] Add tests to make sure that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True BUG=589726 ========== to ========== [tools/perf] Add tests to make sure that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Description was changed from ========== [tools/perf] Add tests to make sure that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ==========
nednguyen@google.com changed reviewers: + petrcermak@chromium.org
https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (left): https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:19: class _SystemHealthBenchmark(perf_benchmark.PerfBenchmark): Remove these because the benchmarks are disabled & they don't follow system health benchmarks' convention.
Looks good overall with a couple of comments. Thanks, Petr https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... File tools/perf/benchmarks/benchmark_unittest.py (right): https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:10: import sys nit: sort order ('s' < 'u') https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:12: from benchmarks import system_health nit: I would probably do `from benchmarks import system_health as system_health_benchmark` so that the name is unambiguous throughout the file. WDYT? https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:13: from page_sets.system_health import system_health_stories nit: sort order ('b' < 'c' < 'p'). If you want to keep the SH imports together, then I suggest you separate them using a blank line. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:65: for s in _GetAllSystemHealthBenchmarks(): nit: could you change the variable to |b| as well so that it's consistent with the rest of the test? https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:68: '%s must have name started with "system_health." prefix') you forgot to add `% s` https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:68: '%s must have name started with "system_health." prefix') also, I wonder whether '%r' wouldn't be more applicable: %s -> 'module.ClassName' %r -> '<class module.ClassName at 0x7f83d1757870>' Alternatively, how about we use '%s' with `b.Name()`? https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:79: continue nit: this should be indented only 2 spaces (-2) https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:79: continue Does this mean that the WebView benchmark shouldn't be a part of System Health or it should use the SH story set? https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (left): https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:19: class _SystemHealthBenchmark(perf_benchmark.PerfBenchmark): On 2016/08/02 19:21:49, nednguyen wrote: > Remove these because the benchmarks are disabled & they don't follow system > health benchmarks' convention. Acknowledged.
Should the SH unittests perhaps be in a separate unittest file (not the generic benchmark one)?
Description was changed from ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... File tools/perf/benchmarks/benchmark_unittest.py (right): https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:10: import sys On 2016/08/03 08:49:27, petrcermak wrote: > nit: sort order ('s' < 'u') Done. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:12: from benchmarks import system_health On 2016/08/03 08:49:27, petrcermak wrote: > nit: I would probably do `from benchmarks import system_health as > system_health_benchmark` so that the name is unambiguous throughout the file. > WDYT? Done. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:13: from page_sets.system_health import system_health_stories On 2016/08/03 08:49:26, petrcermak wrote: > nit: sort order ('b' < 'c' < 'p'). If you want to keep the SH imports together, > then I suggest you separate them using a blank line. Done. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:65: for s in _GetAllSystemHealthBenchmarks(): On 2016/08/03 08:49:27, petrcermak wrote: > nit: could you change the variable to |b| as well so that it's consistent with > the rest of the test? Done. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:68: '%s must have name started with "system_health." prefix') On 2016/08/03 08:49:27, petrcermak wrote: > also, I wonder whether '%r' wouldn't be more applicable: > > %s -> 'module.ClassName' > %r -> '<class module.ClassName at 0x7f83d1757870>' > > Alternatively, how about we use '%s' with `b.Name()`? Done. https://codereview.chromium.org/2199333005/diff/20001/tools/perf/benchmarks/b... tools/perf/benchmarks/benchmark_unittest.py:79: continue On 2016/08/03 08:49:27, petrcermak wrote: > Does this mean that the WebView benchmark shouldn't be a part of System Health > or it should use the SH story set? Probably it should use the SH story set. We probably should add blank case to sh story et.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/03 08:49:53, petrcermak wrote: > Should the SH unittests perhaps be in a separate unittest file (not the generic > benchmark one)? Done
LGTM with some final nits. Thanks, Petr https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_unittest.py (right): https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:27: def testNaming(self): nit: testNamePrefix might be better? https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:31: '%r must have name started with "system_health." prefix' % b) nit: s/started/starting/ https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:39: def testSystemHealthStorySetAreUsed(self): nit: s/Are/Is/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_s5_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL) winx64_10_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
Description was changed from ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ==========
https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_unittest.py (right): https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:27: def testNaming(self): On 2016/08/03 15:47:27, petrcermak wrote: > nit: testNamePrefix might be better? Done. https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:31: '%r must have name started with "system_health." prefix' % b) On 2016/08/03 15:47:27, petrcermak wrote: > nit: s/started/starting/ Done. https://codereview.chromium.org/2199333005/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_unittest.py:39: def testSystemHealthStorySetAreUsed(self): On 2016/08/03 15:47:27, petrcermak wrote: > nit: s/Are/Is/ Done.
Description was changed from ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Description was changed from ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ==========
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/2199333005/#ps60001 (title: "Address nits")
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 unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 ========== to ========== [tools/perf] Add unittests coverage for system health benchmarks These tests include: + Asserting that all system health benchmarks set ShouldTearDownStateAfterEachStoryRun to True + Asserting that all system health benchmarks' names have prefix "system_health." + Asserting that all system health benchmarks use system health story sets. BUG=589726 Committed: https://crrev.com/3ed59d7bc3c1d0bc7ce94df3fd7d0decf6ce061b Cr-Commit-Position: refs/heads/master@{#409583} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3ed59d7bc3c1d0bc7ce94df3fd7d0decf6ce061b Cr-Commit-Position: refs/heads/master@{#409583} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
