# HG changeset patch
# User Andrew Halberstadt <ahalberstadt@mozilla.com>
# Date 1521753855 14400
# Node ID ad34ac3d45a62dff961bd247e36ab209af2cc0e5
# Parent  c56baa0de4506fdb0f1fff8262fb1086ccd3a448
Bug 1436802 - [lint] Add some tests for the flake8 linter integration r=andi

This essentially tests tools/lint/python/flake8.py. Though it also adds
a basic framework for testing all the other linters as well. Getting this
added now will allow others to collaborate on adding more tests without
needing to get to 100% coverage for all linters right off the bat.

All python tests under tools/lint/test will run as part of the 'ml' task
on Linux, and the build task on Windows (OSX coverage is currently missing
for python tests).

The flake8 linter currently has a bug where the 'exclude' argument is
ignored. This is why we need to also exclude 'tools/lint/test/files' in
topsrcdir/.flake8, even though it is already listed in the
'mach_commands.py'. Other linters shouldn't need to do this, the exclusion
in 'mach_commands.py' should be good enough. See bug 1277851 for more
details.

MozReview-Commit-ID: 9ho8C83eeuj

diff --git a/.flake8 b/.flake8
--- a/.flake8
+++ b/.flake8
@@ -1,6 +1,7 @@
 [flake8]
 # See http://pep8.readthedocs.io/en/latest/intro.html#configuration
 ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402, E741
 max-line-length = 99
 exclude =
     testing/mochitest/pywebsocket,
+    tools/lint/test/files,
diff --git a/tools/lint/mach_commands.py b/tools/lint/mach_commands.py
--- a/tools/lint/mach_commands.py
+++ b/tools/lint/mach_commands.py
@@ -33,17 +33,17 @@ class MachCommands(MachCommandBase):
     @Command(
         'lint', category='devenv',
         description='Run linters.',
         parser=setup_argument_parser)
     def lint(self, *runargs, **lintargs):
         """Run linters."""
         from mozlint import cli
         lintargs.setdefault('root', self.topsrcdir)
