Bug 1436802 - [lint] Add some tests for the flake8 linter integration r=andi
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Thu, 22 Mar 2018 17:24:15 -0400
changeset 464134 ad34ac3d45a62dff961bd247e36ab209af2cc0e5
parent 464133 430a1c231025cee13e68fccd07883dcceb741aad
child 464135 26240d2027da0d4d010f7009ce4509e2c6969da2
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersandi
bugs1436802, 1277851
milestone61.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 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
.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',
 ]