Support proper retrying of telemetry_unittests in recipes

This patch ports the retry logic we had for mojo_python_tests over to telemetry_unittests and telemetry_perf_unittests, since they are using
the common syntax for retries.

(I actually refactored the code to be shared in a new PythonBasedTest step class).

Note that we still don't upload the results to the flakiness dashboard yet, though. Adding that should will be straightforward.

R=phajdan.jr@chromium.org
BUG=359521
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=285595

[Paweł Hajdan Jr., 2014-07-24 15:00:34]

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium/steps.py:450: assert False, "This method
should not be called."
Can we remove path_to_harness then?

I prefer a slight duplication of the api.python call to an "irregular" API
surface.

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium/steps.py:547: # TODO: This should be
converted to a PythonBasedTest, although it
nit: TODO(dpranke)?

[Dirk Pranke, 2014-07-24 16:02:45]

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium/steps.py:450: assert False, "This method
should not be called."
On 2014/07/24 15:00:34, Paweł Hajdan Jr. wrote:
> Can we remove path_to_harness then?
> 
> I prefer a slight duplication of the api.python call to an "irregular" API
> surface.

Yeah, that seems like an improvement to me as well, now that you've suggested
it. Will do.

https://codereview.chromium.org/417883002/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium/steps.py:547: # TODO: This should be
converted to a PythonBasedTest, although it
On 2014/07/24 15:00:34, Paweł Hajdan Jr. wrote:
> nit: TODO(dpranke)?

Sure, I can add that.

[Dirk Pranke, 2014-07-24 16:24:52]

Updated. Please take another look?

[Paweł Hajdan Jr., 2014-07-25 13:13:14]

LGTM

[Dirk Pranke, 2014-07-25 16:14:48]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-07-25 16:15:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dpranke@chromium.org/417883002/40001

[commit-bot: I haz the power, 2014-07-25 16:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-25 16:15:21]

Failed to apply patch for build/scripts/slave/recipe_modules/chromium/steps.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/scripts/slave/recipe_modules/chromium/steps.py
  Hunk #1 succeeded at 377 (offset -11 lines).
  Hunk #2 FAILED at 530.
  1 out of 2 hunks FAILED -- saving rejects to file
build/scripts/slave/recipe_modules/chromium/steps.py.rej

Patch:       build/scripts/slave/recipe_modules/chromium/steps.py
Index: scripts/slave/recipe_modules/chromium/steps.py
diff --git build/scripts/slave/recipe_modules/chromium/steps.py
build/scripts/slave/recipe_modules/chromium/steps.py
index
3f8ed8aaadcc516b0445eeebf183eadd679722d0..9649c6d962343533c598617f853e9116a684316a
100644
--- a/build/scripts/slave/recipe_modules/chromium/steps.py
+++ b/build/scripts/slave/recipe_modules/chromium/steps.py
@@ -388,55 +388,76 @@ class SwarmingGTestTest(Test):
     return self._results[suffix].failures
 
 
-class TelemetryUnitTests(Test):
-  name = 'telemetry_unittests'
+class PythonBasedTest(Test):
+  @staticmethod
+  def compile_targets(_):
+    return []
+
+  def run_step(self, api, suffix, cmd_args, **kwargs):
+    raise NotImplementedError()
 
   def run(self, api, suffix):
-    # Until telemetry tests output JSON, need to fail on failure with patch.
-    # Otherwise, if the tests were failing on trunks and a cl introduces a
-    # new regression the cl would land since the failure text is hardcoded
-    # below. http://crbug.com/359521.
-    return api.chromium.run_telemetry_unittests(
-        suffix, always_run=True, can_fail_build=True)
+    args = ['--write-full-results-to',
+            api.json.test_results(add_json_log=False)]
+    if suffix == 'without patch':
+      args.extend(self.failures(api, 'with patch'))
 
-  @staticmethod
-  def compile_targets(_):
-    return ['chrome']
+    def followup_fn(step_result):
+      r = step_result.json.test_results
+      p = step_result.presentation
+
+      p.step_text += api.test_utils.format_step_text([
+        ['unexpected_failures:', r.unexpected_failures.keys()],
+      ])
+
+    return self.run_step(
+        api, suffix, args, can_fail_build=(not suffix),
+        step_test_data=lambda: api.json.test_api.canned_test_output(
+            True), followup_fn=followup_fn)
 
   def has_valid_results(self, api, suffix):
-    return True
+    # TODO(dpranke): we should just return zero/nonzero for success/fail.
+    # crbug.com/357866
+    step = api.step_history[self._step_name(suffix)]
+    return (step.json.test_results.valid and
+            step.retcode <= step.json.test_results.MAX_FAILURES_EXIT_STATUS)
 
   def failures(self, api, suffix):
-    # TODO(phajdan.jr): Make it possible to retry individual failing telemetry
-    # tests (add JSON).
-    if api.step_history[self._step_name(suffix)].retcode:
-      return ['telemetry_unittest']
-    return []
+    sn = self._step_name(suffix)
+    return api.step_history[sn].json.test_results.unexpected_failures
 
