style: get close to full coverage of spack style

Previous tests of `spack style` didn't really run the tools --
they just ensure that the commands worked enough to get coverage.

This adds several real tests and ensures that we hit the corner
cases in `spack style`.  This also tests sucess as well as failure
cases.

lib/spack/spack/cmd/style.py

@@ -38,6 +38,9 @@ def grouper(iterable, n, fillvalue=None):
         yield filter(None, group)
 
 
+#: directory where spack style started, for relativizing paths
+initial_working_dir = None
+
 #: List of directories to exclude from checks.
 exclude_directories = [spack.paths.external_path]
 
@@ -183,34 +186,39 @@ def setup_parser(subparser):
     )
 
 
+def cwd_relative(path):
+    """Translate prefix-relative path to current working directory-relative."""
+    return os.path.relpath(os.path.join(spack.paths.prefix, path), initial_working_dir)
+
+
 def rewrite_and_print_output(
     output, args, re_obj=re.compile(r"^(.+):([0-9]+):"), replacement=r"{0}:{1}:"
 ):
     """rewrite ouput with <file>:<line>: format to respect path args"""
-    if args.root_relative or re_obj is None:
-        # print results relative to repo root.
-        for line in output.split("\n"):
-            print("  " + output)
-    else:
     # print results relative to current working directory
-        def cwd_relative(path):
+    def translate(match):
         return replacement.format(
-                os.path.relpath(
+            cwd_relative(match.group(1)), *list(match.groups()[1:])
-                    os.path.join(spack.paths.prefix, path.group(1)), os.getcwd()
-                ),
-                *list(path.groups()[1:])
         )
 
     for line in output.split("\n"):
         if not line:
             continue
-            print("  " + re_obj.sub(cwd_relative, line))
+        if not args.root_relative and re_obj:
+            line = re_obj.sub(translate, line)
+        print("  " + line)
 
 
 def print_style_header(file_list, args):
     tools = [tool for tool in tool_order if getattr(args, tool)]
     tty.msg("Running style checks on spack:", "selected: " + ", ".join(tools))
-    tty.msg("Modified files:", *[filename.strip() for filename in file_list])
+
+    # translate modified paths to cwd_relative if needed
+    paths = [filename.strip() for filename in file_list]
+    if not args.root_relative:
+        paths = [cwd_relative(filename) for filename in paths]
+
+    tty.msg("Modified files:", *paths)
     sys.stdout.flush()
 
 
@@ -308,6 +316,10 @@ def run_black(black_cmd, file_list, args):
 
 
 def style(parser, args):
+    # save initial working directory for relativizing paths later
+    global initial_working_dir
+    initial_working_dir = os.getcwd()
+
     file_list = args.files
     if file_list:
 

lib/spack/spack/test/cmd/flake8.py

@@ -1,77 +0,0 @@
-# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
-# Spack Project Developers. See the top-level COPYRIGHT file for details.
-#
-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-import argparse
-import os
-import pytest
-import sys
-
-from llnl.util.filesystem import FileFilter
-
-import spack.paths
-from spack.cmd.style import style, setup_parser, changed_files
-from spack.repo import Repo
-from spack.util.executable import which
-
-
-@pytest.fixture(scope="module")
-def parser():
-    """Returns the parser for the ``flake8`` command"""
-    parser = argparse.ArgumentParser()
-    setup_parser(parser)
-    return parser
-
-
-@pytest.fixture(scope="module")
-def flake8_package():
-    """Flake8 only checks files that have been modified.
-    This fixture makes a small change to the ``flake8``
-    mock package, yields the filename, then undoes the
-    change on cleanup.
-    """
-    repo = Repo(spack.paths.mock_packages_path)
-    filename = repo.filename_for_package_name("flake8")
-    package = FileFilter(filename)
-
-    # Make the change
-    package.filter("state = 'unmodified'", "state = 'modified'", string=True)
-
-    yield filename
-
-    # Undo the change
-    package.filter("state = 'modified'", "state = 'unmodified'", string=True)
-
-
-def test_changed_files(parser, flake8_package):
-    args = parser.parse_args([])
-
-    # changed_files returns file paths relative to the root
-    # directory of Spack. Convert to absolute file paths.
-    files = changed_files(args)
-    files = [os.path.join(spack.paths.prefix, path) for path in files]
-
-    # There will likely be other files that have changed
-    # when these tests are run
-    assert flake8_package in files
-
-
-# As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported
-# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html
-@pytest.mark.skipif(
-    sys.version_info[:2] <= (2, 6) or (3, 0) <= sys.version_info[:2] <= (3, 3),
-    reason="flake8 no longer supports Python 2.6 or 3.3 and older",
-)
-@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
-def test_flake8(parser, flake8_package):
-    # Only test the flake8_package that we modified
-    # Otherwise, the unit tests would fail every time
-    # the flake8 tests fail
-    args = parser.parse_args(["--no-mypy", flake8_package])
-    style(parser, args)
-    # Get even more coverage
-    args = parser.parse_args(
-        ["--no-mypy", "--output", "--root-relative", flake8_package]
-    )
-    style(parser, args)

