[Telemetry] Add Expectation module that enables disabling benchmarks/stories

telemetry/telemetry/story/expectations.py

Index: telemetry/telemetry/story/expectations.py
diff --git a/telemetry/telemetry/story/expectations.py b/telemetry/telemetry/story/expectations.py
new file mode 100644
index 0000000000000000000000000000000000000000..e1923e2ca2f2c338413011d24f300898dae105ff
--- /dev/null
+++ b/telemetry/telemetry/story/expectations.py
@@ -0,0 +1,89 @@
+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+
+class TestConditions(object):

[perezju, 2017/05/02 09:19:52]

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.

[rnephew (Reviews Here), 2017/05/10 19:45:13]

I really like this idea.
Done.

+  MAC = 'mac'
+  WIN = 'win'
+  LINUX = 'linux'
+  ANDROID = 'android'
+  MOBILE = 'mobile'
+  DESKTOP = 'desktop'
+  ALL = 'all'
+  NO_BATTOR = 'no_battor'
+
+  _DESKTOP_PLATFORMS = ['mac', 'win', 'linux']
+  _MOBILE_PLATFORMS = ['android']
+
+  @classmethod
+  def IsDesktop(cls, platform):
+    return platform in cls._DESKTOP_PLATFORMS
+
+  @classmethod
+  def IsMobile(cls, platform):
+    return platform in cls._MOBILE_PLATFORMS
+
+
+class StoryExpectations(object):
+  def __init__(self):
+    self._expectations = {'disabled_platforms': []}

[perezju, 2017/05/02 09:19:52]

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.

[rnephew (Reviews Here), 2017/05/10 19:45:13]

Done.

+    self._frozen = False
+    self.SetExpectations()
+    self._Freeze()
+
+  def SetExpectations(self):
+    """ Sets the Expectations for test disabling
+
+    Override in subclasses to disable tests."""
+    pass
+
+  def _Freeze(self):
+    self._frozen = True
+
+  def DisableBenchmark(self, platforms, reason):
+    assert not self._frozen, ('Cannot disable benchmark on a frozen '
+                              'StoryExpectation object.')
+    self._expectations['disabled_platforms'].append((platforms, reason))
+
+  @staticmethod
+  def _CommonPlatformChecks(platforms, state):
+    current_platform = state.platform.GetOSName()
+    if TestConditions.ALL in platforms:
+      return True
+    if (TestConditions.NO_BATTOR in platforms and
+        not state.platform.HasBattOrConnected()):
+      return True
+    if current_platform in platforms:
+      return True
+    if (TestConditions.DESKTOP in platforms and
+        TestConditions.IsDesktop(current_platform)):
+      return True
+    if (TestConditions.MOBILE in platforms and
+        TestConditions.IsMobile(current_platform)):
+      return True
+
+    return False
+
+  def IsBenchmarkDisabled(self, state):
+    reason_for = None
+    for platforms, reason in self._expectations.get('disabled_platforms'):
+      if self._CommonPlatformChecks(platforms, state):
+        reason_for = reason
+        break
+    return reason_for is not None, reason_for
+
+  def DisableStory(self, story, platforms, reason=''):

[nednguyen, 2017/05/02 00:04:01]

s/platforms/conditions/g
we should also assert all the conditions are recognized ones  

[perezju, 2017/05/02 09:19:52]

Let's not provide a default for "reason", it should always be given.

[nednguyen, 2017/05/09 16:44:24]

In addition, I think we should even assert that reason must be non-empty.

[rnephew (Reviews Here), 2017/05/10 19:45:13]

Done.

[rnephew (Reviews Here), 2017/05/10 19:45:13]

Done.

+    assert not self._frozen, ('Cannot disable stories on a frozen '
+                              'StoryExpectation object.')
+    if not self._expectations.get(str(story)):
+      self._expectations[str(story)] = []

[nednguyen, 2017/05/02 00:04:02]

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.

[perezju, 2017/05/02 09:19:52]

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.

[nednguyen, 2017/05/02 16:44:42]

+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, 2017/05/09 16:44:24]

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.

[rnephew (Reviews Here), 2017/05/10 19:45:13]

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.

[nednguyen, 2017/05/10 20:01:38]

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.

[rnephew (Reviews Here), 2017/05/11 15:05:58]

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?

+    self._expectations[str(story)].append((platforms, reason))
+
+  def IsStoryDisabled(self, story, state):
+    reason_for = None
+    for platforms, reason in self._expectations.get(str(story), []):
+      if self._CommonPlatformChecks(platforms, state):
+        reason_for = reason
+        break
+    return reason_for is not None, reason_for

[perezju, 2017/05/02 09:19:52]

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?

[rnephew (Reviews Here), 2017/05/10 19:45:13]

Its returning a tuple of (is_disabled, reason)

[perezju, 2017/05/11 10:10:41]

Yes, but the _condition_ is missing.

[rnephew (Reviews Here), 2017/05/11 15:05:58]

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.

[perezju, 2017/05/11 15:26:53]

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.)

[rnephew (Reviews Here), 2017/05/11 15:42:49]

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.

[perezju, 2017/05/11 16:14:28]

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...

[rnephew (Reviews Here), 2017/05/11 17:14:11]

Done.