From 0dd04ffbfb97bad2dfe0530a23ab5fa8af89a26b Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 6 Jul 2021 02:43:12 -0700 Subject: 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 | 48 +++++++----- lib/spack/spack/test/cmd/flake8.py | 77 ------------------- lib/spack/spack/test/cmd/style.py | 152 +++++++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 95 deletions(-) delete mode 100644 lib/spack/spack/test/cmd/flake8.py create mode 100644 lib/spack/spack/test/cmd/style.py diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py index dec8cbad4e..0eceb4dc2d 100644 --- a/lib/spack/spack/cmd/style.py +++ b/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 :: 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): - return replacement.format( - os.path.relpath( - os.path.join(spack.paths.prefix, path.group(1)), os.getcwd() - ), - *list(path.groups()[1:]) - ) + # print results relative to current working directory + def translate(match): + return replacement.format( + cwd_relative(match.group(1)), *list(match.groups()[1:]) + ) - for line in output.split("\n"): - if not line: - continue - print(" " + re_obj.sub(cwd_relative, line)) + for line in output.split("\n"): + if not line: + continue + 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: diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py deleted file mode 100644 index c1de26a278..0000000000 --- a/lib/spack/spack/test/cmd/flake8.py +++ /dev/null @@ -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) diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py new file mode 100644 index 0000000000..ceaae5f694 --- /dev/null +++ b/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 -- cgit v1.2.3-70-g09d2