Bug 1436802 - [lint] Add some tests for the flake8 linter integration draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Thu, 22 Mar 2018 17:24:15 -0400
changeset 774756 01d460fdc136f2764b11c54864be766050f9c61c
parent 774711 8c71359d60e21055074ae8bc3dcb796d20f0cbaf
push id104495
push userahalberstadt@mozilla.com
push dateThu, 29 Mar 2018 14:28:13 +0000
bugs1436802, 1277851
milestone61.0a1
Bug 1436802 - [lint] Add some tests for the flake8 linter integration 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
.flake8
taskcluster/ci/source-test/python.yml
tools/lint/mach_commands.py
tools/lint/python/flake8.py
tools/lint/test/conftest.py
tools/lint/test/files/flake8/bad.py
tools/lint/test/files/flake8/custom/.flake8
tools/lint/test/files/flake8/custom/good.py
tools/lint/test/python.ini
tools/lint/test/test_flake8.py
tools/moz.build
--- 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,
--- a/taskcluster/ci/source-test/python.yml
+++ b/taskcluster/ci/source-test/python.yml
@@ -102,16 +102,17 @@ mozlint:
     description: python/mozlint unit tests
     treeherder:
         symbol: py(ml)
     run:
         mach: python-test --subsuite mozlint
     when:
         files-changed:
             - 'python/mozlint/**'
+            - 'tools/lint/**'
 
 mozterm:
     description: python/mozterm unit tests
     treeherder:
         symbol: py(term)
     run:
         mach: python-test --subsuite mozterm
     when:
--- 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 "
--- 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
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
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.")
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
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.")
new file mode 100644
--- /dev/null
+++ b/tools/lint/test/python.ini
@@ -0,0 +1,4 @@
+[DEFAULT]
+subsuite=mozlint, os == "linux"
+
+[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()
--- a/tools/moz.build
+++ b/tools/moz.build
@@ -59,10 +59,11 @@ SPHINX_TREES['try'] = 'tryselect/docs'
 with Files('tryselect/docs/**'):
     SCHEDULES.exclusive = ['docs']
 
 CRAMTEST_MANIFESTS += [
     'tryselect/test/cram.ini',
 ]
 
 PYTHON_UNITTEST_MANIFESTS += [
+    'lint/test/python.ini',
     'tryselect/test/python.ini',
 ]