Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843403005:
[Telemetry] Add Expectation module that enables disabling benchmarks/stories (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, ashleymarie1 Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Add Expectation module that enables disabling benchmarks/stories
This work is for better disabling of telemetry tests. This first
CL lays the ground works by creating the story_expectation object
and by extending the benchmark class api to include a way to
get story expectations. This implements the default behavior of creating
an empty expectation set if none is specified.
go/StoryExpectationsDD
BUG=chromium:711065
Review-Url: https://codereview.chromium.org/2843403005
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/3dc93e28655fd0318f9e35d40e66edadb17adb01
Patch Set 1 #Patch Set 2 : Add BattOr platform support #
Total comments: 10
Patch Set 3 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 28
Patch Set 4 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 6
Patch Set 5 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 12
Patch Set 6 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 2
Patch Set 7 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 30
Patch Set 8 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 14
Patch Set 9 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 36
Patch Set 10 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 2
Patch Set 11 : 30 -> 50 #
Total comments: 23
Patch Set 12 : [Telemetry] Have benchmarks have story expectations. #
Total comments: 6
Patch Set 13 : [Telemetry] Have benchmarks have story expectations. #
Messages
Total messages: 55 (13 generated)
Patchset #1 (id:1) has been deleted
rnephew@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org
https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... File telemetry/telemetry/story/story_expectations.py (right): https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. Let just name this file expectations.py https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:6: class Platforms(object): This naming may cause confusion because we already have https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... How about name this TestCondition? 'mac', 'win', 'no_battor' can also be thought test conditions https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:30: self._expectations = {'disabled_platforms': []} why make this a dictionary? Can you change this to: self._disabled_platforms = set()? https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:88: for platforms, reason in self._expectations.get(str(story), []): this code probably should be shared with the code from line 52 to 73? https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... File telemetry/telemetry/story/story_expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations_unittest.py:134: def testDisableStory(self): this single test method have too many tests in it. I suggest split it to few methods, each match a single theme. For example: testDisableStoryOnePlatform testDisableStoryAllPlatform ...
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... File telemetry/telemetry/story/story_expectations.py (right): https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/01 18:29:25, nednguyen wrote: > Let just name this file expectations.py Done. https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:6: class Platforms(object): On 2017/05/01 18:29:25, nednguyen wrote: > This naming may cause confusion because we already have > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > How about name this TestCondition? > 'mac', 'win', 'no_battor' can also be thought test conditions Done. https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:30: self._expectations = {'disabled_platforms': []} On 2017/05/01 18:29:25, nednguyen wrote: > why make this a dictionary? Can you change this to: > self._disabled_platforms = set()? Because I am storing the story disabling as a tuple of ([list, of, platforms], reason) and sets really dislike having lists as parts of tuples inside of them. >>> s = set() >>> s.add((['this', 'is', 'a', 'test'], 'Reason')) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unhashable type: 'list' http://stackoverflow.com/questions/13464152/typeerror-unhashable-type-list-wh... https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations.py:88: for platforms, reason in self._expectations.get(str(story), []): On 2017/05/01 18:29:25, nednguyen wrote: > this code probably should be shared with the code from line 52 to 73? Done. https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... File telemetry/telemetry/story/story_expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/40001/telemetry/telemetry/sto... telemetry/telemetry/story/story_expectations_unittest.py:134: def testDisableStory(self): On 2017/05/01 18:29:26, nednguyen wrote: > this single test method have too many tests in it. I suggest split it to few > methods, each match a single theme. For example: > testDisableStoryOnePlatform > testDisableStoryAllPlatform > ... Done.
I should have done git mv instead of renaming then git adding :/ Sorry about that; it messed up the diffs.
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:76: def DisableStory(self, story, platforms, reason=''): s/platforms/conditions/g we should also assert all the conditions are recognized ones https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] I don't think we should key by str(story). These 3 things must match: 1) The string we use for representing a story in the log output 2) The string we use for representing a story in the json output results (for example, the flickr in https://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf/buil...) 3) The string we use for representing the story we want to disable. +ashley: this should also be taken to account when we integrate Telemetry benchmark with flakiness dashboard.
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:6: class TestConditions(object): Throwing out for consideration a half-baked idea (feel free to disregard): I'm wondering whether each "condition" should be it's own class. The class can then include code to test whether the condition is met on a given state/browser (e.g. "no_battor", "is_svelte", "has_fancy_gpu"). That would be as opposed to having test condition names here, and code to test said conditions below in StoryExpectations. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:30: self._expectations = {'disabled_platforms': []} I know it's unlikely that we'll have a story named "disabled_platforms", but the potential for collisions stills makes me feel icky. At least let's use '_disabled_platforms' or, better, keep annotations for benchmark/stories separate. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:76: def DisableStory(self, story, platforms, reason=''): Let's not provide a default for "reason", it should always be given. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] On 2017/05/02 00:04:02, nednguyen wrote: > I don't think we should key by str(story). These 3 things must match: > > 1) The string we use for representing a story in the log output > 2) The string we use for representing a story in the json output results (for > example, the flickr in > https://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf/buil...) > 3) The string we use for representing the story we want to disable. > > +ashley: this should also be taken to account when we integrate Telemetry > benchmark with flakiness dashboard. Related, we should probably do an overall cleanup of story names; system health has set a good example there. As opposed to things like "http_en_m_wikipedia_org_wiki_Science" which are not good story names; and I hope I never have to type something like that to disable a story. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for when logging, I would also like to know the condition why the story got disabled, e.g. something like "load:news:cnn disabled on WIN due to crbug.com/123" So we probably need to return both the reason and the condition? https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations_unittest.py:32: nit: extra blank line here https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations_unittest.py:55: 'TEST') nit: use 'crbug/123' (or something like that) rather than "TEST" to set the tone of what we expect to be usually set as reason.
Description was changed from ========== [Telemetry] Have benchmarks have story expectations. This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ========== to ========== [Telemetry] Have benchmarks have story expectations. This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ==========
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] On 2017/05/02 09:19:52, perezju wrote: > On 2017/05/02 00:04:02, nednguyen wrote: > > I don't think we should key by str(story). These 3 things must match: > > > > 1) The string we use for representing a story in the log output > > 2) The string we use for representing a story in the json output results (for > > example, the flickr in > > > https://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf/buil...) > > 3) The string we use for representing the story we want to disable. > > > > +ashley: this should also be taken to account when we integrate Telemetry > > benchmark with flakiness dashboard. > > Related, we should probably do an overall cleanup of story names; system health > has set a good example there. As opposed to things like > "http_en_m_wikipedia_org_wiki_Science" which are not good story names; and I > hope I never have to type something like that to disable a story. +1. I discussed with Ashley offline. We should do clean up of story/page names before proceeding further on this & json test results work.
nednguyen@google.com changed reviewers: + charliea@chromium.org
On 2017/05/02 16:44:42, nednguyen wrote: > https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... > File telemetry/telemetry/story/expectations.py (right): > > https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... > telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = > [] > On 2017/05/02 09:19:52, perezju wrote: > > On 2017/05/02 00:04:02, nednguyen wrote: > > > I don't think we should key by str(story). These 3 things must match: > > > > > > 1) The string we use for representing a story in the log output > > > 2) The string we use for representing a story in the json output results > (for > > > example, the flickr in > > > > > > https://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf/buil...) > > > 3) The string we use for representing the story we want to disable. > > > > > > +ashley: this should also be taken to account when we integrate Telemetry > > > benchmark with flakiness dashboard. > > > > Related, we should probably do an overall cleanup of story names; system > health > > has set a good example there. As opposed to things like > > "http_en_m_wikipedia_org_wiki_Science" which are not good story names; and I > > hope I never have to type something like that to disable a story. > > +1. I discussed with Ashley offline. We should do clean up of story/page names > before proceeding further on this & json test results work. +Charlie as reviewer for this
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:76: def DisableStory(self, story, platforms, reason=''): On 2017/05/02 09:19:52, perezju wrote: > Let's not provide a default for "reason", it should always be given. In addition, I think we should even assert that reason must be non-empty. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] On 2017/05/02 16:44:42, nednguyen wrote: > On 2017/05/02 09:19:52, perezju wrote: > > On 2017/05/02 00:04:02, nednguyen wrote: > > > I don't think we should key by str(story). These 3 things must match: > > > > > > 1) The string we use for representing a story in the log output > > > 2) The string we use for representing a story in the json output results > (for > > > example, the flickr in > > > > > > https://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf/buil...) > > > 3) The string we use for representing the story we want to disable. > > > > > > +ashley: this should also be taken to account when we integrate Telemetry > > > benchmark with flakiness dashboard. > > > > Related, we should probably do an overall cleanup of story names; system > health > > has set a good example there. As opposed to things like > > "http_en_m_wikipedia_org_wiki_Science" which are not good story names; and I > > hope I never have to type something like that to disable a story. > > +1. I discussed with Ashley offline. We should do clean up of story/page names > before proceeding further on this & json test results work. Discuss offline. We can move forward with this & do the story/page naming overhaul in parallel. Some notes: 1) We can add assert that story has "short name", so for benchmark with long story name, they need to do the long -> short name conversion to use this. 2) We should assert that story name does exist in corresponding story_set. Not clear where this assertion should happen then, but if sheriffs disable using a story name that doesn't exist, telemetry_perf_unittests hopefully will fail, block landing that CL.
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:6: class TestConditions(object): I really like this idea. Done. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:30: self._expectations = {'disabled_platforms': []} On 2017/05/02 09:19:52, perezju wrote: > I know it's unlikely that we'll have a story named "disabled_platforms", but the > potential for collisions stills makes me feel icky. > > At least let's use '_disabled_platforms' or, better, keep annotations for > benchmark/stories separate. Done. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:76: def DisableStory(self, story, platforms, reason=''): On 2017/05/02 09:19:52, perezju wrote: > Let's not provide a default for "reason", it should always be given. Done. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:76: def DisableStory(self, story, platforms, reason=''): On 2017/05/09 16:44:24, nednguyen wrote: > On 2017/05/02 09:19:52, perezju wrote: > > Let's not provide a default for "reason", it should always be given. > > In addition, I think we should even assert that reason must be non-empty. Done. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] 1 is done, is there a way to do 2 as a linter warning? I'm not all that familiar with adding checks to the linter. This seems like it would be a good thing to have at that level, but might be hard to actually put there. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/02 09:19:52, perezju wrote: > when logging, I would also like to know the condition why the story got > disabled, e.g. something like > > "load:news:cnn disabled on WIN due to crbug.com/123" > > So we probably need to return both the reason and the condition? Its returning a tuple of (is_disabled, reason) https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations_unittest.py:32: On 2017/05/02 09:19:52, perezju wrote: > nit: extra blank line here Done. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations_unittest.py:55: 'TEST') On 2017/05/02 09:19:52, perezju wrote: > nit: use 'crbug/123' (or something like that) rather than "TEST" to set the tone > of what we expect to be usually set as reason. Done.
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] On 2017/05/10 19:45:13, rnephew (Reviews Here) wrote: > 1 is done, is there a way to do 2 as a linter warning? I'm not all that familiar > with adding checks to the linter. This seems like it would be a good thing to > have at that level, but might be hard to actually put there. It's not easy to add linter warning, and I doubt that gonna be reliable enough given that code can come at all sort of different forms. https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, conditions, reason): Can you either: 1) Alias the TestCondition class, ALL = AllTestCondition, ALL_MAC = MacTestCondition, ... so that users can do: DisableStory('foo', [expectations.ALL_MAC, expectations.ANDROID_M, ...], 'crbug.com...') ? https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, conditions, reason): s/story/story_name/g
https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, conditions, reason): On 2017/05/10 20:01:39, nednguyen wrote: > Can you either: > 1) Alias the TestCondition class, ALL = AllTestCondition, ALL_MAC = > MacTestCondition, ... so that users can do: > > DisableStory('foo', [expectations.ALL_MAC, expectations.ANDROID_M, ...], > 'crbug.com...') ? 2) is naming those classes that way, but it could be a bit odd.
https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, conditions, reason): On 2017/05/10 20:02:14, nednguyen wrote: > On 2017/05/10 20:01:39, nednguyen wrote: > > Can you either: > > 1) Alias the TestCondition class, ALL = AllTestCondition, ALL_MAC = > > MacTestCondition, ... so that users can do: > > > > DisableStory('foo', [expectations.ALL_MAC, expectations.ANDROID_M, ...], > > 'crbug.com...') ? > > 2) is naming those classes that way, but it could be a bit odd. Yeah, I wasn't exactly happy with how it was before with using *TestCondition to set it before, the aliasing is a good solution I think. https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, conditions, reason): On 2017/05/10 20:01:39, nednguyen wrote: > s/story/story_name/g Done. https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:50: def IsStoryDisabled(self, story, state): I dont think that the story->story_name change should be /g. Here we are actually passing the story object so having it just be 'story' fits better. I could also change it to pass just the story name if you think thats better, since .display_name is the only part of the story being used.
On 2017/05/10 20:53:01, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... > File telemetry/telemetry/story/expectations.py (right): > > https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, > conditions, reason): > On 2017/05/10 20:02:14, nednguyen wrote: > > On 2017/05/10 20:01:39, nednguyen wrote: > > > Can you either: > > > 1) Alias the TestCondition class, ALL = AllTestCondition, ALL_MAC = > > > MacTestCondition, ... so that users can do: > > > > > > DisableStory('foo', [expectations.ALL_MAC, expectations.ANDROID_M, ...], > > > 'crbug.com...') ? > > > > 2) is naming those classes that way, but it could be a bit odd. > > Yeah, I wasn't exactly happy with how it was before with using *TestCondition to > set it before, the aliasing is a good solution I think. > > https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story, > conditions, reason): > On 2017/05/10 20:01:39, nednguyen wrote: > > s/story/story_name/g > > Done. > > https://codereview.chromium.org/2843403005/diff/100001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:50: def IsStoryDisabled(self, story, > state): > I dont think that the story->story_name change should be /g. Here we are > actually passing the story object so having it just be 'story' fits better. I > could also change it to pass just the story name if you think thats better, > since .display_name is the only part of the story being used. Right. I added '/g' without looking through the whole file :P
lgtm with nits Please wait for Juan & CHarlie https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:12: class Platform(object): You can use https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... & expand FakePlatform class if needed instead
https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:23: def DisableBenchmark(self, conditions, reason): Add doc string for this. Also include example of how one would disable this. (see example in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:32: def IsBenchmarkDisabled(self, state): Same https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story_name, conditions, reason): Ditto https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:50: def IsStoryDisabled(self, story, state): Ditto https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:64: class AllTestCondition(_TestCondition): let hide all these class with prefix "_" so users will use only the alias.
https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:23: def DisableBenchmark(self, conditions, reason): On 2017/05/10 21:10:57, nednguyen wrote: > Add doc string for this. Also include example of how one would disable this. > (see example in > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) Done. https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:32: def IsBenchmarkDisabled(self, state): On 2017/05/10 21:10:57, nednguyen wrote: > Same Done. https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:39: def DisableStory(self, story_name, conditions, reason): On 2017/05/10 21:10:57, nednguyen wrote: > Ditto Done. https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:50: def IsStoryDisabled(self, story, state): On 2017/05/10 21:10:57, nednguyen wrote: > Ditto Done. https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:64: class AllTestCondition(_TestCondition): On 2017/05/10 21:10:57, nednguyen wrote: > let hide all these class with prefix "_" so users will use only the alias. Done.
https://codereview.chromium.org/2843403005/diff/140001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/140001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:52: def IsBenchmarkDisabled(self, state): Actually this isn't quite right. The reason is a benchmark can have multiple shared states. (See https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) I think the best you can do here: if disabled condition is ALL, then nothing matter. else: assert benchmark's allow_mixed_story_states is False.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:12: class Platform(object): On 2017/05/10 21:07:58, nednguyen wrote: > You can use > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > & expand FakePlatform class if needed instead Done. https://codereview.chromium.org/2843403005/diff/140001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/140001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:52: def IsBenchmarkDisabled(self, state): On 2017/05/10 21:38:26, nednguyen wrote: > Actually this isn't quite right. The reason is a benchmark can have multiple > shared states. (See > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) > > I think the best you can do here: > if disabled condition is ALL, then nothing matter. > else: > assert benchmark's allow_mixed_story_states is False. Oh, you actually pointed out a naming problem. It actually isn't using a shared state there, it is using |possible_browser|. They both contain a |object|.platform property, which is probably where the problem stems from. Fixed the naming of this. It will be used here: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/10 19:45:13, rnephew (Reviews Here) wrote: > On 2017/05/02 09:19:52, perezju wrote: > > when logging, I would also like to know the condition why the story got > > disabled, e.g. something like > > > > "load:news:cnn disabled on WIN due to crbug.com/123" > > > > So we probably need to return both the reason and the condition? > > Its returning a tuple of (is_disabled, reason) Yes, but the _condition_ is missing. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/be... File telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/be... telemetry/telemetry/benchmark_unittest.py:167: self.assertTrue( I think there is an .assertIsInstance https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:14: self.DisableStory('story_name2', [expectations.ALL], 'crbug/789') I would prefer if reasons are given as 'crbug.com/789', that way it may be (1) auto-linkified in some contexts or at least (2) you can select, right-click and Get "go to ..." on Chrome. Even if these are just comments, they should nudge developers to the expected usage patterns. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:42: reason: reason for disabling the test. nits: - For consistency with usage bellow: "List of ..", "Reason for .." - "disabling the test" -> "disabling the benchmark". https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:44: assert reason nit: Also provide some message, i.e. 'A reason for disabling must be given.' https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:80: reason: Reason for disabling the test. nit: "the test" -> "the story". https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:82: assert reason ditto https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:92: def IsStoryDisabled(self, story, state): Should this also get a possible_browser instead of state? https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:127: class _MacTestCondition(_TestCondition): Another random idea, most of these could be a single PlatformTestCondition class, which you then instantiate as: ALL_MAC = PlatformTestCondition('mac') ... ALL_ANDROID = PlatformTestCondition('android') ALL_DESKTOP = PlatformTestCondition('mac', 'win', 'linux') ALL_MOBILE = ALL_ANDROID This way you also don't have to make the class names private, since people will use the instances (not the classes) for disabling. Also not sure if we really need the "ALL_" prefix on these? https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:177: ALL_MOBILE = _MobileTestCondition could you write an "ANDROID_SVELTE" condition? (also OK to do on follow up CLs) https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition When would you expect to (temporarily) disable a story only on platforms that do not have a battor installed?
Will address the rest of the comments in the next patch, just wanted to get responses up to some of the comments up during the London overlap. https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:80: self._expectations[str(story)] = [] On 2017/05/10 20:01:38, nednguyen wrote: > On 2017/05/10 19:45:13, rnephew (Reviews Here) wrote: > > 1 is done, is there a way to do 2 as a linter warning? I'm not all that > familiar > > with adding checks to the linter. This seems like it would be a good thing to > > have at that level, but might be hard to actually put there. > > It's not easy to add linter warning, and I doubt that gonna be reliable enough > given that code can come at all sort of different forms. Thats what I feared (it not being easy and/or reliable enough). The story expectation has no direct knowledge of the story set as of now, so it might be quite a bit of work to get it to have that knowledge. Should I do that in this CL, or in a follow up one? https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/11 10:10:41, perezju wrote: > On 2017/05/10 19:45:13, rnephew (Reviews Here) wrote: > > On 2017/05/02 09:19:52, perezju wrote: > > > when logging, I would also like to know the condition why the story got > > > disabled, e.g. something like > > > > > > "load:news:cnn disabled on WIN due to crbug.com/123" > > > > > > So we probably need to return both the reason and the condition? > > > > Its returning a tuple of (is_disabled, reason) > > Yes, but the _condition_ is missing. The reason (be it a bug or a text description) should contain enough information to determine why it was disabled. Where ever is calling this method will have access to the state however, and can determine what platform it is currently on and at least output that along with the reason. If others feel this wont be the case, I can also pass back the condition as a third item in the tuple. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:92: def IsStoryDisabled(self, story, state): On 2017/05/11 10:10:41, perezju wrote: > Should this also get a possible_browser instead of state? No, at this level we have a defined state and no access to the possible_browser object. It will be run somewhere around here: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:127: class _MacTestCondition(_TestCondition): On 2017/05/11 10:10:41, perezju wrote: > Another random idea, most of these could be a single PlatformTestCondition > class, which you then instantiate as: > > ALL_MAC = PlatformTestCondition('mac') > ... > ALL_ANDROID = PlatformTestCondition('android') > ALL_DESKTOP = PlatformTestCondition('mac', 'win', 'linux') > ALL_MOBILE = ALL_ANDROID > > This way you also don't have to make the class names private, since people will > use the instances (not the classes) for disabling. > > Also not sure if we really need the "ALL_" prefix on these? The ALL_ is because we later plan to add more conditions that will check what type of (for example) mac machine we are on. So we can only disable on macbook pro (insert version here) if it is only failing there. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:177: ALL_MOBILE = _MobileTestCondition On 2017/05/11 10:10:41, perezju wrote: > could you write an "ANDROID_SVELTE" condition? (also OK to do on follow up CLs) There are quite a few more conditions I would like to introduce. This first pass was just a minimum number of them to be useful. I was planning on having another CL that just adds more complicated ones. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition On 2017/05/11 10:10:41, perezju wrote: > When would you expect to (temporarily) disable a story only on platforms that do > not have a battor installed? This is more to support disabling of the entire benchmark if the battor is not present. DisableStory and DisableBenchmark use the same conditions, so it would be possible to disable at the story level if there is no battor; but I agree that it doesn't seem likely to actually happen in practice.
https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:127: class _MacTestCondition(_TestCondition): On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > On 2017/05/11 10:10:41, perezju wrote: > > Another random idea, most of these could be a single PlatformTestCondition > > class, which you then instantiate as: > > > > ALL_MAC = PlatformTestCondition('mac') > > ... > > ALL_ANDROID = PlatformTestCondition('android') > > ALL_DESKTOP = PlatformTestCondition('mac', 'win', 'linux') > > ALL_MOBILE = ALL_ANDROID > > > > This way you also don't have to make the class names private, since people > will > > use the instances (not the classes) for disabling. > > > > Also not sure if we really need the "ALL_" prefix on these? > > The ALL_ is because we later plan to add more conditions that will check what > type of (for example) mac machine we are on. So we can only disable on macbook > pro (insert version here) if it is only failing there. It's also consistent with the naming we use in trybot: all all-android all-linux all-mac all-win android-fyi android-nexus5 android-nexus5X android-nexus6 ...
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/11 15:05:58, rnephew (Reviews Here) wrote: > On 2017/05/11 10:10:41, perezju wrote: > > On 2017/05/10 19:45:13, rnephew (Reviews Here) wrote: > > > On 2017/05/02 09:19:52, perezju wrote: > > > > when logging, I would also like to know the condition why the story got > > > > disabled, e.g. something like > > > > > > > > "load:news:cnn disabled on WIN due to crbug.com/123" > > > > > > > > So we probably need to return both the reason and the condition? > > > > > > Its returning a tuple of (is_disabled, reason) > > > > Yes, but the _condition_ is missing. > > The reason (be it a bug or a text description) should contain enough information > to determine why it was disabled. Where ever is calling this method will have > access to the state however, and can determine what platform it is currently on > and at least output that along with the reason. If others feel this wont be the > case, I can also pass back the condition as a third item in the tuple. I've seen enough cases of "@disabled('win', crbug.com/123)" where you click on the bug and it's about some error on Android or something else. The thing is that bugs change faster that code reviews (may be renamed, repurposed), and it's good to be able to spot those issues and fix them. Also, if we later introduce more complex conditions, it would be great to know _which_ of the conditions caused the story/benchmark to be skipped. (May not be easy for the caller to figure out.) https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:92: def IsStoryDisabled(self, story, state): On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > On 2017/05/11 10:10:41, perezju wrote: > > Should this also get a possible_browser instead of state? > > No, at this level we have a defined state and no access to the possible_browser > object. > > It will be run somewhere around here: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Then this is strange because each of Is{Benchmark,Story}Disabled appear to be calling: - condition.ShouldDisable(possible_browser) - condition.ShouldDisable(state) which one should _TestCondition's expect? https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:127: class _MacTestCondition(_TestCondition): On 2017/05/11 15:10:32, nednguyen wrote: > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > On 2017/05/11 10:10:41, perezju wrote: > > > Another random idea, most of these could be a single PlatformTestCondition > > > class, which you then instantiate as: > > > > > > ALL_MAC = PlatformTestCondition('mac') > > > ... > > > ALL_ANDROID = PlatformTestCondition('android') > > > ALL_DESKTOP = PlatformTestCondition('mac', 'win', 'linux') > > > ALL_MOBILE = ALL_ANDROID > > > > > > This way you also don't have to make the class names private, since people > > will > > > use the instances (not the classes) for disabling. > > > > > > Also not sure if we really need the "ALL_" prefix on these? > > > > The ALL_ is because we later plan to add more conditions that will check what > > type of (for example) mac machine we are on. So we can only disable on macbook > > pro (insert version here) if it is only failing there. > > It's also consistent with the naming we use in trybot: > all > all-android > all-linux > all-mac > all-win > android-fyi > android-nexus5 > android-nexus5X > android-nexus6 > ... Acknowledged. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:177: ALL_MOBILE = _MobileTestCondition On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > On 2017/05/11 10:10:41, perezju wrote: > > could you write an "ANDROID_SVELTE" condition? (also OK to do on follow up > CLs) > > There are quite a few more conditions I would like to introduce. This first pass > was just a minimum number of them to be useful. I was planning on having another > CL that just adds more complicated ones. Acknowledged. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > On 2017/05/11 10:10:41, perezju wrote: > > When would you expect to (temporarily) disable a story only on platforms that > do > > not have a battor installed? > > This is more to support disabling of the entire benchmark if the battor is not > present. DisableStory and DisableBenchmark use the same conditions, so it would > be possible to disable at the story level if there is no battor; but I agree > that it doesn't seem likely to actually happen in practice. But that sounds like a "permanent" disable ("i.e. this benchmark does not work on devices without a battor, and never will") as opposed to a "temporary" one ("i.e. this benchmark/story is now failing under some conditions, and we want to take it down there until it's fixed."). I understood from our discussions that "permanent" disables should be done elsewhere?
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for I see your point, and I think there are two possible solutions to this: We could have the TestCondition output a log message about what condition detected the disabling condition. Or if we dont think returning 3 things is too burdensome/confusing we could return which test condition as well. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:92: def IsStoryDisabled(self, story, state): On 2017/05/11 15:26:53, perezju wrote: > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > On 2017/05/11 10:10:41, perezju wrote: > > > Should this also get a possible_browser instead of state? > > > > No, at this level we have a defined state and no access to the > possible_browser > > object. > > > > It will be run somewhere around here: > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > Then this is strange because each of Is{Benchmark,Story}Disabled appear to be > calling: > > - condition.ShouldDisable(possible_browser) > - condition.ShouldDisable(state) > > which one should _TestCondition's expect? I think I am going to rewrite it to take the platform part, which both contain. Thats what it really wants anyway. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition On 2017/05/11 15:26:53, perezju wrote: > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > On 2017/05/11 10:10:41, perezju wrote: > > > When would you expect to (temporarily) disable a story only on platforms > that > > do > > > not have a battor installed? > > > > This is more to support disabling of the entire benchmark if the battor is not > > present. DisableStory and DisableBenchmark use the same conditions, so it > would > > be possible to disable at the story level if there is no battor; but I agree > > that it doesn't seem likely to actually happen in practice. > > But that sounds like a "permanent" disable ("i.e. this benchmark does not work > on devices without a battor, and never will") as opposed to a "temporary" one > ("i.e. this benchmark/story is now failing under some conditions, and we want to > take it down there until it's fixed."). > > I understood from our discussions that "permanent" disables should be done > elsewhere? Well, I did think of one case where we might want to do it. The long running stories will have a trace that is too large to process if they contain battor data. We could want to temporarily disable them until we fix the size issue.
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/11 15:42:49, rnephew (Reviews Here) wrote: > I see your point, and I think there are two possible solutions to this: > > We could have the TestCondition output a log message about what condition > detected the disabling condition. Or if we dont think returning 3 things is too > burdensome/confusing we could return which test condition as well. No strong opinions on the implementation, but I would love if we can log at some point (like I suggested before) something along the lines of: "{story/benchmark} disabled on {condition} due to {reason}" That information could also make it's way to the SkipValue [1] object that should also be constructed at some point. [1]: https://github.com/catapult-project/catapult/blob/23c9f59f4c6bba6a92e5bcae0a1... https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition On 2017/05/11 15:42:49, rnephew (Reviews Here) wrote: > On 2017/05/11 15:26:53, perezju wrote: > > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > > On 2017/05/11 10:10:41, perezju wrote: > > > > When would you expect to (temporarily) disable a story only on platforms > > that > > > do > > > > not have a battor installed? > > > > > > This is more to support disabling of the entire benchmark if the battor is > not > > > present. DisableStory and DisableBenchmark use the same conditions, so it > > would > > > be possible to disable at the story level if there is no battor; but I agree > > > that it doesn't seem likely to actually happen in practice. > > > > But that sounds like a "permanent" disable ("i.e. this benchmark does not work > > on devices without a battor, and never will") as opposed to a "temporary" one > > ("i.e. this benchmark/story is now failing under some conditions, and we want > to > > take it down there until it's fixed."). > > > > I understood from our discussions that "permanent" disables should be done > > elsewhere? > > Well, I did think of one case where we might want to do it. The long running > stories will have a trace that is too large to process if they contain battor > data. We could want to temporarily disable them until we fix the size issue. Sounds good. Maybe add a comment about example usage along those lines in _NoBattOrTestCondition?
https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/80001/telemetry/telemetry/sto... telemetry/telemetry/story/expectations.py:89: return reason_for is not None, reason_for On 2017/05/11 16:14:28, perezju wrote: > On 2017/05/11 15:42:49, rnephew (Reviews Here) wrote: > > I see your point, and I think there are two possible solutions to this: > > > > We could have the TestCondition output a log message about what condition > > detected the disabling condition. Or if we dont think returning 3 things is > too > > burdensome/confusing we could return which test condition as well. > > No strong opinions on the implementation, but I would love if we can log at some > point (like I suggested before) something along the lines of: > > "{story/benchmark} disabled on {condition} due to {reason}" > > That information could also make it's way to the SkipValue [1] object that > should also be constructed at some point. > > [1]: > https://github.com/catapult-project/catapult/blob/23c9f59f4c6bba6a92e5bcae0a1... Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/be... File telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/be... telemetry/telemetry/benchmark_unittest.py:167: self.assertTrue( On 2017/05/11 10:10:41, perezju wrote: > I think there is an .assertIsInstance Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:14: self.DisableStory('story_name2', [expectations.ALL], 'crbug/789') On 2017/05/11 10:10:41, perezju wrote: > I would prefer if reasons are given as 'crbug.com/789', that way it may be (1) > auto-linkified in some contexts or at least (2) you can select, right-click and > Get "go to ..." on Chrome. > > Even if these are just comments, they should nudge developers to the expected > usage patterns. Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:42: reason: reason for disabling the test. On 2017/05/11 10:10:41, perezju wrote: > nits: > - For consistency with usage bellow: "List of ..", "Reason for .." > - "disabling the test" -> "disabling the benchmark". > Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:44: assert reason On 2017/05/11 10:10:41, perezju wrote: > nit: Also provide some message, i.e. 'A reason for disabling must be given.' Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:80: reason: Reason for disabling the test. On 2017/05/11 10:10:41, perezju wrote: > nit: "the test" -> "the story". Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:82: assert reason On 2017/05/11 10:10:41, perezju wrote: > ditto Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:92: def IsStoryDisabled(self, story, state): On 2017/05/11 15:42:49, rnephew (Reviews Here) wrote: > On 2017/05/11 15:26:53, perezju wrote: > > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > > On 2017/05/11 10:10:41, perezju wrote: > > > > Should this also get a possible_browser instead of state? > > > > > > No, at this level we have a defined state and no access to the > > possible_browser > > > object. > > > > > > It will be run somewhere around here: > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > Then this is strange because each of Is{Benchmark,Story}Disabled appear to be > > calling: > > > > - condition.ShouldDisable(possible_browser) > > - condition.ShouldDisable(state) > > > > which one should _TestCondition's expect? > > I think I am going to rewrite it to take the platform part, which both contain. > Thats what it really wants anyway. Done. https://codereview.chromium.org/2843403005/diff/180001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: NO_BATTOR = _NoBattOrTestCondition On 2017/05/11 16:14:28, perezju wrote: > On 2017/05/11 15:42:49, rnephew (Reviews Here) wrote: > > On 2017/05/11 15:26:53, perezju wrote: > > > On 2017/05/11 15:05:59, rnephew (Reviews Here) wrote: > > > > On 2017/05/11 10:10:41, perezju wrote: > > > > > When would you expect to (temporarily) disable a story only on platforms > > > that > > > > do > > > > > not have a battor installed? > > > > > > > > This is more to support disabling of the entire benchmark if the battor is > > not > > > > present. DisableStory and DisableBenchmark use the same conditions, so it > > > would > > > > be possible to disable at the story level if there is no battor; but I > agree > > > > that it doesn't seem likely to actually happen in practice. > > > > > > But that sounds like a "permanent" disable ("i.e. this benchmark does not > work > > > on devices without a battor, and never will") as opposed to a "temporary" > one > > > ("i.e. this benchmark/story is now failing under some conditions, and we > want > > to > > > take it down there until it's fixed."). > > > > > > I understood from our discussions that "permanent" disables should be done > > > elsewhere? > > > > Well, I did think of one case where we might want to do it. The long running > > stories will have a trace that is too large to process if they contain battor > > data. We could want to temporarily disable them until we fix the size issue. > > Sounds good. Maybe add a comment about example usage along those lines in > _NoBattOrTestCondition? Done.
lgtm, w/ a few final comments. https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:46: self.assertIsInstance(True, bool) left over from debugging? https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:73: return False, None Idea: since the reason is always required, maybe just return either "reason" or "None"? The caller can also that value as a "bool" to figure out whether the thing is disabled. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:106: platform: A platform property. property -> object https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:115: logging.critical('%s is disabled on %s due to %s.', I don't think this should be critical. Maybe .info? https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:203: return 'No BattOr' nit: 'Platforms with no BattOr' https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:213: NO_BATTOR = _NoBattOrTestCondition Maybe make these instances rather than classes? It looks a bit weird that we need to create the object just to get the string in "str(condition())". https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:53: is_disabled, reason = e.IsBenchmarkDisabled(self._state.platform) I think we don't need a mock state any more, right? These tests only need to know about the platform.
I also redid how the TestConditions are defined (when you look at it it should be obvious what I did). https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:46: self.assertIsInstance(True, bool) On 2017/05/12 08:15:05, perezju wrote: > left over from debugging? Ah, yeah. I couldn't remember the order to put the object, class in so I did that really quick to test. I thought putting it in a completely unrelated test would make me not forget to take it out. I thought wrong. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:73: return False, None On 2017/05/12 08:15:05, perezju wrote: > Idea: since the reason is always required, maybe just return either "reason" or > "None"? The caller can also that value as a "bool" to figure out whether the > thing is disabled. Done. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:106: platform: A platform property. On 2017/05/12 08:15:05, perezju wrote: > property -> object Done. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:115: logging.critical('%s is disabled on %s due to %s.', On 2017/05/12 08:15:05, perezju wrote: > I don't think this should be critical. Maybe .info? Done. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:203: return 'No BattOr' On 2017/05/12 08:15:05, perezju wrote: > nit: 'Platforms with no BattOr' Done. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:213: NO_BATTOR = _NoBattOrTestCondition On 2017/05/12 08:15:05, perezju wrote: > Maybe make these instances rather than classes? > > It looks a bit weird that we need to create the object just to get the string in > "str(condition())". Done. https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:53: is_disabled, reason = e.IsBenchmarkDisabled(self._state.platform) On 2017/05/12 08:15:05, perezju wrote: > I think we don't need a mock state any more, right? These tests only need to > know about the platform. Done.
On 2017/05/12 17:49:18, rnephew (Reviews Here) wrote: > I also redid how the TestConditions are defined (when you look at it it should > be obvious what I did). > > https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... > File common/battor/battor/battor_wrapper_unittest.py (right): > > https://codereview.chromium.org/2843403005/diff/200001/common/battor/battor/b... > common/battor/battor/battor_wrapper_unittest.py:46: self.assertIsInstance(True, > bool) > On 2017/05/12 08:15:05, perezju wrote: > > left over from debugging? > > Ah, yeah. I couldn't remember the order to put the object, class in so I did > that really quick to test. I thought putting it in a completely unrelated test > would make me not forget to take it out. I thought wrong. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > File telemetry/telemetry/story/expectations.py (right): > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:73: return False, None > On 2017/05/12 08:15:05, perezju wrote: > > Idea: since the reason is always required, maybe just return either "reason" > or > > "None"? The caller can also that value as a "bool" to figure out whether the > > thing is disabled. > > Done. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:106: platform: A platform property. > On 2017/05/12 08:15:05, perezju wrote: > > property -> object > > Done. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:115: logging.critical('%s is disabled > on %s due to %s.', > On 2017/05/12 08:15:05, perezju wrote: > > I don't think this should be critical. Maybe .info? > > Done. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:203: return 'No BattOr' > On 2017/05/12 08:15:05, perezju wrote: > > nit: 'Platforms with no BattOr' > > Done. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:213: NO_BATTOR = > _NoBattOrTestCondition > On 2017/05/12 08:15:05, perezju wrote: > > Maybe make these instances rather than classes? > > > > It looks a bit weird that we need to create the object just to get the string > in > > "str(condition())". > > Done. > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > File telemetry/telemetry/story/expectations_unittest.py (right): > > https://codereview.chromium.org/2843403005/diff/200001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations_unittest.py:53: is_disabled, reason = > e.IsBenchmarkDisabled(self._state.platform) > On 2017/05/12 08:15:05, perezju wrote: > > I think we don't need a mock state any more, right? These tests only need to > > know about the platform. > > Done. Please consider update the CL title to "Add Expectation module that enable disabling benchmarks' failing stories" or "Add Benchmark Expectation which allows disabling failing tests at story level". In general, I think "Add feature X" is preferable to "Have feature X"
Description was changed from ========== [Telemetry] Have benchmarks have story expectations. This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ========== to ========== [Telemetry] Add Expectation module that enable disabling benchmarks/stories This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ==========
> Please consider update the CL title to "Add Expectation module that enable > disabling benchmarks' failing stories" or "Add Benchmark Expectation which > allows disabling failing tests at story level". In general, I think "Add feature > X" is preferable to "Have feature X" Updated title.
Description was changed from ========== [Telemetry] Add Expectation module that enable disabling benchmarks/stories This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ========== to ========== [Telemetry] Add Expectation module that enables disabling benchmarks/stories This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ==========
https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/be... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/be... telemetry/telemetry/benchmark.py:270: """ Returns a StoryExpectation object. nit: (here and elsewhere) delete space between quotes and start of comment https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:8: class StoryExpectations(object): nit: I think this should probably have a top-level comment on what StoryExpectations is. An example is great, but it'd be nice to have a tiny bit of what I'm looking for when reading the example https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:54: def IsBenchmarkDisabled(self, platform): see comments below about IsStoryDisabled https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:79: DisableBenchmark([expectations.ALL_WIN], 'crbug.com/123') Should this instead read DisableStory? Otherwise, it doesn't really seem like this is an example usage https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:87: assert len(story_name) < 30, 'Story name too long.' I think that it might make sense to give a longer description here, possibly including some information about what to do if your story name *is* too long. I think this is an error that a lot of people are likely to hit and, without prior knowledge, it's hard to understand why Telemetry gives a hoot about how long your story name is. Maybe something "Story name exceeds limit of 30 characters. This limit is in place to encourage Telemetry benchmark owners to use short, simple story names (e.g. 'google_search_images', not 'http://www.google.com/images/1234/abc')." Without this, I worry that people are just going to be POed at Telemetry. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:99: Example: This example feels pretty artificial. Isn't the pydoc before this example clear enough? https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:100: for story in stories: nit: shouldn't this be if not IsStoryDisabled(story, platform): story.Run() https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:152: return (current_platform == 'mac' or current_platform == 'win' or could you do: current_platform in ['mac, 'win', 'linux'] ? https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:168: # Example use case for NO_BATTOR test condition: nit: not sure this comment is necessary. IMO if people want examples of why a NoBattOr test condition might be useful, they should just do a code search for it for an up-to-date example. If no such example exists, then we should delete the condition anyhow https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: ALL_MAC = _TestConditionByPlatform('mac', 'Mac Platforms') What's the reason for prefixing these with "ALL"? It doesn't seem to add much https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:31: self.assertTrue(e._frozen) Don't use unit tests to test the internal state of your class! go/unit-test#public-apis If there's no way to test this without accessing internals, then it's not a part of your public API and shouldn't be tested anyway. If there is a way to test it, then you should do that. Another way to think of it: rather than checking that e._frozen is true, e._expectations is {}, and that e._disabled_platforms is false, what are the _ramifications_ of those things being true? It sounds like you probably expect a given story or benchmark to pass (I think, due to e._expectations being empty?). You would also expect the test to be enabled on all platforms (e._disabled_platforms is false). Also, e._frozen is true, so presumably trying to change the StoryExpectations will cause some failure. Those are your things that you should be testing. Whenever you go to access the internal member that's responsible for a behavior, try to reframe the test in a way to instead test the behavior that the internal member is responsible for. This will almost always lead to more resilient tests. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:38: e.DisableBenchmark(['test'], 'test') nit: these seem like two different behaviors (one benchmark-related, one story-related) https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:45: self.DisableBenchmark( What specific behavior is this trying to test? A red flag to me about this test is that it's what I call a "wandering" unit test, that looks like: // Set up // Assert // Change some things // Assert something else // Etc. Specifically, the red flag to me is changing something _after_ doing the initial assertion. If your test is focused on testing a single behavior, this is almost never the case. Is the purpose of this test to test that a single ALL_WIN expectation works as intended? That ALL_MAC works as intended? That multiple expectations together function together in the expected way? All of those seem like valid things that you might want to test, but it's not clear which of those is being tested here. I think this test should probably be broken up into: testDisabledBenchmarkWin testDisabledBenchmarkMac testDisabledBenchmarkLinux testDisabledBenchmarkAndroid As for multiple expectations together functioning the correct way, I think it's important to think of the corner cases of how that _could_ go wrong. It looks like ShouldDisable functions and does a logic an OR between all of the different expectations, and returns true if any of those expectations returns true. Given that, it seems like there are at least three behaviors that are worth testing, each in different stories - What happens if you have a single expectation that returns true? - What happens if you have a single expectation that returns false? - What happens if you have multiple expectations, one of them returns true, and one of them returns false? Sorry if it seems like I'm being a jerk about unit tests: my old team ended up facing huge maintenance problems due to too many tests that were wandering, and the tests made it _really_ hard to tell what the important parts of the test were, and so everyone was afraid to touch those tests for fear that we were losing coverage. Also, could you look at other tests in in this file to check whether they meet the same criteria? https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:48: 'crbug/123') IMO, if we're going for realism, we might as well do "crbug.com/123" instead of "crbug/123" https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:222: # Test disabling on one platform. nit: don't think this comment is necessary (the info is present in the test name, which is better anyhow) https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... telemetry/telemetry/testing/fakes/__init__.py:84: def SetBattOrDetected(self, b): I'm not a fan of the lack of parallelism between the "Get" and the "Set" here. Maybe "IsBattOrConnected" and "SetBattOrConnected" would be better? That seems to more closely mirror https://cs.chromium.org/search/?q=isbattorconnected&sq=package:chromium&type=cs, anyhow, and consistency is always good.
https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/be... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/be... telemetry/telemetry/benchmark.py:270: """ Returns a StoryExpectation object. On 2017/05/12 21:30:31, charliea wrote: > nit: (here and elsewhere) delete space between quotes and start of comment Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:8: class StoryExpectations(object): On 2017/05/12 21:30:31, charliea wrote: > nit: I think this should probably have a top-level comment on what > StoryExpectations is. An example is great, but it'd be nice to have a tiny bit > of what I'm looking for when reading the example Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:54: def IsBenchmarkDisabled(self, platform): On 2017/05/12 21:30:31, charliea wrote: > see comments below about IsStoryDisabled Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:79: DisableBenchmark([expectations.ALL_WIN], 'crbug.com/123') On 2017/05/12 21:30:32, charliea wrote: > Should this instead read DisableStory? Otherwise, it doesn't really seem like > this is an example usage Yeah, c/p error. Well. c/p properly, forgot to change it to reflect its new location. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:87: assert len(story_name) < 30, 'Story name too long.' On 2017/05/12 21:30:31, charliea wrote: > I think that it might make sense to give a longer description here, possibly > including some information about what to do if your story name *is* too long. I > think this is an error that a lot of people are likely to hit and, without prior > knowledge, it's hard to understand why Telemetry gives a hoot about how long > your story name is. Maybe something "Story name exceeds limit of 30 characters. > This limit is in place to encourage Telemetry benchmark owners to use short, > simple story names (e.g. 'google_search_images', not > 'http://www.google.com/images/1234/abc')." Without this, I worry that people are > just going to be POed at Telemetry. Done, and also added a unittest for story length that I did not add when I added length checking. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:99: Example: On 2017/05/12 21:30:31, charliea wrote: > This example feels pretty artificial. Isn't the pydoc before this example clear > enough? Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:100: for story in stories: On 2017/05/12 21:30:32, charliea wrote: > nit: shouldn't this be > > if not IsStoryDisabled(story, platform): > story.Run() Acknowledged. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:152: return (current_platform == 'mac' or current_platform == 'win' or On 2017/05/12 21:30:31, charliea wrote: > could you do: > > current_platform in ['mac, 'win', 'linux'] > > ? Done, and that also let me simplify the TestConditions even further. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:168: # Example use case for NO_BATTOR test condition: One reviewer says add comment, one says do not add comment.. I'm going to let you guys duke this out since I have no strong feelings about it one way or the other. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: ALL_MAC = _TestConditionByPlatform('mac', 'Mac Platforms') On 2017/05/12 21:30:31, charliea wrote: > What's the reason for prefixing these with "ALL"? It doesn't seem to add much To mirror how we do it for tryjobs. We will also be adding more conditions that have more narrow scope (svelte_android being an example that comes to mind). There is a comment thread a few patches back on it. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:31: self.assertTrue(e._frozen) On 2017/05/12 21:30:32, charliea wrote: > Don't use unit tests to test the internal state of your class! > > go/unit-test#public-apis > > If there's no way to test this without accessing internals, then it's not a part > of your public API and shouldn't be tested anyway. If there is a way to test it, > then you should do that. > > Another way to think of it: rather than checking that e._frozen is true, > e._expectations is {}, and that e._disabled_platforms is false, what are the > _ramifications_ of those things being true? > > It sounds like you probably expect a given story or benchmark to pass (I think, > due to e._expectations being empty?). You would also expect the test to be > enabled on all platforms (e._disabled_platforms is false). Also, e._frozen is > true, so presumably trying to change the StoryExpectations will cause some > failure. Those are your things that you should be testing. Whenever you go to > access the internal member that's responsible for a behavior, try to reframe the > test in a way to instead test the behavior that the internal member is > responsible for. This will almost always lead to more resilient tests. I just think this test isn't very useful then. Just getting rid of it because the expected behavior after being frozen is tested elsewhere. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:38: e.DisableBenchmark(['test'], 'test') On 2017/05/12 21:30:32, charliea wrote: > nit: these seem like two different behaviors (one benchmark-related, one > story-related) Its actually just testing that after the Expectation set is frozen, you cannot add anymore expectations. There are two ways to add expectations, and it tests both. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:45: self.DisableBenchmark( I think the big problem here is when I changed the testcondition to its current form, I didn't revamp the unittests to reflect that fact; not that the unit tests are wandering. Its harder to test something thats several levels away, thus the tests are more complicated than they need to. Once I fixed that, it naturally pruned itself down to your 3 cases. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:48: 'crbug/123') On 2017/05/12 21:30:32, charliea wrote: > IMO, if we're going for realism, we might as well do "crbug.com/123" instead of > "crbug/123" > > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:222: # Test disabling on one platform. On 2017/05/12 21:30:32, charliea wrote: > nit: don't think this comment is necessary (the info is present in the test > name, which is better anyhow) Done. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... telemetry/telemetry/testing/fakes/__init__.py:84: def SetBattOrDetected(self, b): On 2017/05/12 21:30:32, charliea wrote: > I'm not a fan of the lack of parallelism between the "Get" and the "Set" here. > Maybe "IsBattOrConnected" and "SetBattOrConnected" would be better? > > That seems to more closely mirror > https://cs.chromium.org/search/?q=isbattorconnected&sq=package:chromium&type=cs, > anyhow, and consistency is always good. I agree in principal, but this will have to wait for another CL, since it is already named in platform_backend as HasBattOrConnected: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:31: self.assertTrue(e._frozen) On 2017/05/12 22:55:30, rnephew (Reviews Here) wrote: > On 2017/05/12 21:30:32, charliea wrote: > > Don't use unit tests to test the internal state of your class! > > > > go/unit-test#public-apis > > > > If there's no way to test this without accessing internals, then it's not a > part > > of your public API and shouldn't be tested anyway. If there is a way to test > it, > > then you should do that. > > > > Another way to think of it: rather than checking that e._frozen is true, > > e._expectations is {}, and that e._disabled_platforms is false, what are the > > _ramifications_ of those things being true? > > > > It sounds like you probably expect a given story or benchmark to pass (I > think, > > due to e._expectations being empty?). You would also expect the test to be > > enabled on all platforms (e._disabled_platforms is false). Also, e._frozen is > > true, so presumably trying to change the StoryExpectations will cause some > > failure. Those are your things that you should be testing. Whenever you go to > > access the internal member that's responsible for a behavior, try to reframe > the > > test in a way to instead test the behavior that the internal member is > > responsible for. This will almost always lead to more resilient tests. > > I just think this test isn't very useful then. Just getting rid of it because > the expected behavior after being frozen is tested elsewhere. +1 good catch Charlie https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:85: "Story name exceeds limit of 30 characters. This limit is in place to " Not all system health stories pass this test (e.g. "long_running:tools:gmail-background"), bump it to 40 and we should be fine. (Or should we enforce the limit and rename those stories to have shorter names?)
On 2017/05/15 10:38:21, perezju wrote: > https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... > File telemetry/telemetry/story/expectations_unittest.py (right): > > https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations_unittest.py:31: > self.assertTrue(e._frozen) > On 2017/05/12 22:55:30, rnephew (Reviews Here) wrote: > > On 2017/05/12 21:30:32, charliea wrote: > > > Don't use unit tests to test the internal state of your class! > > > > > > go/unit-test#public-apis > > > > > > If there's no way to test this without accessing internals, then it's not a > > part > > > of your public API and shouldn't be tested anyway. If there is a way to test > > it, > > > then you should do that. > > > > > > Another way to think of it: rather than checking that e._frozen is true, > > > e._expectations is {}, and that e._disabled_platforms is false, what are the > > > _ramifications_ of those things being true? > > > > > > It sounds like you probably expect a given story or benchmark to pass (I > > think, > > > due to e._expectations being empty?). You would also expect the test to be > > > enabled on all platforms (e._disabled_platforms is false). Also, e._frozen > is > > > true, so presumably trying to change the StoryExpectations will cause some > > > failure. Those are your things that you should be testing. Whenever you go > to > > > access the internal member that's responsible for a behavior, try to reframe > > the > > > test in a way to instead test the behavior that the internal member is > > > responsible for. This will almost always lead to more resilient tests. > > > > I just think this test isn't very useful then. Just getting rid of it because > > the expected behavior after being frozen is tested elsewhere. > > +1 good catch Charlie > > https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... > File telemetry/telemetry/story/expectations.py (right): > > https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... > telemetry/telemetry/story/expectations.py:85: "Story name exceeds limit of 30 > characters. This limit is in place to " > Not all system health stories pass this test (e.g. > "long_running:tools:gmail-background"), bump it to 40 and we should be fine. > > (Or should we enforce the limit and rename those stories to have shorter names?) It just needs to be short enough so people can easily copy & paste and not have to do "#pylint: disable long line..", IMO. I vote for bumping it to 40, or maybe 50.
https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/240001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:85: "Story name exceeds limit of 30 characters. This limit is in place to " On 2017/05/15 10:38:21, perezju wrote: > Not all system health stories pass this test (e.g. > "long_running:tools:gmail-background"), bump it to 40 and we should be fine. > > (Or should we enforce the limit and rename those stories to have shorter names?) Set to 50.
https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:168: # Example use case for NO_BATTOR test condition: On 2017/05/12 22:55:30, rnephew (Reviews Here) wrote: > One reviewer says add comment, one says do not add comment.. I'm going to let > you guys duke this out since I have no strong feelings about it one way or the > other. Sorry about that - this kind of churn sucks, and you did the right thin by just telling me to go talk to the other reviewer directly. If the trace size issue is the only reason we have for the NoBattOrTestCondition, I think we can just go ahead and remove it. Ehsan recently completed the work to remove the Javascript trace size limitations and I'm going to give it a try today. If it turns out that we still have problems with this, we can always add it back in in a later CL. https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:178: ALL_MAC = _TestConditionByPlatform('mac', 'Mac Platforms') On 2017/05/12 22:55:30, rnephew (Reviews Here) wrote: > On 2017/05/12 21:30:31, charliea wrote: > > What's the reason for prefixing these with "ALL"? It doesn't seem to add much > > To mirror how we do it for tryjobs. We will also be adding more conditions that > have more narrow scope (svelte_android being an example that comes to mind). > There is a comment thread a few patches back on it. SGTM As long as there's solid rationale for it, I don't care too much. Would you mind adding a quick comment that they're prefixed with ALL_ to mirror naming for tryjobs? https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2843403005/diff/220001/telemetry/telemetry/te... telemetry/telemetry/testing/fakes/__init__.py:84: def SetBattOrDetected(self, b): On 2017/05/12 22:55:30, rnephew (Reviews Here) wrote: > On 2017/05/12 21:30:32, charliea wrote: > > I'm not a fan of the lack of parallelism between the "Get" and the "Set" here. > > Maybe "IsBattOrConnected" and "SetBattOrConnected" would be better? > > > > That seems to more closely mirror > > > https://cs.chromium.org/search/?q=isbattorconnected&sq=package:chromium&type=cs, > > anyhow, and consistency is always good. > > I agree in principal, but this will have to wait for another CL, since it is > already named in platform_backend as HasBattOrConnected: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... SGTM https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... File telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... telemetry/telemetry/benchmark_unittest.py:168: expectations, story_module.expectations.StoryExpectations) possible suggestion: just inline b.GetExpectations() here because it's only used once https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. s/pased/passed https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. supernit suggestion: use "given" instead of "passed to method" It's a little bit less awkward, especially when you have lots of arguments. This would make it just "Disables the benchmark under the given conditions." https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:56: """Returns if the benchmark has been disabled for the platform. I think that you can collapse this whole thing down and get rid of the "Returns" clause at the bottom: """Returns the reason the benchmark was disabled, or None if the benchmark is enabled. Args: platform: A platform object """ https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:73: """Disable story under given conditions. supernit: s/Disables story under given conditions./Disables the story under the given conditions. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:100: """Returns if the story has been disabled for the current platform. See notes for IsBenchmarkDisabled https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:109: supernit: delete empty first line https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:30: def testAllWin(self): nit: I think you should actually split most of these tests in two: for example, testAllWinDisabledReturnsReason and testAllWinEnabledReturnsNone https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: self.assertTrue(expectations.ALL_WIN.ShouldDisable(p)) Rather than asserting true, I think you should assert that the returned reason is equal. Regular callers can treat the value as truthy/falsy all they want, but in the unit test, it's our job to be as precise as possible when verifying the function's inputs and outputs https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:144: self.assertTrue(reason) (here and elsewhere in this file where you assert twice on the same value) is self.assertTrue necessary? It seems like it's strictly less descriptive than the following assertEqual https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:148: MockStory('disbled'), self.platform) nit: disabled?
https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... File telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... telemetry/telemetry/benchmark_unittest.py:168: expectations, story_module.expectations.StoryExpectations) On 2017/05/15 21:15:41, charliea wrote: > possible suggestion: just inline b.GetExpectations() here because it's only used > once Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. On 2017/05/15 21:15:41, charliea wrote: > s/pased/passed Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. On 2017/05/15 21:15:42, charliea wrote: > supernit suggestion: use "given" instead of "passed to method" > > It's a little bit less awkward, especially when you have lots of arguments. This > would make it just "Disables the benchmark under the given conditions." Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:56: """Returns if the benchmark has been disabled for the platform. On 2017/05/15 21:15:41, charliea wrote: > I think that you can collapse this whole thing down and get rid of the "Returns" > clause at the bottom: > > """Returns the reason the benchmark was disabled, or None if the benchmark is > enabled. > > Args: > platform: A platform object > """ Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:73: """Disable story under given conditions. On 2017/05/15 21:15:41, charliea wrote: > supernit: s/Disables story under given conditions./Disables the story under the > given conditions. Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:100: """Returns if the story has been disabled for the current platform. On 2017/05/15 21:15:42, charliea wrote: > See notes for IsBenchmarkDisabled Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:109: On 2017/05/15 21:15:42, charliea wrote: > supernit: delete empty first line Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:30: def testAllWin(self): On 2017/05/15 21:15:42, charliea wrote: > nit: I think you should actually split most of these tests in two: for example, > testAllWinDisabledReturnsReason and testAllWinEnabledReturnsNone Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: self.assertTrue(expectations.ALL_WIN.ShouldDisable(p)) On 2017/05/15 21:15:42, charliea wrote: > Rather than asserting true, I think you should assert that the returned reason > is equal. Regular callers can treat the value as truthy/falsy all they want, but > in the unit test, it's our job to be as precise as possible when verifying the > function's inputs and outputs Thats part of the StoryExpectation, not part of the TestCondition. The given test condition just returns True if htat condition is true (ALL_MAC returns true if its a mac). The 'reason' is stored in the SToryExpectations part and is tested bellow. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:144: self.assertTrue(reason) On 2017/05/15 21:15:42, charliea wrote: > (here and elsewhere in this file where you assert twice on the same value) is > self.assertTrue necessary? It seems like it's strictly less descriptive than the > following assertEqual Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:148: MockStory('disbled'), self.platform) On 2017/05/15 21:15:42, charliea wrote: > nit: disabled? Done.
https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... File telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/be... telemetry/telemetry/benchmark_unittest.py:168: expectations, story_module.expectations.StoryExpectations) On 2017/05/15 21:15:41, charliea wrote: > possible suggestion: just inline b.GetExpectations() here because it's only used > once Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. On 2017/05/15 21:15:41, charliea wrote: > s/pased/passed Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under conditions pased to method. On 2017/05/15 21:15:42, charliea wrote: > supernit suggestion: use "given" instead of "passed to method" > > It's a little bit less awkward, especially when you have lots of arguments. This > would make it just "Disables the benchmark under the given conditions." Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:56: """Returns if the benchmark has been disabled for the platform. On 2017/05/15 21:15:41, charliea wrote: > I think that you can collapse this whole thing down and get rid of the "Returns" > clause at the bottom: > > """Returns the reason the benchmark was disabled, or None if the benchmark is > enabled. > > Args: > platform: A platform object > """ Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:73: """Disable story under given conditions. On 2017/05/15 21:15:41, charliea wrote: > supernit: s/Disables story under given conditions./Disables the story under the > given conditions. Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:100: """Returns if the story has been disabled for the current platform. On 2017/05/15 21:15:42, charliea wrote: > See notes for IsBenchmarkDisabled Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:109: On 2017/05/15 21:15:42, charliea wrote: > supernit: delete empty first line Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:30: def testAllWin(self): On 2017/05/15 21:15:42, charliea wrote: > nit: I think you should actually split most of these tests in two: for example, > testAllWinDisabledReturnsReason and testAllWinEnabledReturnsNone Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: self.assertTrue(expectations.ALL_WIN.ShouldDisable(p)) On 2017/05/15 21:15:42, charliea wrote: > Rather than asserting true, I think you should assert that the returned reason > is equal. Regular callers can treat the value as truthy/falsy all they want, but > in the unit test, it's our job to be as precise as possible when verifying the > function's inputs and outputs Thats part of the StoryExpectation, not part of the TestCondition. The given test condition just returns True if htat condition is true (ALL_MAC returns true if its a mac). The 'reason' is stored in the SToryExpectations part and is tested bellow. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:144: self.assertTrue(reason) On 2017/05/15 21:15:42, charliea wrote: > (here and elsewhere in this file where you assert twice on the same value) is > self.assertTrue necessary? It seems like it's strictly less descriptive than the > following assertEqual Done. https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:148: MockStory('disbled'), self.platform) On 2017/05/15 21:15:42, charliea wrote: > nit: disabled? Done.
Patchset #12 (id:280001) has been deleted
lgtm after fixing the story names. Thanks Randy! https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/260001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: self.assertTrue(expectations.ALL_WIN.ShouldDisable(p)) On 2017/05/15 21:36:07, rnephew (Reviews Here) wrote: > On 2017/05/15 21:15:42, charliea wrote: > > Rather than asserting true, I think you should assert that the returned reason > > is equal. Regular callers can treat the value as truthy/falsy all they want, > but > > in the unit test, it's our job to be as precise as possible when verifying the > > function's inputs and outputs > > Thats part of the StoryExpectation, not part of the TestCondition. The given > test condition just returns True if htat condition is true (ALL_MAC returns true > if its a mac). The 'reason' is stored in the SToryExpectations part and is > tested bellow. Ah, okay, sorry about that. Thanks! https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under given conditions. supernit: s/given/the given https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:30: def testAll(self): maybe testAllAlwaysReturnsTrue? https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: def testAllWinReturnsReason(self): sorry, my fault: if these are generally returning true/false rather than reasons like you said, their names should reflect that (e.g. testAllWinReturnsTrueOnWin, testAllWinReturnsFalseOnOthers, etc.)
https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations.py (right): https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations.py:37: """Disable benchmark under given conditions. On 2017/05/16 16:09:27, charliea wrote: > supernit: s/given/the given Done. https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/expectations_unittest.py (right): https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:30: def testAll(self): On 2017/05/16 16:09:27, charliea wrote: > maybe testAllAlwaysReturnsTrue? Done. https://codereview.chromium.org/2843403005/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/expectations_unittest.py:33: def testAllWinReturnsReason(self): On 2017/05/16 16:09:27, charliea wrote: > sorry, my fault: if these are generally returning true/false rather than reasons > like you said, their names should reflect that (e.g. testAllWinReturnsTrueOnWin, > testAllWinReturnsFalseOnOthers, etc.) 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, perezju@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2843403005/#ps320001 (title: "[Telemetry] Have benchmarks have 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": 320001, "attempt_start_ts": 1494952730654810,
"parent_rev": "b03a65db0c7a672f18a11e5a255a3da0e0207eb0", "commit_rev":
"3dc93e28655fd0318f9e35d40e66edadb17adb01"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Add Expectation module that enables disabling benchmarks/stories This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 ========== to ========== [Telemetry] Add Expectation module that enables disabling benchmarks/stories This work is for better disabling of telemetry tests. This first CL lays the ground works by creating the story_expectation object and by extending the benchmark class api to include a way to get story expectations. This implements the default behavior of creating an empty expectation set if none is specified. go/StoryExpectationsDD BUG=chromium:711065 Review-Url: https://codereview.chromium.org/2843403005 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
