Merge ~jslarraz/review-tools:move-report into review-tools:master

[Jorge Sancho Larraz]

many: move Report class away from bin/snap-review, and use it also on individual checks

[Alex Murray (alexmurray)]

LGTM - yay removal of more global variables!

review: Approve

diff --git a/bin/snap-review b/bin/snap-review
index 2c26158..777f418 100755
--- a/bin/snap-review
+++ b/bin/snap-review
@@ -19,107 +19,7 @@ from reviewtools.common import (
 )
 
 from reviewtools.sr_declaration import validate_json
-
-
-class Report(object):
-    results = {}
-    errors = {}
-    warnings = {}
-    info = {}
-
-    def __init__(self, filename, verbose: bool = False):
-        self.filename = filename
-        self.verbose = verbose
-
-    # review_report[<section>][<result_type>][<review_name>] = <result>
-    #   section: section of the report (one per review module)
-    #   result_type: info, warn, error
-    #   review_name: name of the check (prefixed with self.review_type)
-    #   result: contents of the review
-    #   link: url for more information
-    #   manual_review: force manual review
-    #   override_result_type: prefix results with [<result_type>] and set
-    #     result_type to override_result_type
-    def add_result(
-        self,
-        section,
-        result_type,
-        review_name,
-        result,
-        link=None,
-        manual_review=False,
-    ):
-        """Add result to report"""
-        if section not in self.results:
-            self.results[section] = {
-                "info": {},
-                "warn": {},
-                "error": {},
-            }
-
-        if result_type not in self.results[section]:
-            raise Exception("Invalid result type '%s'" % result_type)
-
-        if review_name not in self.results[section][result_type]:
-            self.results[section][result_type][review_name] = dict()
-
-        self.results[section][result_type][review_name].update(
-            {"text": "%s" % result, "manual_review": manual_review}
-        )
-        if link is not None:
-            self.results[section][result_type][review_name]["link"] = link
-
-    def _summarise_results(self):
-        for module in self.results:
-            for key in self.results[module]["error"]:
-                self.errors[key] = self.results[module]["error"][key]
-            for key in self.results[module]["warn"]:
-                self.warnings[key] = self.results[module]["warn"][key]
-            if self.verbose:
-                for key in self.results[module]["info"]:
-                    self.info[key] = self.results[module]["info"][key]
-
-    def print_findings(self, results, description):
-        """
-        Print a summary of the issues found.
-        """
-        if not description or not results:
-            return ""
-        print(description)
-        print("".center(len(description), "-"))
-        for key in sorted(results.keys()):
-            print(" - %s" % key)
-            print("\t%s" % results[key]["text"])
-            if "link" in results[key]:
-                print("\t%s" % results[key]["link"])
-
-    def show_results(self, report_type="console"):
-        self._summarise_results()
-
-        if report_type == "json":
-            print(
-                json.dumps(
-                    self.results, sort_keys=True, indent=2, separators=(",", ": ")
-                )
-            )
-        elif report_type == "sdk":
-            for section in sorted(self.results.keys()):
-                output = self.results[section]
-                print("= %s =" % section)
-                print(
-                    json.dumps(output, sort_keys=True, indent=2, separators=(",", ": "))
-                )
-        else:
-            self.print_findings(self.errors, "Errors")
-            self.print_findings(self.warnings, "Warnings")
-            if self.verbose:
-                self.print_findings(self.info, "Info")
-            if "runtime-errors" in self.results:
-                print("%s: RUNTIME ERROR" % self.filename)
-            elif self.warnings or self.errors:
-                print("%s: FAIL" % self.filename)
-            else:
-                print("%s: pass" % self.filename)
+from reviewtools.report import Report
 
 
 def main():
@@ -244,13 +144,12 @@ def main():
     rc = 0
     report = Report(args.filename, args.verbose)
     for module in modules:
-        section = module.replace("sr_", "snap.v2_")
         try:
             review_class = modules_mod.find_main_class(module)
             review = review_class(args.filename, overrides=overrides)
 
-            review.do_checks()
-            report.results[section] = review.review_report
+            review_report = review.do_checks()
+            report.update(review_report)
         except Exception:
             print("Caught exception (setting rc=1 and continuing):")
             traceback.print_exc(file=sys.stdout)
