[test] Refactoring - Use subject/observer pattern for progress indicators.

tools/testrunner/local/progress.py

 # Copyright 2012 the V8 project authors. All rights reserved.
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 #       notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 #       copyright notice, this list of conditions and the following
 #       disclaimer in the documentation and/or other materials provided
@@ skipping 31 matching lines @@
   for part in command:
     if ' ' in part:
       # Escape spaces.  We may need to escape more characters for this
       # to work properly.
       parts.append('"%s"' % part)
     else:
       parts.append(part)
   return " ".join(parts)
 
 
+class IndicatorNotifier:

[tandrii(chromium), 2015/06/09 09:18:01]

inherit from "(object)"

[Michael Achenbach, 2015/06/09 12:47:14]

Done.

+  """Holds a list of progress indicators and notifies them all on events."""
+  def __init__(self):
+    self.indicators = []
+
+  def register(self, indicator):

[tandrii(chromium), 2015/06/09 09:18:01]

nit: maybe Register for consistency with code around?

[Michael Achenbach, 2015/06/09 12:47:14]

Done.

+    self.indicators.append(indicator)
+
+  def __getattr__(self, func_name):
+    """Generic event."""

[tandrii(chromium), 2015/06/09 09:18:01]

how about: """Generic event dispatcher.""" ?

and I'd add an assert:

assert callable(ProgressIndicator.__dict__[func_name]), "unknown event: %s" %
func_name

[Michael Achenbach, 2015/06/09 12:47:14]

now changed design...

+    def func(*args, **kwargs):
+      for indicator in self.indicators:
+        getattr(indicator, func_name)(*args, **kwargs)

[tandrii(chromium), 2015/06/09 09:18:01]

maybe also add setattr(self, func_name, func) ?

[Michael Achenbach, 2015/06/09 12:47:14]

now changed design...

+    return func
+
+
 class ProgressIndicator(object):
 
-  def __init__(self):
-    self.runner = None
+  def SetRunner(self, runner):

[tandrii(chromium), 2015/06/09 09:18:01]

IMO, I'd keep the constructor.

Am I right that SetRunner is really for documentation, not really for anything
else? because bla.runner = runner still works anyways...

[Jakob Kummerow, 2015/06/09 09:49:51]

Agreed. The constructor exists mostly for documentation purposes too -- the idea
is that looking at the constructor is enough to understand the object layout, as
all fields are initialized there.
I guess unless setattr is implemented as you suggested above, exposing a setter
is the only [elegant] way to set the runner for all registered progress
indicators.

[tandrii(chromium), 2015/06/09 10:14:34]

I don't think setattr above will change anything, as it only binds functions.
Setters are not pythonic, properties are. Yet i'm guilty of using them myself,
purely for being more explicit = e.g., documentation. So, don't mind having it
SetRunner() here :)

[Michael Achenbach, 2015/06/09 12:47:14]

I keep the constructor, but I also need the setter to not be required to make
also attribute assignment generic in the IndicatorNotifier (if I add a generic
__setattr__ it also collides with the initialization of self.indicators = []).

[tandrii(chromium), 2015/06/09 13:24:45]

Acknowledged.

+    self.runner = runner
 
   def Starting(self):
     pass
 
   def Done(self):
     pass
 
   def AboutToRun(self, test):
     pass
 
@@ skipping 178 matching lines @@
       'stderr': '%s',
     }
     super(MonochromeProgressIndicator, self).__init__(templates)
 
   def ClearLine(self, last_line_length):
     print ("\r" + (" " * last_line_length) + "\r"),
 
 
 class JUnitTestProgressIndicator(ProgressIndicator):
 
-  def __init__(self, progress_indicator, junitout, junittestsuite):
-    self.progress_indicator = progress_indicator
+  def __init__(self, junitout, junittestsuite):
     self.outputter = junit_output.JUnitTestOutput(junittestsuite)
     if junitout:
       self.outfile = open(junitout, "w")
     else:
       self.outfile = sys.stdout
 
