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

tools/testrunner/local/progress.py

Index: tools/testrunner/local/progress.py
diff --git a/tools/testrunner/local/progress.py b/tools/testrunner/local/progress.py
index a9519e926d7552418e56aeaf2a7fb764e86bdfbb..9b03261233da07f28c65d4a479a40074fa76f8ed 100644
--- a/tools/testrunner/local/progress.py
+++ b/tools/testrunner/local/progress.py
@@ -49,10 +49,26 @@ def EscapeCommand(command):
   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
@@ -251,29 +267,19 @@ class MonochromeProgressIndicator(CompactProgressIndicator):
 
 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()
@@ -292,25 +298,17 @@ class JUnitTestProgressIndicator(ProgressIndicator):
         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:
@@ -340,11 +338,7 @@ class JsonTestProgressIndicator(ProgressIndicator):
     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:
@@ -366,9 +360,6 @@ class JsonTestProgressIndicator(ProgressIndicator):
       "duration": test.duration,
     })
 
-  def Heartbeat(self):
-    self.progress_indicator.Heartbeat()
-
 
 PROGRESS_INDICATORS = {
   'verbose': VerboseProgressIndicator,