diff --git a/bin/snap-verify-declaration b/bin/snap-verify-declaration
index 152c6c7..7d7ee40 100755
--- a/bin/snap-verify-declaration
+++ b/bin/snap-verify-declaration
@@ -81,9 +81,16 @@ def main():
             "verify_snap_declaration() raised exception for snap decl: %s" % e,
             output_type=error_output_type,
         )
-    review.set_report_type(error_output_type)
 
-    return review.do_report()
+    report = review.review_report
+    report.show_summary(error_output_type)
+
+    rc = 0
+    if report.errors:
+        rc = 2
+    elif report.warnings:
+        rc = 1
+    return rc
 
 
 if __name__ == "__main__":
diff --git a/reviewtools/common.py b/reviewtools/common.py
index d2c15b8..d85ecc0 100644
--- a/reviewtools/common.py
+++ b/reviewtools/common.py
@@ -35,6 +35,7 @@ import types
 import yaml
 
 from reviewtools.overrides import common_external_symlink_override
+from reviewtools.report import Report
 
 REPORT_OUTPUT = "json"
 RESULT_TYPES = ["info", "warn", "error"]
@@ -156,19 +157,19 @@ class ReviewBase(object):
 
     def __init__(self, review_type, overrides=None):
         self.review_type = review_type
-        # TODO: rename as pkg_report
-        self.review_report = dict()
-
-        global RESULT_TYPES
-        for r in RESULT_TYPES:
-            self.review_report[r] = dict()
-
         self.overrides = overrides if overrides is not None else {}
+        self.review_report = Report()
+
+        # TODO: this hack is used to generate tests/test.sh output. We may want to revisit it.
+        self.section = "snap.v2_" + self.review_type.split("-")[0]
 
-    def set_report_type(self, t):
-        global REPORT_OUTPUT
-        if t is not None and t in ["console", "json"]:
-            REPORT_OUTPUT = t
+        # TODO: initialization here is only needed to ensure consistent output with tests/test.sh.expected
+        # It can be removed if it is ok to not include a section in the report if that section is empty
+        self.review_report.results[self.section] = {
+            "error": {},
+            "warn": {},
+            "info": {},
+        }
 
     def _get_check_name(self, name, app="", extra=""):
         name = ":".join([self.review_type, name])
@@ -195,13 +196,11 @@ class ReviewBase(object):
         manual_review=False,
     ):
         """Add result to report"""
-        global RESULT_TYPES
-        report = self.review_report
-
-        if result_type not in RESULT_TYPES:
-            error("Invalid result type '%s'" % result_type)
+        self.review_report.add_result(
+            self.section, result_type, review_name, result, link, manual_review
+        )
 
-        if review_name not in report[result_type]:
+        if review_name not in self.review_report.results[self.section][result_type]:
             # log info about check so it can be collected into the
             # check-names.list file
             # format should be
@@ -210,32 +209,6 @@ class ReviewBase(object):
             name = ":".join(review_name.split(":")[:2])
             link_text = link if link is not None else ""
             logging.debug(msg.format(name, link_text))
-            report[result_type][review_name] = dict()
-
-        report[result_type][review_name].update(
-            {"text": "%s" % result, "manual_review": manual_review}
-        )
-        if link is not None:
-            report[result_type][review_name]["link"] = link
-
-    # Only called by ./bin/* individually, not 'snap-review'
-    def do_report(self):
-        """Print report"""
-        global REPORT_OUTPUT
-
-        if REPORT_OUTPUT == "json":
-            jsonmsg(self.review_report)
-        else:
-            import pprint
-
-            pprint.pprint(self.review_report)
-
-        rc = 0
-        if len(self.review_report["error"]):
-            rc = 2
-        elif len(self.review_report["warn"]):
-            rc = 1
-        return rc
 
     def do_checks(self):
         """Run all methods that start with check_"""
@@ -250,9 +223,7 @@ class ReviewBase(object):
             func = getattr(self, methodname)
             func()
 
-    def set_review_type(self, name):
-        """Set review name"""
-        self.review_type = name
+        return self.review_report
 
 
 #
@@ -263,42 +234,15 @@ class ReviewBase(object):
 def error(out, exit_code=1, do_exit=True, output_type=None):
     """Print error message and exit"""
     global REPORT_OUTPUT
