Bug 1316925 - Keep track of failed linters in stylish formatter summary, r?jgraham draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Mon, 14 Nov 2016 11:56:46 -0500
changeset 438624 6988a14cb72be5d36596c8e01815d4f0089ab5f5
parent 438429 458c900dd4ef310d5bffae1f2bb97da50839cc66
child 536974 8618927d2e9c74a46fed24ee72a078c4a9923ece
push id35793
push userahalberstadt@mozilla.com
push dateMon, 14 Nov 2016 22:17:09 +0000
reviewersjgraham
bugs1316925
milestone53.0a1
Bug 1316925 - Keep track of failed linters in stylish formatter summary, r?jgraham This replaces the "return_code" property on the LintRoller object with a list of "failed" linters. This is a bit more useful as it lets us know exactly which linters had a problem (whereas previously we just knew *something* went wrong). This patch pushes determining the return code back into cli.py, which I think is fine. In addition, we now pass the list of failed linters into the formatter. This allows us to clarify exactly how many linters hit a failure which is a lot better than a seemingly "successful" summary message. Finally I also removed the "no files to lint" message because I've seen several people confuse it for an error. I'll probably add it back as a debug log message when we switch to using mozlog for output. MozReview-Commit-ID: 4wyCeOZdOf8
python/mozlint/mozlint/cli.py
python/mozlint/mozlint/formatters/__init__.py
python/mozlint/mozlint/formatters/stylish.py
python/mozlint/mozlint/formatters/treeherder.py
python/mozlint/mozlint/roller.py
python/mozlint/mozlint/types.py
python/mozlint/test/test_roller.py
--- a/python/mozlint/mozlint/cli.py
+++ b/python/mozlint/mozlint/cli.py
@@ -100,16 +100,16 @@ def run(paths, linters, fmt, rev, workdi
     # run all linters
     results = lint.roll(paths, rev=rev, workdir=workdir)
 
     formatter = formatters.get(fmt)
 
     # Explicitly utf-8 encode the output as some of the formatters make
     # use of unicode characters. This will prevent a UnicodeEncodeError
     # on environments where utf-8 isn't the default
-    print(formatter(results).encode('utf-8', 'replace'))
-    return lint.return_code
+    print(formatter(results, failed=lint.failed).encode('utf-8', 'replace'))
+    return 1 if results or lint.failed else 0
 
 
 if __name__ == '__main__':
     parser = MozlintParser()
     args = vars(parser.parse_args())
     sys.exit(run(**args))
--- a/python/mozlint/mozlint/formatters/__init__.py
+++ b/python/mozlint/mozlint/formatters/__init__.py
@@ -5,17 +5,17 @@
 import json
 
 from ..result import ResultEncoder
 from .stylish import StylishFormatter
 from .treeherder import TreeherderFormatter
 
 
 class JSONFormatter(object):
-    def __call__(self, results):
+    def __call__(self, results, **kwargs):
         return json.dumps(results, cls=ResultEncoder)
 
 
 all_formatters = {
     'json': JSONFormatter,
     'stylish': StylishFormatter,
     'treeherder': TreeherderFormatter,
 }
--- a/python/mozlint/mozlint/formatters/stylish.py
+++ b/python/mozlint/mozlint/formatters/stylish.py
@@ -38,17 +38,17 @@ class StylishFormatter(object):
     _colors = {
         'grey': [247, 8, 7],
         'red': [1],
         'yellow': [3],
         'brightred': [9, 1],
         'brightyellow': [11, 3],
     }
     fmt = "  {c1}{lineno}{column}  {c2}{level}{normal}  {message}  {c1}{rule}({linter}){normal}"
-    fmt_summary = "{t.bold}{c}\u2716 {problem} ({error}, {warning}){t.normal}"
+    fmt_summary = "{t.bold}{c}\u2716 {problem} ({error}, {warning}{failure}){t.normal}"
 
     def __init__(self, disable_colors=None):
         if disable_colors or not blessings:
             self.term = NullTerminal()
         else:
             self.term = blessings.Terminal()
         self.num_colors = self.term.number_of_colors
 
@@ -72,18 +72,19 @@ class StylishFormatter(object):
         self.max_level = max(self.max_level, len(str(err.level)))
         self.max_message = max(self.max_message, len(err.message))
 
     def _pluralize(self, s, num):
         if num != 1:
             s += 's'
         return str(num) + ' ' + s
 
-    def __call__(self, result):
+    def __call__(self, result, failed=None, **kwargs):
         message = []
+        failed = failed or []
 
         num_errors = 0
         num_warnings = 0
         for path, errors in sorted(result.iteritems()):
             self._reset_max()
 
             message.append(self.term.underline(path))
             # Do a first pass to calculate required padding
@@ -105,18 +106,26 @@ class StylishFormatter(object):
                     level=err.level.ljust(self.max_level),
                     message=err.message.ljust(self.max_message),
                     rule='{} '.format(err.rule) if err.rule else '',
                     linter=err.linter.lower(),
                 ))
 
             message.append('')  # newline
 
