Improve tool that analyzes gm JSON summary

gm/display_json_results.py

 #!/usr/bin/env python

[epoger, 2013/05/17 17:56:45]

This file was renamed from "confirm_no_failures_in_json.py" using svn mv

 # Copyright (c) 2013 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.
 
-"""Utility to confirm that a JSON summary written by GM contains no failures.
+"""Utility to display a summary of JSON-format GM results, and exit with
+a nonzero errorcode if there were non-ignored failures in the GM results.
 
 Usage:
-  python confirm_no_failures_in_json.py <filename>
+  python display_json_results.py <filename>
+
+TODO(epoger): We may want to add flags so that the caller can choose which
+error types cause a nonzero return code.

[borenet, 2013/05/17 19:48:33]

Other options might be nice as well, for example to clean up the output, we
might want to only list the first 5 or 10 failures in each category.

[epoger, 2013/05/18 03:09:54]

Good idea... added to the TODO.  If it's OK with you, I'd like to commit this
as-is for now (since it is already useful in present form) and add the other
stuff later as needed.

[borenet, 2013/05/20 12:09:55]

Agreed.

 """
 
 __author__ = 'Elliot Poger'
 
 
 import json
 import sys
 
 
 # These constants must be kept in sync with the kJsonKey_ constants in
 # gm_expectations.cpp !
 JSONKEY_ACTUALRESULTS = 'actual-results'
 JSONKEY_ACTUALRESULTS_FAILED = 'failed'
-
-# This is the same indent level as used by jsoncpp, just for consistency.
-JSON_INDENTLEVEL = 3
+JSONKEY_ACTUALRESULTS_FAILUREIGNORED = 'failure-ignored'
+JSONKEY_ACTUALRESULTS_NOCOMPARISON = 'no-comparison'
+JSONKEY_ACTUALRESULTS_SUCCEEDED = 'succeeded'
 
 
-def Assert(filepath):
-  """Raises an exception if the JSON summary at filepath contains any failed
-  tests, or if we were unable to read the JSON summary."""
-  failed_tests = GetFailedTests(filepath)
-  if failed_tests:
-    raise Exception('JSON file %s contained these test failures...\n%s' % (
-        filepath, json.dumps(failed_tests, indent=JSON_INDENTLEVEL)))
+class ResultAccumulator(object):
+  """Object that accumulates results of a given type, and can generate a
+     summary upon request."""
+
+  def __init__(self, name, do_list, do_fail):
+    """name: name of the category this result type falls into
+       do_list: whether to list all of the tests with this results type
+       do_fail: whether to return with nonzero exit code if there are any
+                results of this type
+    """
+    self._name = name
+    self._do_list = do_list
+    self._do_fail = do_fail
+    self._testnames = []
+
+  def AddResult(self, testname):
+    """Adds a result of this particular type.
+       testname: (string) name of the test"""
+    self._testnames.append(testname)
+
+  def ShouldSignalFailure(self):
+    """Returns true if this result type is serious (self._do_fail is True)
+       and there were any results of this type."""
+    if self._do_fail and self._testnames:
+      return True
+    else:
+      return False
+
+  def GetSummaryLine(self):
+    """Returns a single-line string summary of all results added to this
+       accumulator so far."""
+    summary = ''
+    if self._do_fail:
+      summary += '[*] '
+    else:
+      summary += '[ ] '
+    summary += str(len(self._testnames))
+    summary += ' '
+    summary += self._name
+    if self._do_list:
+      summary += ': '
+      for testname in self._testnames:
+        summary += testname
+        summary += ' '
+    return summary
 
 
-def GetFailedTests(filepath):
-  """Returns the dictionary of failed tests from the JSON file at filepath."""
+# Map labels within the JSON file to the ResultAccumulator for each label.
+RESULTS_MAP = {

[borenet, 2013/05/17 19:48:33]

Is there any reason for this to be global?  I'm also wondering if it should be
named like a constant if we're modifying it.

[epoger, 2013/05/18 03:09:54]

No need for it to be global... I moved its definition into Display(), because
that's the only place it is used anyway.

I think it was fine for it to be a constant, anyway, because the dictionary
itself is not modified.  (The ResultAccumulators inside are modified, though.)

+    JSONKEY_ACTUALRESULTS_FAILED:
+        ResultAccumulator(name='ExpectationsMismatch',
+                          do_list=True, do_fail=True),
+    JSONKEY_ACTUALRESULTS_FAILUREIGNORED:
+        ResultAccumulator(name='IgnoredExpectationsMismatch',
+                          do_list=True, do_fail=False),
+    JSONKEY_ACTUALRESULTS_NOCOMPARISON:
+        ResultAccumulator(name='MissingExpectations',
+                          do_list=False, do_fail=False),
+    JSONKEY_ACTUALRESULTS_SUCCEEDED:
+        ResultAccumulator(name='Passed',
+                          do_list=False, do_fail=False),
+}
+
+
+def Display(filepath):
+  """Displays a summary of the results in a JSON file, and returns True if
+     those results indicate a significant failure.
+     filepath: (string) path to JSON file"""
+  failure = False
   json_dict = json.load(open(filepath))
   actual_results = json_dict[JSONKEY_ACTUALRESULTS]
-  return actual_results[JSONKEY_ACTUALRESULTS_FAILED]
+  for label, accumulator in RESULTS_MAP.iteritems():
+    results = actual_results[label]
+    if results:
+      for result in results:
+        accumulator.AddResult(result)
+    print accumulator.GetSummaryLine()
+    failure = failure or accumulator.ShouldSignalFailure()
+  print '(results marked with [*] will cause nonzero return value)'
+  return failure
 
 
 if '__main__' == __name__:

[epoger, 2013/05/17 17:56:45]

Here are two sample runs:

epoger@wpgntat-ubiq141:~/src/skia/white/trunk$ python gm/display_json_results.py
gm/tests/outputs/compared-against-identical-pixels-json/output-expected/json-summary.txt
; echo $?
[ ] 0 IgnoredExpectationsMismatch: 
[*] 0 ExpectationsMismatch: 
[ ] 0 MissingExpectations
[ ] 2 Passed
(results marked with [*] will cause nonzero return value)
0

epoger@wpgntat-ubiq141:~/src/skia/white/trunk$ python gm/display_json_results.py
gm/tests/outputs/compared-against-different-pixels-json/output-expected/json-summary.txt
; echo $?
[ ] 0 IgnoredExpectationsMismatch: 
[*] 2 ExpectationsMismatch: 565/selftest1.png 8888/selftest1.png 
[ ] 0 MissingExpectations
[ ] 0 Passed
(results marked with [*] will cause nonzero return value)
1

   if len(sys.argv) != 2:
     raise Exception('usage: %s <input-json-filepath>' % sys.argv[0])
-  Assert(sys.argv[1])
+  failure = Display(sys.argv[1])

[borenet, 2013/05/17 19:48:33]

You could also do:

sys.exit(Display(sys.argv[1]))

and change "failure" to be 0 or 1.  That has the nice benefit that you aren't
defining a global variable called "failure."

[epoger, 2013/05/18 03:09:54]

If it's OK with you, I would really rather keep the boolean return type for
Display(), because the "return-0-to-signal-success" custom is much more of a
shell-level concept than a Python concept.  A boolean is a cleaner
representation of "did it fail or not".

But I did rewrite this slightly, I hope it gives some of the improvement you
were looking for...

[borenet, 2013/05/20 12:09:55]

Yep!  Now the only thing that's slightly confusing to me is that I've typically
seen functions which return booleans return True on success and False on
failure.

[epoger, 2013/05/21 16:02:51]

Good point.  I inverted the meaning of the returned boolean (and updated the
documentation accordingly)

+  if failure:
+    exit(1)
+  else:
+    exit(0)