-    global RESULT_TYPES
-
-    if output_type is not None:
-        ReviewBase.set_report_type(None, output_type)
+    if output_type is None:
+        output_type = REPORT_OUTPUT
 
-    try:
-        if REPORT_OUTPUT == "json":
-            # mock up expected json format:
-            #  {
-            #    "test-family": {
-            #      "error": {
-            #        "test-name": {
-            #          "manual_review": ...,
-            #          "text": ...
-            #        }
-            #      },
-            #      "info": {},
-            #      "warn": {}
-            #    }
-            #  }
-            family = "runtime-errors"
-            name = "msg"
-
-            report = dict()
-            report[family] = dict()
-            for r in RESULT_TYPES:
-                report[family][r] = dict()
-            report[family]["error"][name] = dict()
-            report[family]["error"][name]["text"] = out
-            report[family]["error"][name]["manual_review"] = True
-
-            jsonmsg(report)
-        else:
-            print("ERROR: %s" % (out), file=sys.stderr)
-    except IOError:
-        pass
+    if output_type == "json":
+        report = Report()
+        report.add_result("runtime-errors", "error", "msg", out, manual_review=True)
+        report.show_results(output_type)
+    else:
+        print("ERROR: %s" % (out), file=sys.stderr)
 
     if do_exit:
         sys.exit(exit_code)
@@ -329,11 +273,6 @@ def debug(out):
             pass
 
 
-def jsonmsg(out):
-    """Format out as json"""
-    msg(json.dumps(out, sort_keys=True, indent=2, separators=(",", ": ")))
-
-
 def cmd(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT):
     """Try to execute the given command."""
     debug(" ".join(command))
@@ -782,8 +721,15 @@ def run_check(cls):
         overrides = None
 
     review = cls(fn, overrides=overrides)
-    review.do_checks()
-    rc = review.do_report()
+    report = review.do_checks()
+    report.show_summary()
+
+    rc = 0
+    if report.errors:
+        rc = 2
+    elif report.warnings:
+        rc = 1
+
     sys.exit(rc)
 
 
