Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749633004:
Track telemetry benchmark cycle time (Closed)
DescriptionTrack telemetry benchmark cycle time
BUG=chromium:700086
Review-Url: https://codereview.chromium.org/2749633004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/bb15ceda2809b58f629b173e0af7689261bdb915
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add tests #
Total comments: 6
Patch Set 3 : Fix tests #
Total comments: 4
Patch Set 4 : Nits #Patch Set 5 : Remove unused import #
Total comments: 2
Messages
Total messages: 37 (13 generated)
martiniss@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com, sullivan@chromium.org
PTAL
On 2017/03/13 23:05:07, martiniss wrote: > PTAL Can you add test for this in story_runner_unittest.py? Also can you run a benchmark & compare this data with the data from time command. time tools/perf/run_benchmark run system_heath.desktop_common --browser=system
https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/story_runner.py:304: """ time should start here https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/story_runner.py:375: results.AddSummaryValue(scalar.ScalarValue( this should be move to finally block at 384
Description was changed from ========== Track telemetry benchmark cycle time BUG=chromium:#700086 ========== to ========== Track telemetry benchmark cycle time BUG=chromium:700086 ==========
On 2017/03/13 at 23:26:27, nednguyen wrote: > On 2017/03/13 23:05:07, martiniss wrote: > > PTAL > > Can you add test for this in story_runner_unittest.py? Also can you run a benchmark & compare this data with the data from time command. > > time tools/perf/run_benchmark run system_heath.desktop_common --browser=system Added tests. I misread this, and thought I should compare times with and without patch. I ran the test, but I can't find the data to make sure the time is accurate enough. :/
https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:372: duration = time.time() - start I mean duration should be after the finally block in line 381. That makes sure that if the resulst uploading takes too long, the timing is tracked https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1040: def testRunBenchmarkTimeDuration(self): Can you only mock time and not other story_runner.Run? See test in testPagesetRepeat. This test seems mostly test certain methods be called with some expected arguments. I generally prefer test that treat the state as blackbox & only asserting the outcome.
https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:383: None, 'BenchmarkDuration', 'minutes', duration / 60)) nits: divide by 60.0
FYI, this is stalled on me checking the values reported by the test. I ran it locally, and it gave me numbers which don't seem to correspond with how long the command actually took. I've been WFH for a while, and I don't exactly know how to run this test on a chromebook (which is what I have at home) :/ https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/story_runner.py:375: results.AddSummaryValue(scalar.ScalarValue( On 2017/03/13 at 23:57:21, nednguyen wrote: > this should be move to finally block at 384 Done.
On 2017/03/23 21:23:59, martiniss wrote: > FYI, this is stalled on me checking the values reported by the test. I ran it > locally, and it gave me numbers which don't seem to correspond with how long the > command actually took. Is it possible to test on a perf try job? https://www.chromium.org/developers/telemetry/performance-try-bots > I've been WFH for a while, and I don't exactly know how to run this test on a > chromebook (which is what I have at home) :/ > > https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/story_runner.py (right): > > https://codereview.chromium.org/2749633004/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/story_runner.py:375: > results.AddSummaryValue(scalar.ScalarValue( > On 2017/03/13 at 23:57:21, nednguyen wrote: > > this should be move to finally block at 384 > > Done.
That looks like it'll work. Thanks!
https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1040: def testRunBenchmarkTimeDuration(self): On 2017/03/17 20:49:32, nednguyen wrote: > Can you only mock time and not other story_runner.Run? See test in > testPagesetRepeat. > > This test seems mostly test certain methods be called with some expected > arguments. I generally prefer test that treat the state as blackbox & only > asserting the outcome. Ping
I'm going to punt review of this CL to Ned.
eakuefner@chromium.org changed reviewers: - eakuefner@chromium.org
martiniss@chromium.org changed reviewers: + eakuefner@chromium.org
https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:383: None, 'BenchmarkDuration', 'minutes', duration / 60)) On 2017/03/17 at 20:55:08, nednguyen wrote: > nits: divide by 60.0 Done https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2749633004/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1040: def testRunBenchmarkTimeDuration(self): On 2017/03/17 at 20:49:32, nednguyen wrote: > Can you only mock time and not other story_runner.Run? See test in testPagesetRepeat. > > This test seems mostly test certain methods be called with some expected arguments. I generally prefer test that treat the state as blackbox & only asserting the outcome. Done.
martiniss@chromium.org changed reviewers: - eakuefner@chromium.org
lgtm with nits nice test! https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1060: options = fakes._FakeBrowserFinderOptions() Use options = fakes.CreateBrowserFinderOptions() instead https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1087: self.assertTrue(abs(duration - 1) < 0.001) nits: self.assertAlmostEqual(duration, 1)
The CQ bit was checked by martiniss@chromium.org
https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1060: options = fakes._FakeBrowserFinderOptions() On 2017/03/23 at 23:26:52, nednguyen wrote: > Use options = fakes.CreateBrowserFinderOptions() instead Done. https://codereview.chromium.org/2749633004/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1087: self.assertTrue(abs(duration - 1) < 0.001) On 2017/03/23 at 23:26:52, nednguyen wrote: > nits: self.assertAlmostEqual(duration, 1) Done.
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2749633004/#ps60001 (title: "Nits")
The CQ bit was checked by martiniss@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: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by martiniss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2749633004/#ps80001 (title: "Remove unused import")
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": 80001, "attempt_start_ts": 1490390341666730,
"parent_rev": "c16e499aa7f367419c77bf2499b2ab63a3c1dcdd", "commit_rev":
"bb15ceda2809b58f629b173e0af7689261bdb915"}
Message was sent while issue was closed.
Description was changed from ========== Track telemetry benchmark cycle time BUG=chromium:700086 ========== to ========== Track telemetry benchmark cycle time BUG=chromium:700086 Review-Url: https://codereview.chromium.org/2749633004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2777673003/ by sullivan@chromium.org. The reason for reverting is: This is causing disabled tests to show up as red on perf waterfall. BUG=chromium:705251.
Message was sent while issue was closed.
https://codereview.chromium.org/2749633004/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:331: return 17 hmhh, this needs some unittest to avoid mistake in the future.
Message was sent while issue was closed.
https://codereview.chromium.org/2749633004/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2749633004/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:331: return 17 On 2017/03/26 at 01:46:04, nednguyen wrote: > hmhh, this needs some unittest to avoid mistake in the future. Wow, I don't remember doing this at all. We definitely should add some unit tests. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