-class TelemetryPerfUnitTests(Test):
-  name = 'telemetry_perf_unittests'
 
-  def run(self, api, suffix):
-    # Until telemetry tests output JSON, need to fail on failure with patch.
-    # Otherwise, if the tests were failing on trunks and a cl introduces a
-    # new regression the cl would land since the failure text is hardcoded
-    # below. http://crbug.com/359521.
-    return api.chromium.run_telemetry_perf_unittests(
-        suffix, always_run=True, can_fail_build=True)
+class MojoPythonTests(PythonBasedTest):  # pylint: disable=W0232
+  name = 'mojo_python_tests'
+
+  def run_step(self, api, suffix, cmd_args, **kwargs):
+    return api.python(self._step_name(suffix),
+                      api.path['checkout'].join('mojo', 'tools',
+                                                'run_mojo_python_tests.py'),
+                      cmd_args, **kwargs)
+
+
+class TelemetryUnitTests(PythonBasedTest):  # pylint: disable=W0232
+  name = 'telemetry_unittests'
 
   @staticmethod
   def compile_targets(_):
-    return ['chrome']
+      return ['chrome']
 
-  def has_valid_results(self, api, suffix):
-    return True
+  def run_step(self, api, suffix, cmd_args, **kwargs):
+    return api.chromium.run_telemetry_unittests(suffix, cmd_args, **kwargs)
 
-  def failures(self, api, suffix):
-    # TODO(phajdan.jr): Make it possible to retry individual failing telemetry
-    # tests (add JSON).
-    if api.step_history[self._step_name(suffix)].retcode:
-      return ['telemetry_perf_unittests']
-    return []
+
+class TelemetryPerfUnitTests(PythonBasedTest):
+  name = 'telemetry_perf_unittests'
+
+  @staticmethod
+  def compile_targets(_):
+    return ['chrome']
+
+  def run_step(self, api, suffix, cmd_args, **kwargs):
+    return api.chromium.run_telemetry_perf_unittests(suffix, cmd_args,
+                                                     **kwargs)
 
 
 class NaclIntegrationTest(Test):  # pylint: disable=W0232
@@ -509,51 +530,10 @@ class AndroidInstrumentationTest(Test):
     return [self.compile_target]
 
 
-class MojoPythonTests(Test):  # pylint: disable=W0232
-  name = 'mojo_python_tests'
-
-  @staticmethod
-  def compile_targets(_):
-    return []
-
-  def run(self, api, suffix):
-    args = ['--write-full-results-to',
-            api.json.test_results(add_json_log=False)]
-    if suffix == 'without patch':
-      args.extend(self.failures(api, 'with patch'))
-
-    def followup_fn(step_result):
-      r = step_result.json.test_results
-      p = step_result.presentation
-
-      p.step_text += api.test_utils.format_step_text([
-        ['unexpected_failures:', r.unexpected_failures.keys()],
-      ])
-
-    return api.python(
-        self._step_name(suffix),
-        api.path['checkout'].join(
-            'mojo',
-            'tools',
-            'run_mojo_python_tests.py'),
-        args,
-        can_fail_build=(not suffix),
-        step_test_data=lambda: api.json.test_api.canned_test_output(
-            True), followup_fn=followup_fn)
-
-  def has_valid_results(self, api, suffix):
-    # TODO(dpranke): we should just return zero/nonzero for success/fail.
-    # crbug.com/357866
-    step = api.step_history[self._step_name(suffix)]
-    return (step.json.test_results.valid and
-            step.retcode <= step.json.test_results.MAX_FAILURES_EXIT_STATUS)
-
-  def failures(self, api, suffix):
-    sn = self._step_name(suffix)
-    return api.step_history[sn].json.test_results.unexpected_failures
-
-
 class BlinkTest(Test):
+  # TODO(dpranke): This should be converted to a PythonBasedTest, although it
+  # will need custom behavior because we archive the results as well.
+
   name = 'webkit_tests'
 
   def __init__(self, api):

[Dirk Pranke, 2014-07-25 16:25:50]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-07-25 16:26:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dpranke@chromium.org/417883002/60001

[commit-bot: I haz the power, 2014-07-25 16:27:13]

Change committed as 285595

[dmichael (off chromium), 2014-07-25 17:51:15]

A revert of this CL has been created in
https://codereview.chromium.org/419673005/ by dmichael@chromium.org.

The reason for reverting is: Broke telemetry_unittests:
http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests%20%283%29/b....

[Dirk Pranke, 2014-07-25 18:28:55]

On 2014/07/25 17:51:15, dmichael wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/419673005/ by mailto:dmichael@chromium.org.
> 
> The reason for reverting is: Broke telemetry_unittests:
>
http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests%20%283%29/b....

Thanks for the revert! I'll look into the failure.