-  def Starting(self):
-    self.progress_indicator.runner = self.runner
-    self.progress_indicator.Starting()
-
   def Done(self):
-    self.progress_indicator.Done()
     self.outputter.FinishAndWrite(self.outfile)
     if self.outfile != sys.stdout:
       self.outfile.close()
 
-  def AboutToRun(self, test):
-    self.progress_indicator.AboutToRun(test)
-
   def HasRun(self, test, has_unexpected_output):
-    self.progress_indicator.HasRun(test, has_unexpected_output)
     fail_text = ""
     if has_unexpected_output:
       stdout = test.output.stdout.strip()
       if len(stdout):
         fail_text += "stdout:\n%s\n" % stdout
       stderr = test.output.stderr.strip()
       if len(stderr):
         fail_text += "stderr:\n%s\n" % stderr
       fail_text += "Command: %s" % EscapeCommand(self.runner.GetCommand(test))
       if test.output.HasCrashed():
         fail_text += "exit code: %d\n--- CRASHED ---" % test.output.exit_code
       if test.output.HasTimedOut():
         fail_text += "--- TIMEOUT ---"
     self.outputter.HasRunTest(
         [test.GetLabel()] + self.runner.context.mode_flags + test.flags,
         test.duration,
         fail_text)
 
-  def Heartbeat(self):
-    self.progress_indicator.Heartbeat()
 
 class JsonTestProgressIndicator(ProgressIndicator):
 
-  def __init__(self, progress_indicator, json_test_results, arch, mode):
-    self.progress_indicator = progress_indicator
+  def __init__(self, json_test_results, arch, mode):
     self.json_test_results = json_test_results
     self.arch = arch
     self.mode = mode
     self.results = []
     self.tests = []
 
-  def Starting(self):
-    self.progress_indicator.runner = self.runner
-    self.progress_indicator.Starting()
-
   def Done(self):
-    self.progress_indicator.Done()
     complete_results = []
     if os.path.exists(self.json_test_results):
       with open(self.json_test_results, "r") as f:
         # Buildbot might start out with an empty file.
         complete_results = json.loads(f.read() or "[]")
 
     # Sort tests by duration.
     timed_tests = [t for t in self.tests if t.duration is not None]
     timed_tests.sort(lambda a, b: cmp(b.duration, a.duration))
     slowest_tests = [
       {
         "name": test.GetLabel(),
         "flags": test.flags,
         "command": EscapeCommand(self.runner.GetCommand(test)).replace(
             ABS_PATH_PREFIX, ""),
         "duration": test.duration,
       } for test in timed_tests[:20]
     ]
 
     complete_results.append({
       "arch": self.arch,
       "mode": self.mode,
       "results": self.results,
       "slowest_tests": slowest_tests,
     })
 
     with open(self.json_test_results, "w") as f:
       f.write(json.dumps(complete_results))
 
-  def AboutToRun(self, test):
-    self.progress_indicator.AboutToRun(test)
-
   def HasRun(self, test, has_unexpected_output):
-    self.progress_indicator.HasRun(test, has_unexpected_output)
     # Buffer all tests for sorting the durations in the end.
     self.tests.append(test)
     if not has_unexpected_output:
       # Omit tests that run as expected. Passing tests of reruns after failures
       # will have unexpected_output to be reported here has well.
       return
 
     self.results.append({
       "name": test.GetLabel(),
       "flags": test.flags,
       "command": EscapeCommand(self.runner.GetCommand(test)).replace(
           ABS_PATH_PREFIX, ""),
       "run": test.run,
       "stdout": test.output.stdout,
       "stderr": test.output.stderr,
       "exit_code": test.output.exit_code,
       "result": test.suite.GetOutcome(test),
       "expected": list(test.outcomes or ["PASS"]),
       "duration": test.duration,
     })
 
-  def Heartbeat(self):
-    self.progress_indicator.Heartbeat()
-
 
 PROGRESS_INDICATORS = {
   'verbose': VerboseProgressIndicator,
   'dots': DotsProgressIndicator,
   'color': ColorProgressIndicator,
   'mono': MonochromeProgressIndicator
 }