lib/spack/spack/test/cmd/style.py

@@ -0,0 +1,152 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import os
+import shutil
+import sys
+
+import pytest
+
+from llnl.util.filesystem import FileFilter
+
+import spack.main
+import spack.paths
+import spack.repo
+from spack.cmd.style import changed_files
+from spack.util.executable import which
+
+style = spack.main.SpackCommand("style")
+
+
+@pytest.fixture(scope="function")
+def flake8_package():
+    """Style only checks files that have been modified. This fixture makes a small
+    change to the ``flake8`` mock package, yields the filename, then undoes the
+    change on cleanup.
+    """
+    repo = spack.repo.Repo(spack.paths.mock_packages_path)
+    filename = repo.filename_for_package_name("flake8")
+    tmp = filename + ".tmp"
+
+    try:
+        shutil.copy(filename, tmp)
+        package = FileFilter(filename)
+        package.filter("state = 'unmodified'", "state = 'modified'", string=True)
+        yield filename
+    finally:
+        shutil.move(tmp, filename)
+
+
+@pytest.fixture
+def flake8_package_with_errors(scope="function"):
+    """A flake8 package with errors."""
+    repo = spack.repo.Repo(spack.paths.mock_packages_path)
+    filename = repo.filename_for_package_name("flake8")
+    tmp = filename + ".tmp"
+
+    try:
+        shutil.copy(filename, tmp)
+        package = FileFilter(filename)
+        package.filter("state = 'unmodified'", "state    =    'modified'", string=True)
+        yield filename
+    finally:
+        shutil.move(tmp, filename)
+
+
+def test_changed_files(flake8_package):
+    # changed_files returns file paths relative to the root
+    # directory of Spack. Convert to absolute file paths.
+    files = [os.path.join(spack.paths.prefix, path) for path in changed_files()]
+
+    # There will likely be other files that have changed
+    # when these tests are run
+    assert flake8_package in files
+
+
+def test_changed_files_all_files(flake8_package):
+    # it's hard to guarantee "all files", so do some sanity checks.
+    files = set([
+        os.path.join(spack.paths.prefix, path)
+        for path in changed_files(all_files=True)
+    ])
+
+    # spack has a lot of files -- check that we're in the right ballpark
+    assert len(files) > 6000
+
+    # a builtin package
+    zlib = spack.repo.path.get_pkg_class("zlib")
+    assert zlib.module.__file__ in files
+
+    # a core spack file
+    assert os.path.join(spack.paths.module_path, "spec.py") in files
+
+    # a mock package
+    assert flake8_package in files
+
+    # this test
+    assert __file__ in files
+
+    # ensure externals are excluded
+    assert not any(f.startswith(spack.paths.external_path) for f in files)
+
+
+# As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported
+# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html
+skip_old_python = pytest.mark.skipif(
+    sys.version_info[:2] <= (2, 6) or (3, 0) <= sys.version_info[:2] <= (3, 3),
+    reason="flake8 no longer supports Python 2.6 or 3.3 and older",
+)
+
+
+@skip_old_python
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+def test_style(flake8_package, tmpdir):
+    root_relative = os.path.relpath(flake8_package, spack.paths.prefix)
+
+    # use a working directory to test cwd-relative paths, as tests run in
+    # the spack prefix by default
+    with tmpdir.as_cwd():
+        relative = os.path.relpath(flake8_package)
+
+        # no args
+        output = style()
+        assert relative in output
+        assert "spack style checks were clean" in output
+
+        # one specific arg
+        output = style(flake8_package)
+        assert relative in output
+        assert "spack style checks were clean" in output
+
+        # specific file that isn't changed
+        output = style(__file__)
+        assert relative not in output
+        assert __file__ in output
+        assert "spack style checks were clean" in output
+
+    # root-relative paths
+    output = style("--root-relative", flake8_package)
+    assert root_relative in output
+    assert "spack style checks were clean" in output
+
+
+@skip_old_python
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+def test_style_with_errors(flake8_package_with_errors):
+    root_relative = os.path.relpath(flake8_package_with_errors, spack.paths.prefix)
+    output = style("--root-relative", flake8_package_with_errors, fail_on_error=False)
+    assert root_relative in output
+    assert style.returncode == 1
+    assert "spack style found errors" in output
+
+
+@skip_old_python
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+def test_style_with_black(flake8_package_with_errors):
+    output = style("--black", flake8_package_with_errors, fail_on_error=False)
+    assert "black found errors" in output
+    assert style.returncode == 1
+    assert "spack style found errors" in output