-        lintargs['exclude'] = ['obj*']
+        lintargs['exclude'] = ['obj*', 'tools/lint/test/files']
         cli.SEARCH_PATHS.append(here)
         self._activate_virtualenv()
         return cli.run(*runargs, **lintargs)
 
     @Command('eslint', category='devenv',
              description='Run eslint or help configure eslint for optimal development.')
     @CommandArgument('paths', default=None, nargs='*',
                      help="Paths to file or directories to lint, like "
diff --git a/tools/lint/python/flake8.py b/tools/lint/python/flake8.py
--- a/tools/lint/python/flake8.py
+++ b/tools/lint/python/flake8.py
@@ -136,16 +136,20 @@ def run_process(config, cmd):
 
 def setup(root):
     if not reinstall_flake8():
         print(FLAKE8_INSTALL_ERROR)
         return 1
 
 
 def lint(paths, config, **lintargs):
+    # TODO don't store results in a global
+    global results
+    results = []
+
     cmdargs = [
         os.path.join(bindir, 'flake8'),
         '--format', '{"path":"%(path)s","lineno":%(row)s,'
                     '"column":%(col)s,"rule":"%(code)s","message":"%(text)s"}',
     ]
 
     # Run any paths with a .flake8 file in the directory separately so
     # it gets picked up. This means only .flake8 files that live in
diff --git a/tools/lint/test/conftest.py b/tools/lint/test/conftest.py
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/conftest.py
@@ -0,0 +1,98 @@
+import os
+import sys
+from collections import defaultdict
+
+from mozbuild.base import MozbuildObject
+from mozlint.pathutils import findobject
+from mozlint.parser import Parser
+
+import pytest
+
+here = os.path.abspath(os.path.dirname(__file__))
+build = MozbuildObject.from_environment(cwd=here)
+
+lintdir = os.path.dirname(here)
+sys.path.insert(0, lintdir)
+
+
+@pytest.fixture(scope='module')
+def root(request):
+    """Return the root directory for the files of the linter under test.
+
+    For example, with LINTER=flake8 this would be tools/lint/test/files/flake8.
+    """
+    if not hasattr(request.module, 'LINTER'):
+        pytest.fail("'root' fixture used from a module that didn't set the LINTER variable")
+    return os.path.join(here, 'files', request.module.LINTER)
+
+
+@pytest.fixture(scope='module')
+def paths(root):
+    """Return a function that can resolve file paths relative to the linter
+    under test.
+
+    Can be used like `paths('foo.py', 'bar/baz')`. This will return a list of
+    absolute paths under the `root` files directory.
+    """
+    def _inner(*paths):
+        if not paths:
+            return [root]
+        return [os.path.normpath(os.path.join(root, p)) for p in paths]
+    return _inner
+
+
+@pytest.fixture(scope='module')
+def config(request):
+    """Finds, loads and returns the config for the linter name specified by the
+    LINTER global variable in the calling module.
+
+    This implies that each test file (that uses this fixture) should only be
+    used to test a single linter. If no LINTER variable is defined, the test
+    will fail.
+    """
+    if not hasattr(request.module, 'LINTER'):
+        pytest.fail("'config' fixture used from a module that didn't set the LINTER variable")
+
+    name = request.module.LINTER
+    config_path = os.path.join(lintdir, '{}.yml'.format(name))
+    parser = Parser()
+    # TODO Don't assume one linter per yaml file
+    return parser.parse(config_path)[0]
+
+
+@pytest.fixture(scope='module', autouse=True)
+def run_setup(config):
+    """Make sure that if the linter named in the LINTER global variable has a
+    setup function, it gets called before running the tests.
+    """
+    if 'setup' not in config:
+        return
+
+    func = findobject(config['setup'])
+    func(build.topsrcdir)
+
+
+@pytest.fixture(scope='module')
+def lint(config, root):
+    """Find and return the 'lint' function for the external linter named in the
+    LINTER global variable.
+
+    This will automatically pass in the 'config' and 'root' arguments if not
+    specified.
+    """
+    try:
+        func = findobject(config['payload'])
+    except (ImportError, ValueError):
+        pytest.fail("could not resolve a lint function from '{}'".format(config['payload']))
+
+    def wrapper(paths, config=config, root=root, collapse_results=False, **lintargs):
+        results = func(paths, config, root=root, **lintargs)
+        if not collapse_results:
+            return results
+
+        ret = defaultdict(list)
+        for r in results:
+            ret[r.path].append(r)
+        return ret
+
+    return wrapper
diff --git a/tools/lint/test/files/flake8/bad.py b/tools/lint/test/files/flake8/bad.py
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/files/flake8/bad.py
@@ -0,0 +1,5 @@
+# Unused import
+import distutils
+
+print("This is a line that is over 80 characters but under 100. It shouldn't fail.")
+print("This is a line that is over not only 80, but 100 characters. It should most certainly cause a failure.")
diff --git a/tools/lint/test/files/flake8/custom/.flake8 b/tools/lint/test/files/flake8/custom/.flake8
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/files/flake8/custom/.flake8
@@ -0,0 +1,4 @@
+[flake8]
+max-line-length=110
+ignore=
+    F401
diff --git a/tools/lint/test/files/flake8/custom/good.py b/tools/lint/test/files/flake8/custom/good.py
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/files/flake8/custom/good.py
@@ -0,0 +1,5 @@
+# Unused import
+import distutils
+
+print("This is a line that is over 80 characters but under 100. It shouldn't fail.")
+print("This is a line that is over not only 80, but 100 characters. It should also not cause a failure.")
diff --git a/tools/lint/test/python.ini b/tools/lint/test/python.ini
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/python.ini
@@ -0,0 +1,4 @@
+[DEFAULT]
+subsuite=mozlint, os == "linux"
+
+[test_flake8.py]
diff --git a/tools/lint/test/test_flake8.py b/tools/lint/test/test_flake8.py
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/test_flake8.py
@@ -0,0 +1,45 @@
+import mozunit
+import pytest
+
+LINTER = 'flake8'
+
+
+def test_lint_single_file(lint, paths):
+    results = lint(paths('bad.py'))
+    assert len(results) == 2
+    assert results[0].rule == 'F401'
+    assert results[1].rule == 'E501'
+    assert results[1].lineno == 5
+
+    # run lint again to make sure the previous results aren't counted twice
+    results = lint(paths('bad.py'))
+    assert len(results) == 2
+
+
+def test_lint_custom_config(lint, paths):
+    results = lint(paths('custom'))
+    assert len(results) == 0
+
+    results = lint(paths('custom/good.py'))
+    assert len(results) == 0
+
+    results = lint(paths('custom', 'bad.py'))
+    assert len(results) == 2
+
+
+@pytest.mark.xfail(
+    strict=True, reason="Bug 1277851 - custom configs are ignored if specifying a parent path")
+def test_lint_custom_config_from_parent_path(lint, paths):
+    results = lint(paths(), collapse_results=True)
+    assert paths('custom/good.py')[0] not in results
+
+
+@pytest.mark.xfail(strict=True, reason="Bug 1277851 - 'exclude' argument is ignored")
+def test_lint_excluded_file(lint, paths):
+    paths = paths('bad.py')
+    results = lint(paths, exclude=paths)
+    assert len(results) == 0
+
+
+if __name__ == '__main__':
+    mozunit.main()
diff --git a/tools/moz.build b/tools/moz.build
--- a/tools/moz.build
+++ b/tools/moz.build
@@ -37,8 +37,12 @@ with Files("rb/**"):
 with Files("rewriting/**"):
     BUG_COMPONENT = ("Core", "Rewriting and Analysis")
 
 with Files("update-packaging/**"):
     BUG_COMPONENT = ("Release Engineering", "Other")
 
 SPHINX_TREES['lint'] = 'lint/docs'
 SPHINX_TREES['compare-locales'] = 'compare-locales/docs'
+
+PYTHON_UNITTEST_MANIFESTS += [
+    'lint/test/python.ini',
+]