+        # If there were failures, make it clear which linters failed
+        for fail in failed:
+            message.append("{c}A failure occured in the {name} linter.".format(
+                c=self.color('brightred'),
+                name=fail,
+            ))
+
         # Print a summary
         message.append(self.fmt_summary.format(
             t=self.term,
-            c=self.color('brightred') if num_errors else self.color('brightyellow'),
-            problem=self._pluralize('problem', num_errors + num_warnings),
+            c=self.color('brightred') if num_errors or failed else self.color('brightyellow'),
+            problem=self._pluralize('problem', num_errors + num_warnings + len(failed)),
             error=self._pluralize('error', num_errors),
             warning=self._pluralize('warning', num_warnings),
+            failure=', {}'.format(self._pluralize('failure', len(failed))) if failed else '',
         ))
 
         return '\n'.join(message)
--- a/python/mozlint/mozlint/formatters/treeherder.py
+++ b/python/mozlint/mozlint/formatters/treeherder.py
@@ -11,17 +11,17 @@ class TreeherderFormatter(object):
     """Formatter for treeherder friendly output.
 
     This formatter looks ugly, but prints output such that
     treeherder is able to highlight the errors and warnings.
     This is a stop-gap until bug 1276486 is fixed.
     """
     fmt = "TEST-UNEXPECTED-{level} | {path}:{lineno}{column} | {message} ({rule})"
 
-    def __call__(self, result):
+    def __call__(self, result, **kwargs):
         message = []
         for path, errors in sorted(result.iteritems()):
             for err in errors:
                 assert isinstance(err, ResultContainer)
 
                 d = {s: getattr(err, s) for s in err.__slots__}
                 d["column"] = ":%s" % d["column"] if d["column"] else ""
                 d['level'] = d['level'].upper()
--- a/python/mozlint/mozlint/roller.py
+++ b/python/mozlint/mozlint/roller.py
@@ -20,40 +20,40 @@ from .errors import LintersNotConfigured
 from .types import supported_types
 from .parser import Parser
 from .vcs import VCSFiles
 
 
 def _run_linters(queue, paths, **lintargs):
     parse = Parser()
     results = defaultdict(list)
-    return_code = 0
+    failed = []
 
     while True:
         try:
             # The astute reader may wonder what is preventing the worker from
             # grabbing the next linter from the queue after a SIGINT. Because
             # this is a Manager.Queue(), it is itself in a child process which
             # also received SIGINT. By the time the worker gets back here, the
             # Queue is dead and IOError is raised.
             linter_path = queue.get(False)
         except (Empty, IOError):
-            return results, return_code
+            return results, failed
 
         # Ideally we would pass the entire LINTER definition as an argument
         # to the worker instead of re-parsing it. But passing a function from
         # a dynamically created module (with imp) does not seem to be possible
         # with multiprocessing on Windows.
         linter = parse(linter_path)
         func = supported_types[linter['type']]
         res = func(paths, linter, **lintargs) or []
 
         if not isinstance(res, (list, tuple)):
             if res:
