Bug 1494069 - [mozlint] Return extra 'excludes' directly from pathutils.filterpaths, r=rwood
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 16 Oct 2018 21:04:15 +0000
changeset 500144 9c587734513ad0366938cb19b378aaa15b28d209
parent 500143 3cbd82a01bc27518e21aae6668292c0b6f25802b
child 500145 53abbe1d64d4c976e07be000b2525a75b07f6c0a
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrwood
bugs1494069
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1494069 - [mozlint] Return extra 'excludes' directly from pathutils.filterpaths, r=rwood Currently pathutils.filterpaths computes some extra paths that the underlying linter should try to exclude if it can. It sets these on lintargs['exclude'], and relies on the fact that it was passed by reference to work. However, it seems like pytest does some magic under the hood that prevents this value from being propagated back. It will also fail if 'filterpaths' was invoked in a subprocess. It's much simpler and cleaner to pass this value back directly, rather than rely on a reference. Differential Revision: https://phabricator.services.mozilla.com/D8850
python/mozlint/mozlint/pathutils.py
python/mozlint/mozlint/types.py
python/mozlint/test/linters/excludes.yml
python/mozlint/test/test_pathutils.py
python/mozlint/test/test_roller.py
--- a/python/mozlint/mozlint/pathutils.py
+++ b/python/mozlint/mozlint/pathutils.py
@@ -122,53 +122,46 @@ def collapse(paths, base=None, dotfiles=
 
     if full == covered:
         # Every file under this base was covered, so we can collapse them all
         # up into the base path.
         return [base]
     return covered
 
 
-def filterpaths(paths, linter, **lintargs):
+def filterpaths(root, paths, include, exclude=None, extensions=None):
     """Filters a list of paths.
 
-    Given a list of paths, and a linter definition plus extra
-    arguments, return the set of paths that should be linted.
+    Given a list of paths and some filtering rules, return the set of paths
+    that should be linted.
 
     :param paths: A starting list of paths to possibly lint.
-    :param linter: A linter definition.
-    :param lintargs: Extra arguments passed to the linter.
-    :returns: A list of file paths to lint.
+    :param include: A list of paths that should be included (required).
+    :param exclude: A list of paths that should be excluded (optional).
+    :param extensions: A list of file extensions which should be considered (optional).
+    :returns: A tuple containing a list of file paths to lint and a list of
+              paths to exclude.
     """
-    include = linter.get('include', [])
-    exclude = lintargs.get('exclude', [])
-    exclude.extend(linter.get('exclude', []))
-    root = lintargs['root']
-
-    if not lintargs.get('use_filters', True) or (not include and not exclude):
-        return paths
-
     def normalize(path):
         if '*' not in path and not os.path.isabs(path):
             path = os.path.join(root, path)
         return FilterPath(path)
 
     # Includes are always paths and should always exist.
     include = map(normalize, include)
 
     # Exclude paths with and without globs will be handled separately,
     # pull them apart now.
-    exclude = map(normalize, exclude)
+    exclude = map(normalize, exclude or [])
     excludepaths = [p for p in exclude if p.exists]
     excludeglobs = [p.path for p in exclude if not p.exists]
 
-    extensions = linter.get('extensions')
     keep = set()
     discard = set()
-    for path in map(FilterPath, paths):
+    for path in map(normalize, paths):
         # Exclude bad file extensions
         if extensions and path.isfile and path.ext not in extensions:
             continue
 
         if path.match(excludeglobs):
             continue
 
         # First handle include/exclude directives
@@ -197,19 +190,17 @@ def filterpaths(paths, linter, **lintarg
                     keep.add(path)
 
         # Next expand excludes with globs in them so we can add them to
         # the set of files to discard.
         for pattern in excludeglobs:
             for p, f in path.finder.find(pattern):
                 discard.add(path.join(p))
 
-    # Only pass paths we couldn't exclude here to the underlying linter
-    lintargs['exclude'] = collapse([f.path for f in discard])
-    return [f.path for f in keep]
+    return [f.path for f in keep], collapse([f.path for f in discard])
 
 
 def findobject(path):
     """
     Find a Python object given a path of the form <modulepath>:<objectpath>.
     Conceptually equivalent to
 
         def find_object(modulepath, objectpath):
--- a/python/mozlint/mozlint/types.py
+++ b/python/mozlint/mozlint/types.py
@@ -26,17 +26,26 @@ class BaseType(object):
         """Run linter defined by `config` against `paths` with `lintargs`.
 
         :param paths: Paths to lint. Can be a file or directory.
         :param config: Linter config the 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.Issue` objects.
         """
-        paths = filterpaths(paths, config, **lintargs)
+        if lintargs.get('use_filters', True):
+            paths, exclude = filterpaths(
+                lintargs['root'],
+                paths,
+                config['include'],
+                config.get('exclude', []) + lintargs.get('exclude', []),
+                config.get('extensions', []),
+            )
+            lintargs['exclude'] = exclude
+
         if not paths:
             return
 
         if self.batch:
             return self._lint(paths, config, **lintargs)
 
         errors = []
         try:
new file mode 100644
--- /dev/null
+++ b/python/mozlint/test/linters/excludes.yml
@@ -0,0 +1,10 @@
+---
+ExcludesLinter:
+    description: >-
+        Make sure the string foobar never appears in browser js
+        files because it is bad
+    rule: no-foobar
+    exclude: ['**/foobar.js']
+    extensions: ['.js', 'jsm']
+    type: string
+    payload: foobar
--- a/python/mozlint/test/test_pathutils.py
+++ b/python/mozlint/test/test_pathutils.py
@@ -1,68 +1,34 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function
 
 import os
-from copy import deepcopy
 from fnmatch import fnmatch
 
 import mozunit
 import pytest
 
 from mozlint import pathutils
 
 here = os.path.abspath(os.path.dirname(__file__))
 root = os.path.join(here, 'filter')
 
 
-@pytest.fixture
-def filterpaths():
-    lintargs = {
-        'root': root,
-        'use_filters': True,
-    }
-    os.chdir(lintargs['root'])
-
-    def inner(paths, include, exclude, extensions=None, **kwargs):
-        linter = {
-            'include': include,
-            'exclude': exclude,
-            'extensions': extensions,
-        }
-        largs = deepcopy(lintargs)
-        largs.update(kwargs)
-        return pathutils.filterpaths(paths, linter, **largs)
-
-    return inner
-
-
 def assert_paths(a, b):
     def normalize(p):
         if not os.path.isabs(p):
             p = os.path.join(root, p)
         return os.path.normpath(p)
     assert set(map(normalize, a)) == set(map(normalize, b))
 
 
-def test_no_filter(filterpaths):
-    args = {
-        'paths': ['a.py', 'subdir1/b.py'],
-        'include': ['.'],
-        'exclude': ['**/*.py'],
-        'use_filters': False,
-    }
-
-    paths = filterpaths(**args)
-    assert_paths(paths, args['paths'])
-
-
 TEST_CASES = (
     {
         'paths': ['a.js', 'subdir1/subdir3/d.js'],
         'include': ['.'],
         'exclude': ['subdir1'],
         'expected': ['a.js'],
     },
     {
@@ -91,33 +57,35 @@ TEST_CASES = (
         'exclude': [],
         'extensions': ['py'],
         'expected': ['a.py', 'subdir2'],
     },
 )
 
 
 @pytest.mark.parametrize('test', TEST_CASES)
-def test_filterpaths(filterpaths, test):
+def test_filterpaths(test):
     expected = test.pop('expected')
 
-    paths = filterpaths(**test)
+    paths, exclude = pathutils.filterpaths(root, **test)
     assert_paths(paths, expected)
 
 
 @pytest.mark.parametrize('paths,expected', [
     (['subdir1/*'], ['subdir1']),
     (['subdir2/*'], ['subdir2']),
     (['subdir1/*.*', 'subdir1/subdir3/*', 'subdir2/*'], ['subdir1', 'subdir2']),
     ([root + '/*', 'subdir1/*.*', 'subdir1/subdir3/*', 'subdir2/*'], [root]),
     (['subdir1/b.py', 'subdir1/subdir3'], ['subdir1/b.py', 'subdir1/subdir3']),
     (['subdir1/b.py', 'subdir1/b.js'], ['subdir1/b.py', 'subdir1/b.js']),
     (['subdir1/subdir3'], ['subdir1/subdir3']),
 ])
 def test_collapse(paths, expected):
+    os.chdir(root)
+
     inputs = []
     for path in paths:
         base, name = os.path.split(path)
         if '*' in name:
             for n in os.listdir(base):
                 if not fnmatch(n, name):
                     continue
                 inputs.append(os.path.join(base, n))
--- a/python/mozlint/test/test_roller.py
+++ b/python/mozlint/test/test_roller.py
@@ -82,26 +82,34 @@ def test_roll_from_subdir(lint, linters)
 def test_roll_catch_exception(lint, linters, files, capfd):
     lint.read(linters('raises'))
 
     lint.roll(files)  # assert not raises
     out, err = capfd.readouterr()
     assert 'LintException' in err
 
 
-def test_roll_with_excluded_path(lint, linters, files):
+def test_roll_with_global_excluded_path(lint, linters, files):
     lint.lintargs.update({'exclude': ['**/foobar.js']})
 
     lint.read(linters('string', 'regex', 'external'))
     result = lint.roll(files)
 
     assert len(result.issues) == 0
     assert result.failed == set([])
 
 
+def test_roll_with_local_excluded_path(lint, linters, files):
+    lint.read(linters('excludes'))
+    result = lint.roll(files)
+
+    assert len(result.issues) == 0
+    assert result.failed == set([])
+
+
 def test_roll_with_no_files_to_lint(lint, linters, capfd):
     lint.read(linters('string', 'regex', 'external'))
     lint.mock_vcs([])
     result = lint.roll([], workdir=True)
     assert isinstance(result, ResultSummary)
     assert len(result.issues) == 0
     assert len(result.failed) == 0