@@ -950,25 +896,25 @@ def check_results(
         for t in expected.keys():
             for r in expected[t]:
                 testobj.assertTrue(
-                    r in report[t],
+                    r in report.summary[t],
                     "Could not find '%s' (%s) in:\n%s"
-                    % (r, t, json.dumps(report, indent=2)),
+                    % (r, t, json.dumps(report.summary, indent=2)),
                 )
                 for k in expected[t][r]:
                     testobj.assertTrue(
-                        k in report[t][r],
+                        k in report.summary[t][r],
                         "Could not find '%s' (%s) in:\n%s"
-                        % (k, r, json.dumps(report, indent=2)),
+                        % (k, r, json.dumps(report.summary, indent=2)),
                     )
-                testobj.assertEqual(expected[t][r][k], report[t][r][k])
+                testobj.assertEqual(expected[t][r][k], report.summary[t][r][k])
     else:
         for k in expected_counts.keys():
             if expected_counts[k] is None:
                 continue
             testobj.assertEqual(
-                len(report[k]),
+                len(report.summary[k]),
                 expected_counts[k],
-                "(%s not equal)\n%s" % (k, json.dumps(report, indent=2)),
+                "(%s not equal)\n%s" % (k, json.dumps(report.summary, indent=2)),
             )
 
 
diff --git a/reviewtools/report.py b/reviewtools/report.py
new file mode 100644
index 0000000..0364f7c
--- /dev/null
+++ b/reviewtools/report.py
@@ -0,0 +1,141 @@
+import json
+import pprint
+
+
+class Report(object):
+    def __init__(self, snap_name=None, verbose: bool = False):
+        self.snap_name = snap_name if snap_name is not None else ""
+        self.verbose = verbose
+
+        self.results = {}
+        self._summary = {
+            "info": {},
+            "warn": {},
+            "error": {},
+        }
+
+    @property
+    def summary(self) -> dict:
+        self._summarise_results()
+        return self._summary
+
+    @property
+    def errors(self) -> dict:
+        return self.summary["error"]
+
+    @property
+    def warnings(self) -> dict:
+        return self.summary["warn"]
+
+    @property
+    def info(self) -> dict:
+        return self.summary["info"]
+
+    def __iter__(self):
+        # Required for compatibility with test_sr_*
+        return self.summary.__iter__()
+
+    def __getitem__(self, item):
+        # Required for compatibility with test_sr_*
+        return self.summary[item]
+
+    # review_report[<section>][<result_type>][<review_name>] = <result>
+    #   section: section of the report (one per review module)
+    #   result_type: info, warn, error
+    #   review_name: name of the check (prefixed with self.review_type)
+    #   result: contents of the review
+    #   link: url for more information
+    #   manual_review: force manual review
+    #   override_result_type: prefix results with [<result_type>] and set
+    #     result_type to override_result_type
+    def add_result(
+        self,
+        section,
+        result_type,
+        review_name,
+        result,
+        link=None,
+        manual_review=False,
+    ):
+        """Add result to report"""
+        if section not in self.results:
+            self.results[section] = {
+                "info": {},
+                "warn": {},
+                "error": {},
+            }
+
+        if result_type not in self.results[section]:
+            raise Exception("Invalid result type '%s'" % result_type)
+
+        if review_name not in self.results[section][result_type]:
+            self.results[section][result_type][review_name] = dict()
+
+        self.results[section][result_type][review_name].update(
+            {"text": "%s" % result, "manual_review": manual_review}
+        )
+        if link is not None:
+            self.results[section][result_type][review_name]["link"] = link
+
+    def update(self, report):
+        self.results.update(report.results)
+
+    def _summarise_results(self):
+        for module in self.results:
+            for key in self.results[module]["error"]:
+                self._summary["error"][key] = self.results[module]["error"][key]
+            for key in self.results[module]["warn"]:
+                self._summary["warn"][key] = self.results[module]["warn"][key]
+            for key in self.results[module]["info"]:
+                self._summary["info"][key] = self.results[module]["info"][key]
+
+    def print_findings(self, results, description):
+        """
+        Print a summary of the issues found.
+        """
+        if not description or not results:
+            return ""
+        print(description)
+        print("".center(len(description), "-"))
+        for key in sorted(results.keys()):
+            print(" - %s" % key)
+            print("\t%s" % results[key]["text"])
+            if "link" in results[key]:
+                print("\t%s" % results[key]["link"])
+
+    def show_results(self, report_type="console"):
+        if report_type == "json":
+            print(
+                json.dumps(
+                    self.results, sort_keys=True, indent=2, separators=(",", ": ")
+                )
+            )
+        elif report_type == "sdk":
+            for section in sorted(self.results.keys()):
+                output = self.results[section]
+                print("= %s =" % section)
+                print(
+                    json.dumps(output, sort_keys=True, indent=2, separators=(",", ": "))
+                )
+        else:
+            self.print_findings(self.summary["error"], "Errors")
+            self.print_findings(self.summary["warn"], "Warnings")
+            if self.verbose:
+                self.print_findings(self.summary["info"], "Info")
+            if "runtime-errors" in self.results:
+                print("%s: RUNTIME ERROR" % self.snap_name)
+            elif self.summary["warn"] or self.summary["error"]:
+                print("%s: FAIL" % self.snap_name)
+            else:
+                print("%s: pass" % self.snap_name)
+
+    def show_summary(self, report_type="json"):
+        if report_type == "json":
+            print(
+                json.dumps(
+                    self.summary, sort_keys=True, indent=2, separators=(",", ": ")
+                )
+            )
+
+        else:
+            pprint.pprint(self.summary)
diff --git a/tests/test.sh.expected b/tests/test.sh.expected
index 1c726c3..24fbaef 100644
--- a/tests/test.sh.expected
+++ b/tests/test.sh.expected
@@ -119215,7 +119215,7 @@ hello-world_25.snap: FAIL
   "info": {},
   "warn": {}
 }
-= snap.v2_extra_tests =
+= snap.v2_extras =
 {
   "error": {
     "extras-tests-v2:error": {
@@ -119501,7 +119501,7 @@ hello-world_25.snap: FAIL
     "info": {},
     "warn": {}
   },
-  "snap.v2_extra_tests": {
+  "snap.v2_extras": {
     "error": {
       "extras-tests-v2:error": {
         "manual_review": false,