Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887983003:
[Telemetry] Make story runner use story expectations. (Closed)
|
Created:
3 years, 7 months ago by rnephew (Reviews Here) Modified:
3 years, 7 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Make story runner use story expectations.
This is a follow up CL to https://codereview.chromium.org/2843403005/.
This CL uses the newly created StoryExpectations to determine if it should
disable a benchmark/story.
go/StoryExpectationsDD
BUG=chromium:711065
Review-Url: https://codereview.chromium.org/2887983003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/8194c485cd9ab518a44ba8464a987bf55000b3bc
Patch Set 1 #Patch Set 2 : add test for overriding story level disabling by command line option #
Total comments: 2
Patch Set 3 : DisableBenchmark -> PermanentlyDisableBenchmark #Patch Set 4 : [Telemetry] Make story runner use story expectations. #
Total comments: 6
Patch Set 5 : [Telemetry] Make story runner use story expectations. #
Total comments: 16
Patch Set 6 : [Telemetry] Make story runner use story expectations. #
Messages
Total messages: 27 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:243: disabled = expectations.IsStoryDisabled(story, state.platform) What about "--also-run-disabled-tests" flag? Now I think about this, you DisableBenchmark probably should be renamed to DisableBenchmarkPermanently because we probably don't want to run loading.mobile on desktop even if people pass in "--also-run-disabled-tests" flag
https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:243: disabled = expectations.IsStoryDisabled(story, state.platform) On 2017/05/17 19:42:21, nednguyen wrote: > What about "--also-run-disabled-tests" flag? > > Now I think about this, you DisableBenchmark probably should be renamed to > DisableBenchmarkPermanently because we probably don't want to run loading.mobile > on desktop even if people pass in "--also-run-disabled-tests" flag I could foresee someone wanting to do that locally. I have myself done that locally (wanted to run a pageset for a mobile device on the system browser just to check that the pageset recorded properly and not wanting to stare at the mobile device screen).
On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > File telemetry/telemetry/internal/story_runner.py (right): > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/story_runner.py:243: disabled = > expectations.IsStoryDisabled(story, state.platform) > On 2017/05/17 19:42:21, nednguyen wrote: > > What about "--also-run-disabled-tests" flag? > > > > Now I think about this, you DisableBenchmark probably should be renamed to > > DisableBenchmarkPermanently because we probably don't want to run > loading.mobile > > on desktop even if people pass in "--also-run-disabled-tests" flag > > I could foresee someone wanting to do that locally. I have myself done that > locally (wanted to run a pageset for a mobile device on the system browser just > to check that the pageset recorded properly and not wanting to stare at the > mobile device screen). Interesting. What about story that involve native Android action? e.g: https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc...
On 2017/05/17 20:02:12, nednguyen wrote: > On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/story_runner.py (right): > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > telemetry/telemetry/internal/story_runner.py:243: disabled = > > expectations.IsStoryDisabled(story, state.platform) > > On 2017/05/17 19:42:21, nednguyen wrote: > > > What about "--also-run-disabled-tests" flag? > > > > > > Now I think about this, you DisableBenchmark probably should be renamed to > > > DisableBenchmarkPermanently because we probably don't want to run > > loading.mobile > > > on desktop even if people pass in "--also-run-disabled-tests" flag > > > > I could foresee someone wanting to do that locally. I have myself done that > > locally (wanted to run a pageset for a mobile device on the system browser > just > > to check that the pageset recorded properly and not wanting to stare at the > > mobile device screen). > > Interesting. What about story that involve native Android action? e.g: > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc... That.. probably wouldn't work well. I also dont know if my scenario above is one we want to officially support anyway. I can change it to permanent disable and have it not be override-able.
On 2017/05/17 20:37:51, rnephew (Reviews Here) wrote: > On 2017/05/17 20:02:12, nednguyen wrote: > > On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > File telemetry/telemetry/internal/story_runner.py (right): > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > telemetry/telemetry/internal/story_runner.py:243: disabled = > > > expectations.IsStoryDisabled(story, state.platform) > > > On 2017/05/17 19:42:21, nednguyen wrote: > > > > What about "--also-run-disabled-tests" flag? > > > > > > > > Now I think about this, you DisableBenchmark probably should be renamed to > > > > DisableBenchmarkPermanently because we probably don't want to run > > > loading.mobile > > > > on desktop even if people pass in "--also-run-disabled-tests" flag > > > > > > I could foresee someone wanting to do that locally. I have myself done that > > > locally (wanted to run a pageset for a mobile device on the system browser > > just > > > to check that the pageset recorded properly and not wanting to stare at the > > > mobile device screen). > > > > Interesting. What about story that involve native Android action? e.g: > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc... > > That.. probably wouldn't work well. I also dont know if my scenario above is one > we want to officially support anyway. I can change it to permanent disable and > have it not be override-able. sgtm. In general, being a little conservative in supporting features usually makes it easier to evolve the product later.
On 2017/05/17 20:39:40, nednguyen wrote: > On 2017/05/17 20:37:51, rnephew (Reviews Here) wrote: > > On 2017/05/17 20:02:12, nednguyen wrote: > > > On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > File telemetry/telemetry/internal/story_runner.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > telemetry/telemetry/internal/story_runner.py:243: disabled = > > > > expectations.IsStoryDisabled(story, state.platform) > > > > On 2017/05/17 19:42:21, nednguyen wrote: > > > > > What about "--also-run-disabled-tests" flag? > > > > > > > > > > Now I think about this, you DisableBenchmark probably should be renamed > to > > > > > DisableBenchmarkPermanently because we probably don't want to run > > > > loading.mobile > > > > > on desktop even if people pass in "--also-run-disabled-tests" flag > > > > > > > > I could foresee someone wanting to do that locally. I have myself done > that > > > > locally (wanted to run a pageset for a mobile device on the system browser > > > just > > > > to check that the pageset recorded properly and not wanting to stare at > the > > > > mobile device screen). > > > > > > Interesting. What about story that involve native Android action? e.g: > > > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc... > > > > That.. probably wouldn't work well. I also dont know if my scenario above is > one > > we want to officially support anyway. I can change it to permanent disable and > > have it not be override-able. > > sgtm. In general, being a little conservative in supporting features usually > makes it easier to evolve the product later. Done.
On 2017/05/17 20:50:55, rnephew (Reviews Here) wrote: > On 2017/05/17 20:39:40, nednguyen wrote: > > On 2017/05/17 20:37:51, rnephew (Reviews Here) wrote: > > > On 2017/05/17 20:02:12, nednguyen wrote: > > > > On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > > File telemetry/telemetry/internal/story_runner.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > > telemetry/telemetry/internal/story_runner.py:243: disabled = > > > > > expectations.IsStoryDisabled(story, state.platform) > > > > > On 2017/05/17 19:42:21, nednguyen wrote: > > > > > > What about "--also-run-disabled-tests" flag? > > > > > > > > > > > > Now I think about this, you DisableBenchmark probably should be > renamed > > to > > > > > > DisableBenchmarkPermanently because we probably don't want to run > > > > > loading.mobile > > > > > > on desktop even if people pass in "--also-run-disabled-tests" flag > > > > > > > > > > I could foresee someone wanting to do that locally. I have myself done > > that > > > > > locally (wanted to run a pageset for a mobile device on the system > browser > > > > just > > > > > to check that the pageset recorded properly and not wanting to stare at > > the > > > > > mobile device screen). > > > > > > > > Interesting. What about story that involve native Android action? e.g: > > > > > > > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc... > > > > > > That.. probably wouldn't work well. I also dont know if my scenario above is > > one > > > we want to officially support anyway. I can change it to permanent disable > and > > > have it not be override-able. > > > > sgtm. In general, being a little conservative in supporting features usually > > makes it easier to evolve the product later. > > Done. Sorry for not being clear, can you move the stuffs related to disabling at benchmarks level to a separate CL for the ease of review? Otherwise, thing looks lg2me.
On 2017/05/17 20:57:46, nednguyen wrote: > On 2017/05/17 20:50:55, rnephew (Reviews Here) wrote: > > On 2017/05/17 20:39:40, nednguyen wrote: > > > On 2017/05/17 20:37:51, rnephew (Reviews Here) wrote: > > > > On 2017/05/17 20:02:12, nednguyen wrote: > > > > > On 2017/05/17 19:47:43, rnephew (Reviews Here) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > > > File telemetry/telemetry/internal/story_runner.py (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2887983003/diff/60001/telemetry/telemetry/int... > > > > > > telemetry/telemetry/internal/story_runner.py:243: disabled = > > > > > > expectations.IsStoryDisabled(story, state.platform) > > > > > > On 2017/05/17 19:42:21, nednguyen wrote: > > > > > > > What about "--also-run-disabled-tests" flag? > > > > > > > > > > > > > > Now I think about this, you DisableBenchmark probably should be > > renamed > > > to > > > > > > > DisableBenchmarkPermanently because we probably don't want to run > > > > > > loading.mobile > > > > > > > on desktop even if people pass in "--also-run-disabled-tests" flag > > > > > > > > > > > > I could foresee someone wanting to do that locally. I have myself done > > > that > > > > > > locally (wanted to run a pageset for a mobile device on the system > > browser > > > > > just > > > > > > to check that the pageset recorded properly and not wanting to stare > at > > > the > > > > > > mobile device screen). > > > > > > > > > > Interesting. What about story that involve native Android action? e.g: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc... > > > > > > > > That.. probably wouldn't work well. I also dont know if my scenario above > is > > > one > > > > we want to officially support anyway. I can change it to permanent disable > > and > > > > have it not be override-able. > > > > > > sgtm. In general, being a little conservative in supporting features usually > > > makes it easier to evolve the product later. > > > > Done. > > Sorry for not being clear, can you move the stuffs related to disabling at > benchmarks level to a separate CL for the ease of review? Otherwise, thing looks > lg2me. Removed from this cl and created https://codereview.chromium.org/2885173004/ This CL cannot land until that one does.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:326: hard_disable = expectations.IsBenchmarkDisabled(possible_browser.platform) I think just rename these: hard_disable --> permanently_disabled soft_disabled --> temporarily_disabled Also don't do the soft_disable = (hard_disable .." part since it's true logically below but make the code harder to read. https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:327: soft_disable = (hard_disable or not Can you add a TODO to remove decorators.IsBenchmarkEnabled? https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:330: if possible_browser and soft_disable: We can actually remove "possible_browser" check because if it's None , the test would exit at line 324 already.
rnephew@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:326: hard_disable = expectations.IsBenchmarkDisabled(possible_browser.platform) On 2017/05/18 14:36:48, nednguyen wrote: > I think just rename these: > hard_disable --> permanently_disabled > soft_disabled --> temporarily_disabled > > Also don't do the soft_disable = (hard_disable .." part since it's true > logically below but make the code harder to read. Done. https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:327: soft_disable = (hard_disable or not On 2017/05/18 14:36:47, nednguyen wrote: > Can you add a TODO to remove decorators.IsBenchmarkEnabled? Done. https://codereview.chromium.org/2887983003/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:330: if possible_browser and soft_disable: On 2017/05/18 14:36:47, nednguyen wrote: > We can actually remove "possible_browser" check because if it's None , the test > would exit at line 324 already. Done.
lgtm w/ comments, but I defer to Ned for ultimate approval https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:242: if expectations is not None: can you just do if expectations: https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:176: def SetDisabled(self, b): I'm not 100% sure, but I'm guessing that this is the wrong thing to do: we probably either want to use python's @property and to make it self._disabled (to denote that disabled is private), or just have it be self.disabled and access it directly nednguyen@google.com, what's the idiomatic style in catapult? https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:636: # Class used in the next 4 tests This comment seems destined to go out of date https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:684: # Class used in the next 3 tests. same https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:689: def testRunStoryDisabledStory(self): Great unit tests :-) I appreciated you covering all of the behaviors, each in a different test! https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:1157: options.output_dir = tmp_path /applause for being like the only one I've seen to actually create a named temp file the right way
https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:242: if expectations is not None: On 2017/05/18 19:59:47, charliea wrote: > can you just do > > if expectations: Yeah, this is a preferred according to python style guide https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:176: def SetDisabled(self, b): On 2017/05/18 19:59:48, charliea wrote: > I'm not 100% sure, but I'm guessing that this is the wrong thing to do: we > probably either want to use python's @property and to make it self._disabled (to > denote that disabled is private), or just have it be self.disabled and access it > directly > > mailto:nednguyen@google.com, what's the idiomatic style in catapult? IIRC, Set.. & Get.. usually mean they have side effect, or take long time to run, or multiple variables. @property is for the case of very simple attribute access but you care about type check, or data validation. I've seen both in catapult/, but @property probably happens more often. Python style guide also have some guideline on this topic: https://google.github.io/styleguide/pyguide.html?showone=Properties#Properties
lgtm with nits Good tests! https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:685: class _DisableStoryExpectations(story_module.expectations.StoryExpectations): Can you move the definition of these classes to the top level? Defining class inside class seems a bit weird. If you care about making sure that the class definition is nearby the test code, you can do: class _DisableStoryExpectations(..): ... class DisablingTests(StoryRunnerTest): def testRunStoryDisabledStory def testRunStoryOneDisabledOneNot ....
https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:1180: seems like there are too many blank lines here
https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:242: if expectations is not None: On 2017/05/18 20:35:17, nednguyen wrote: > On 2017/05/18 19:59:47, charliea wrote: > > can you just do > > > > if expectations: > > Yeah, this is a preferred according to python style guide Done. https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:176: def SetDisabled(self, b): On 2017/05/18 20:35:17, nednguyen wrote: > On 2017/05/18 19:59:48, charliea wrote: > > I'm not 100% sure, but I'm guessing that this is the wrong thing to do: we > > probably either want to use python's @property and to make it self._disabled > (to > > denote that disabled is private), or just have it be self.disabled and access > it > > directly > > > > mailto:nednguyen@google.com, what's the idiomatic style in catapult? > > IIRC, Set.. & Get.. usually mean they have side effect, or take long time to > run, or multiple variables. @property is for the case of very simple attribute > access but you care about type check, or data validation. > > I've seen both in catapult/, but @property probably happens more often. > > Python style guide also have some guideline on this topic: > https://google.github.io/styleguide/pyguide.html?showone=Properties#Properties Switched to using @property for this. https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:636: # Class used in the next 4 tests On 2017/05/18 19:59:47, charliea wrote: > This comment seems destined to go out of date Done. https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:684: # Class used in the next 3 tests. On 2017/05/18 19:59:47, charliea wrote: > same Done. https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:685: class _DisableStoryExpectations(story_module.expectations.StoryExpectations): On 2017/05/18 20:40:47, nednguyen wrote: > Can you move the definition of these classes to the top level? Defining class > inside class seems a bit weird. > > If you care about making sure that the class definition is nearby the test code, > you can do: > > class _DisableStoryExpectations(..): > ... > > class DisablingTests(StoryRunnerTest): > def testRunStoryDisabledStory > def testRunStoryOneDisabledOneNot > .... > Done. https://codereview.chromium.org/2887983003/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner_unittest.py:1180: On 2017/05/18 20:41:38, nednguyen wrote: > seems like there are too many blank lines here Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2887983003/#ps160001 (title: "[Telemetry] Make story runner use story expectations.")
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": 160001, "attempt_start_ts": 1495141152546300,
"parent_rev": "442398620eeaedc297f0b717493a1fcf291d6436", "commit_rev":
"8194c485cd9ab518a44ba8464a987bf55000b3bc"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Make story runner use story expectations. This is a follow up CL to https://codereview.chromium.org/2843403005/. This CL uses the newly created StoryExpectations to determine if it should disable a benchmark/story. go/StoryExpectationsDD BUG=chromium:711065 ========== to ========== [Telemetry] Make story runner use story expectations. This is a follow up CL to https://codereview.chromium.org/2843403005/. This CL uses the newly created StoryExpectations to determine if it should disable a benchmark/story. go/StoryExpectationsDD BUG=chromium:711065 Review-Url: https://codereview.chromium.org/2887983003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