-                return_code = 1
+                failed.append(linter['name'])
             continue
 
         for r in res:
             results[r.path].append(r)
 
 
 def _run_worker(*args, **lintargs):
     try:
@@ -79,17 +79,18 @@ class LintRoller(object):
     def __init__(self, root=None, **lintargs):
         self.parse = Parser()
         self.vcs = VCSFiles()
 
         self.linters = []
         self.lintargs = lintargs
         self.lintargs['root'] = root or self.vcs.root or os.getcwd()
 
-        self.return_code = None
+        # linters that return non-zero
+        self.failed = None
 
     def read(self, paths):
         """Parse one or more linters and add them to the registry.
 
         :param paths: A path or iterable of paths to linter definitions.
         """
         if isinstance(paths, basestring):
             paths = (paths,)
@@ -138,17 +139,17 @@ class LintRoller(object):
         for i in range(num_procs):
             workers.append(
                 pool.apply_async(_run_worker, args=(queue, paths), kwds=self.lintargs))
         pool.close()
 
         # ignore SIGINT in parent so we can still get partial results
         # from child processes. These should shutdown quickly anyway.
         signal.signal(signal.SIGINT, signal.SIG_IGN)
-        self.return_code = 0
+        self.failed = []
         for worker in workers:
             # parent process blocks on worker.get()
-            results, return_code = worker.get()
-            if results or return_code:
-                self.return_code = 1
+            results, failed = worker.get()
+            if failed:
+                self.failed.extend(failed)
             for k, v in results.iteritems():
                 all_results[k].extend(v)
         return all_results
--- a/python/mozlint/mozlint/types.py
+++ b/python/mozlint/mozlint/types.py
@@ -26,17 +26,16 @@ class BaseType(object):
         :param paths: Paths to lint. Can be a file or directory.
         :param linter: Linter definition paths are being linted against.
         :param lintargs: External arguments to the linter not defined in
                          the definition, but passed in by a consumer.
         :returns: A list of :class:`~result.ResultContainer` objects.
         """
         paths = filterpaths(paths, linter, **lintargs)
         if not paths:
-            print("{}: no files to lint in specified paths".format(linter['name']))
             return
 
         if self.batch:
             return self._lint(paths, linter, **lintargs)
 
         errors = []
         try:
             for p in paths:
--- a/python/mozlint/test/test_roller.py
+++ b/python/mozlint/test/test_roller.py
@@ -22,17 +22,17 @@ def test_roll_no_linters_configured(lint
         lint.roll(files)
 
 
 def test_roll_successful(lint, linters, files):
     lint.read(linters)
 
     result = lint.roll(files)
     assert len(result) == 1
-    assert lint.return_code == 1
+    assert lint.failed == []
 
     path = result.keys()[0]
     assert os.path.basename(path) == 'foobar.js'
 
     errors = result[path]
     assert isinstance(errors, list)
     assert len(errors) == 6
 
@@ -54,29 +54,29 @@ def test_roll_catch_exception(lint, lint
 
 def test_roll_with_excluded_path(lint, linters, files):
     lint.lintargs.update({'exclude': ['**/foobar.js']})
 
     lint.read(linters)
     result = lint.roll(files)
 
     assert len(result) == 0
-    assert lint.return_code == 0
+    assert lint.failed == []
 
 
 def test_roll_with_invalid_extension(lint, lintdir, filedir):
     lint.read(os.path.join(lintdir, 'external.lint'))
     result = lint.roll(os.path.join(filedir, 'foobar.py'))
     assert len(result) == 0
-    assert lint.return_code == 0
+    assert lint.failed == []
 
 
 def test_roll_with_failure_code(lint, lintdir, files):
     lint.read(os.path.join(lintdir, 'badreturncode.lint'))
 
-    assert lint.return_code is None
+    assert lint.failed is None
     result = lint.roll(files)
     assert len(result) == 0
-    assert lint.return_code == 1
+    assert lint.failed == ['BadReturnCodeLinter']
 
 
 if __name__ == '__main__':
     sys.exit(pytest.main(['--verbose', __file__]))