New issue
Advanced search Search tips

Issue 796340 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 6
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Clean up old code for telemetry expectations

Project Member Reported by rnep...@chromium.org, Dec 19 2017

Issue description

As part of the refactoring for TA/DA integration, we changed how we declare expectations. benchmark.py and expectations.py both have code cleanup that is needed.

expectations.py:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/story/expectations.py?q=f:catapult+expectations.py&sq=package:chromium&l=24
We no longer use the SetExpectations method to set them inside the benchmark classes, instead we use the Benchmark.AugmentExpectationsWithParser which calls into StoryExpectations.GetBenchmarkExpectationsFromParser.

benchmark.py:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/benchmark.py?type=cs&q=GetExpectations&sq=package:chromium&l=337
GetExpectations is no longer used to get a benchmarks expectations. Instead we use the expectations @property.

Another refactor that should be considered is merging the GetBenchmarkExpectationsFromParser() method with the StoryExpectations constructor, since the only use case for StoryExpectations after the other refactorings is to consume the expectations file. 
 
Status: Assigned (was: Untriaged)
Thanks for filing this Randy!
Components: Speed>Telemetry
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3deee3395e9dc7e0b31f96f16334261e3dec7bcb

commit 3deee3395e9dc7e0b31f96f16334261e3dec7bcb
Author: Charlie Andrews <charliea@chromium.org>
Date: Wed Dec 20 19:24:42 2017

Add expectations file for javaperftests

This file is the new (hopefully easier) way to disable Telemetry
benchmarks. To disable a benchmark, add a line that looks like:

 crbug.com/123456  [ MAC] run.CronetPerfTestBenchmark [ Skip ]

For more detailed instructions, see http://bit.ly/2BNw4Ca

This CL is part of a broader effort to eventually remove the
GetExpectations method altogether to simplify the Telemetry API.

Bug:  796340 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I32c6d55ff9ce890cd9ab0e9ec1101e882166a695
Reviewed-on: https://chromium-review.googlesource.com/837027
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525405}
[add] https://crrev.com/3deee3395e9dc7e0b31f96f16334261e3dec7bcb/components/cronet/android/test/javaperftests/expectations.config
[modify] https://crrev.com/3deee3395e9dc7e0b31f96f16334261e3dec7bcb/components/cronet/android/test/javaperftests/run.py

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/599ff1ddd17baf30f5037426389d2b9558c06080

commit 599ff1ddd17baf30f5037426389d2b9558c06080
Author: Charlie Andrews <charliea@chromium.org>
Date: Wed Dec 20 21:45:15 2017

Remove all instances of Benchmark.SetExpectations in tools/perf

These are vestiges of the old benchmark disabling method. Most of these
seem to be expectations that were associated with a story set.

Bug:  796340 
Change-Id: I6cf60a0a641bbeb7fc6290188b169ef5b801f9f6
Reviewed-on: https://chromium-review.googlesource.com/836450
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525472}
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/core/perf_data_generator_unittest.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/idle_after_loading_stories.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/intl_ar_fa_he.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/intl_es_fr_pt-BR.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/intl_hi_ru.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/intl_ja_zh.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/intl_ko_th_vi.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/key_mobile_sites_smooth.py
[modify] https://crrev.com/599ff1ddd17baf30f5037426389d2b9558c06080/tools/perf/page_sets/top_25_pages.py

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/3b3f9e1e789dbed2726dcd3c97d9dcd080ca7f23

commit 3b3f9e1e789dbed2726dcd3c97d9dcd080ca7f23
Author: Charlie Andrews <charliea@chromium.org>
Date: Fri Dec 22 16:21:05 2017

Remove Benchmark.GetExpectations

Benchmark.GetExpectations used to provide a way to disable benchmarks
and stories within benchmarks, but has since been replaced with a
central, project-specific expectations.config file.

Bug:  chromium:796340 
Change-Id: Ic19ed3eea9eabccce93f6fb35fe108837ab105bb
Reviewed-on: https://chromium-review.googlesource.com/837720
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/3b3f9e1e789dbed2726dcd3c97d9dcd080ca7f23/telemetry/telemetry/benchmark.py
[modify] https://crrev.com/3b3f9e1e789dbed2726dcd3c97d9dcd080ca7f23/telemetry/telemetry/benchmark_unittest.py
[modify] https://crrev.com/3b3f9e1e789dbed2726dcd3c97d9dcd080ca7f23/telemetry/telemetry/benchmark_runner_unittest.py

Project Member

Comment 6 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/e853531767d21d94507e2f0b786b374de030248b

commit e853531767d21d94507e2f0b786b374de030248b
Author: nednguyen <nednguyen@google.com>
Date: Mon May 07 20:09:27 2018

Remove deadcode related to old test disabling system in Telemetry story runner

Bug:  chromium:796340 
Change-Id: Ic3ed94eab73400c07181fd0ccded28c0b5fe6ab8
Reviewed-on: https://chromium-review.googlesource.com/1048168
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>

[modify] https://crrev.com/e853531767d21d94507e2f0b786b374de030248b/telemetry/telemetry/internal/story_runner.py

Project Member

Comment 7 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a219f660877f87796879022f119d9ff84b12fdaa

commit a219f660877f87796879022f119d9ff84b12fdaa
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon May 07 23:13:52 2018

Roll src/third_party/catapult/ e54b6ffb6..e85353176 (2 commits)

https://chromium.googlesource.com/catapult.git/+log/e54b6ffb6dad..e853531767d2

$ git log e54b6ffb6..e85353176 --date=short --no-merges --format='%ad %ae %s'
2018-05-07 nednguyen Remove deadcode related to old test disabling system in Telemetry story runner
2018-05-04 dtu [pinpoint] Look in new location for perf results file.

Created with:
  roll-dep src/third_party/catapult
BUG= chromium:796340 , chromium:836037 


The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=sullivan@chromium.org

Change-Id: Id4ca3154cc36c121ee3a7b9347df5053decf30f3
Reviewed-on: https://chromium-review.googlesource.com/1048307
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#556604}
[modify] https://crrev.com/a219f660877f87796879022f119d9ff84b12fdaa/DEPS

Status: Fixed (was: Assigned)
Marking this as "Fixed" - I think the most obvious vestiges of the old system are now gone.

Comment 9 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 10 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Speed>Telemetry

Sign in to add a comment