Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3020443002:
[Telemetry] Let --also-run-disabled-tests override StoryExpectations.DisableBenchmark (Closed)
|
Created:
3 years, 3 months ago by rnephew (Reviews Here) Modified:
3 years, 2 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org, shatch Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Let --also-run-disabled-tests override StoryExpectations.DisableBenchmark
Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set.
As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now.
BUG=catapult:#3896
Review-Url: https://chromiumcodereview.appspot.com/3020443002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/f7cc2170e1abdb0b92fe7126a6a7e3f1d2513783
Patch Set 1 #
Total comments: 1
Messages
Total messages: 21 (12 generated)
Description was changed from ========== [Telemetry] Make StoryExpectation DisableBenchmark be overridable. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ========== to ========== [Telemetry] Make StoryExpectation DisableBenchmark be overridable. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ==========
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
The CQ bit was checked by rnephew@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: This issue passed the CQ dry run.
On 2017/09/21 19:04:12, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. ping
Can you improve the description. I was confused a bit at first. Maybe something like: "Enable benchmarks disabled with StoryExpectation.DisableBenchmark to run when --also-run-disabled-tests flag is enabled"
Description was changed from ========== [Telemetry] Make StoryExpectation DisableBenchmark be overridable. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ========== to ========== [Telemetry] Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set" As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ==========
Description was changed from ========== [Telemetry] Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set" As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ========== to ========== [Telemetry] Make StoryExpectation DisableBenchmark be overridable. Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ==========
On 2017/09/22 21:44:46, nednguyen wrote: > Can you improve the description. I was confused a bit at first. Maybe something > like: > > "Enable benchmarks disabled with StoryExpectation.DisableBenchmark to run when > --also-run-disabled-tests flag is enabled" Done.
On 2017/09/22 23:01:49, rnephew (Reviews Here) wrote: > On 2017/09/22 21:44:46, nednguyen wrote: > > Can you improve the description. I was confused a bit at first. Maybe > something > > like: > > > > "Enable benchmarks disabled with StoryExpectation.DisableBenchmark to run when > > --also-run-disabled-tests flag is enabled" > > Done. Can you update the title as well? when I see "Make StoryExpectation DisableBenchmark be overridable", I thought of the word "overridable" in the sense of class inheritance, hence thought you mean making it possible for people to subclass StoryExpectation & override StoryExpectation.DisableBenchmark method Whereas what you really mean here is make it's possible to force running tests disabled with this method.
lgtm but please improve the title https://codereview.chromium.org/3020443002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/3020443002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/story_runner.py:289: temporarily_disabled = not decorators.IsBenchmarkEnabled( unrelated but are we ready to get rid of this?
Description was changed from ========== [Telemetry] Make StoryExpectation DisableBenchmark be overridable. Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ========== to ========== [Telemetry] Let --also-run-disabled-tests override StoryExpectations.DisableBenchmark Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ==========
On 2017/09/25 18:06:35, nednguyen wrote: > lgtm but please improve the title > > https://codereview.chromium.org/3020443002/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/story_runner.py (right): > > https://codereview.chromium.org/3020443002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/story_runner.py:289: temporarily_disabled = not > decorators.IsBenchmarkEnabled( > unrelated but are we ready to get rid of this? Close. in another CL it will go away. I have a seperate chromium side CL that adds a presubmit that ensures no decorators are being used. I'll get rid of it when that lands.
The CQ bit was checked by rnephew@chromium.org
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": 1, "attempt_start_ts": 1506363599650580, "parent_rev":
"d25c2e7444e2ec47bb0035ff5ed6526381260fec", "commit_rev":
"f7cc2170e1abdb0b92fe7126a6a7e3f1d2513783"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Let --also-run-disabled-tests override StoryExpectations.DisableBenchmark Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 ========== to ========== [Telemetry] Let --also-run-disabled-tests override StoryExpectations.DisableBenchmark Enable benchmarks disabled with StoryExpectations.DisableBenchmark to run when --also-run-disabled-tests flag is set. As a holdover from when it was PermanentlyDisableBenchmark, we currently have it so that the disabling cannot be overridden with --also-run-disabled-tests. Since we now use SUPPORTED_PLATFORMS to indicate a benchmark cannot run on a given platform, and DisableBenchmark is for temporary disablings, it makes sense to make this overridable now. BUG=catapult:#3896 Review-Url: https://chromiumcodereview.appspot.com/3020443002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
