From 97d66b637ffa0605662ff541c5833932016c94e0 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 18 Feb 2025 13:28:07 +0100 Subject: [PATCH] Fix tests modifying package.py files (#49093) --- lib/spack/spack/cmd/checksum.py | 70 +++++++++++++-------------- lib/spack/spack/test/cmd/checksum.py | 72 ++++++++++++++++++++++++---- lib/spack/spack/test/repo.py | 19 ++++++-- 3 files changed, 109 insertions(+), 52 deletions(-) diff --git a/lib/spack/spack/cmd/checksum.py b/lib/spack/spack/cmd/checksum.py index b62f901503e..47e54628828 100644 --- a/lib/spack/spack/cmd/checksum.py +++ b/lib/spack/spack/cmd/checksum.py @@ -4,7 +4,7 @@ import re import sys -from typing import Dict, Optional +from typing import Dict, Optional, Tuple import llnl.string import llnl.util.lang @@ -181,7 +181,11 @@ def checksum(parser, args): print() if args.add_to_package: - add_versions_to_package(pkg, version_lines, args.batch) + path = spack.repo.PATH.filename_for_package_name(pkg.name) + num_versions_added = add_versions_to_pkg(path, version_lines) + tty.msg(f"Added {num_versions_added} new versions to {pkg.name} in {path}") + if not args.batch and sys.stdin.isatty(): + editor(path) def print_checksum_status(pkg: PackageBase, version_hashes: dict): @@ -227,20 +231,9 @@ def print_checksum_status(pkg: PackageBase, version_hashes: dict): tty.die("Invalid checksums found.") -def add_versions_to_package(pkg: PackageBase, version_lines: str, is_batch: bool): - """ - Add checksumed versions to a package's instructions and open a user's - editor so they may double check the work of the function. - - Args: - pkg (spack.package_base.PackageBase): A package class for a given package in Spack. - version_lines (str): A string of rendered version lines. - - """ - # Get filename and path for package - filename = spack.repo.PATH.filename_for_package_name(pkg.name) +def _update_version_statements(package_src: str, version_lines: str) -> Tuple[int, str]: + """Returns a tuple of number of versions added and the package's modified contents.""" num_versions_added = 0 - version_statement_re = re.compile(r"([\t ]+version\([^\)]*\))") version_re = re.compile(r'[\t ]+version\(\s*"([^"]+)"[^\)]*\)') @@ -252,33 +245,34 @@ def add_versions_to_package(pkg: PackageBase, version_lines: str, is_batch: bool if match: new_versions.append((Version(match.group(1)), ver_line)) - with open(filename, "r+", encoding="utf-8") as f: - contents = f.read() - split_contents = version_statement_re.split(contents) + split_contents = version_statement_re.split(package_src) - for i, subsection in enumerate(split_contents): - # If there are no more versions to add we should exit - if len(new_versions) <= 0: - break + for i, subsection in enumerate(split_contents): + # If there are no more versions to add we should exit + if len(new_versions) <= 0: + break - # Check if the section contains a version - contents_version = version_re.match(subsection) - if contents_version is not None: - parsed_version = Version(contents_version.group(1)) + # Check if the section contains a version + contents_version = version_re.match(subsection) + if contents_version is not None: + parsed_version = Version(contents_version.group(1)) - if parsed_version < new_versions[0][0]: - split_contents[i:i] = [new_versions.pop(0)[1], " # FIXME", "\n"] - num_versions_added += 1 + if parsed_version < new_versions[0][0]: + split_contents[i:i] = [new_versions.pop(0)[1], " # FIXME", "\n"] + num_versions_added += 1 - elif parsed_version == new_versions[0][0]: - new_versions.pop(0) + elif parsed_version == new_versions[0][0]: + new_versions.pop(0) - # Seek back to the start of the file so we can rewrite the file contents. - f.seek(0) - f.writelines("".join(split_contents)) + return num_versions_added, "".join(split_contents) - tty.msg(f"Added {num_versions_added} new versions to {pkg.name}") - tty.msg(f"Open {filename} to review the additions.") - if sys.stdout.isatty() and not is_batch: - editor(filename) +def add_versions_to_pkg(path: str, version_lines: str) -> int: + """Add new versions to a package.py file. Returns the number of versions added.""" + with open(path, "r", encoding="utf-8") as f: + package_src = f.read() + num_versions_added, package_src = _update_version_statements(package_src, version_lines) + if num_versions_added > 0: + with open(path, "w", encoding="utf-8") as f: + f.write(package_src) + return num_versions_added diff --git a/lib/spack/spack/test/cmd/checksum.py b/lib/spack/spack/test/cmd/checksum.py index 6ea333f286b..4405695ba19 100644 --- a/lib/spack/spack/test/cmd/checksum.py +++ b/lib/spack/spack/test/cmd/checksum.py @@ -3,6 +3,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import argparse +import pathlib import pytest @@ -37,6 +38,10 @@ def get_checksums_for_versions(url_by_version, package_name, **kwargs): def url_exists(url, curl=None): return True + def add_versions_to_pkg(pkg, version_lines, open_in_editor): + raise AssertionError("Should not be called") + + monkeypatch.setattr(spack.cmd.checksum, "add_versions_to_pkg", add_versions_to_pkg) monkeypatch.setattr( spack.package_base.PackageBase, "fetch_remote_versions", fetch_remote_versions ) @@ -88,7 +93,6 @@ def test_checksum_args(arguments, expected): (["--batch", "preferred-test"], "version of preferred-test"), (["--latest", "preferred-test"], "Found 1 version"), (["--preferred", "preferred-test"], "Found 1 version"), - (["--add-to-package", "preferred-test"], "Added 0 new versions to"), (["--verify", "preferred-test"], "Verified 1 of 1"), (["--verify", "zlib", "1.2.13"], "1.2.13 [-] No previous checksum"), ], @@ -271,34 +275,35 @@ def test_checksum_interactive_unrecognized_command(): assert interactive_version_filter(v.copy(), input=input) == v -def test_checksum_versions(mock_packages, can_fetch_versions): +def test_checksum_versions(mock_packages, can_fetch_versions, monkeypatch): pkg_cls = spack.repo.PATH.get_pkg_class("zlib") versions = [str(v) for v in pkg_cls.versions] output = spack_checksum("zlib", *versions) assert "Found 3 versions" in output assert "version(" in output - output = spack_checksum("--add-to-package", "zlib", *versions) - assert "Found 3 versions" in output - assert "Added 0 new versions to" in output -def test_checksum_missing_version(mock_packages, cannot_fetch_versions): +def test_checksum_missing_version(mock_packages, cannot_fetch_versions, monkeypatch): + def add_versions_to_pkg(pkg, version_lines, open_in_editor): + raise AssertionError("Should not be called") + + monkeypatch.setattr(spack.cmd.checksum, "add_versions_to_pkg", add_versions_to_pkg) output = spack_checksum("preferred-test", "99.99.99", fail_on_error=False) assert "Could not find any remote versions" in output output = spack_checksum("--add-to-package", "preferred-test", "99.99.99", fail_on_error=False) assert "Could not find any remote versions" in output - assert "Added 1 new versions to" not in output def test_checksum_deprecated_version(mock_packages, can_fetch_versions): + def add_versions_to_pkg(pkg, version_lines, open_in_editor): + raise AssertionError("Should not be called") + output = spack_checksum("deprecated-versions", "1.1.0", fail_on_error=False) assert "Version 1.1.0 is deprecated" in output output = spack_checksum( "--add-to-package", "deprecated-versions", "1.1.0", fail_on_error=False ) assert "Version 1.1.0 is deprecated" in output - # TODO alecbcs: broken assertion. - # assert "Added 0 new versions to" not in output def test_checksum_url(mock_packages, config): @@ -337,3 +342,52 @@ def test_checksum_manual_download_fails(mock_packages, monkeypatch): monkeypatch.setattr(spack.package_base.PackageBase, "download_instr", error) with pytest.raises(ManualDownloadRequiredError, match=error): spack_checksum(name, *versions) + + +def test_upate_package_contents(tmp_path: pathlib.Path): + """Test that the package.py file is updated with the new versions.""" + pkg_path = tmp_path / "package.py" + pkg_path.write_text( + """\ +from spack.package import * + +class Zlib(Package): + homepage = "http://zlib.net" + url = "http://zlib.net/fossils/zlib-1.2.11.tar.gz" + + version("1.2.11", sha256="c3e5e9fdd5004dcb542feda5ee4f0ff0744628baf8ed2dd5d66f8ca1197cb1a1") + version("1.2.8", sha256="36658cb768a54c1d4dec43c3116c27ed893e88b02ecfcb44f2166f9c0b7f2a0d") + version("1.2.3", sha256="1795c7d067a43174113fdf03447532f373e1c6c57c08d61d9e4e9be5e244b05e") + variant("pic", default=True, description="test") + + def install(self, spec, prefix): + make("install") +""" + ) + version_lines = """\ + version("1.2.13", sha256="abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890") + version("1.2.5", sha256="abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890") + version("1.2.3", sha256="1795c7d067a43174113fdf03447532f373e1c6c57c08d61d9e4e9be5e244b05e") +""" + # two new versions are added + assert spack.cmd.checksum.add_versions_to_pkg(str(pkg_path), version_lines) == 2 + assert ( + pkg_path.read_text() + == """\ +from spack.package import * + +class Zlib(Package): + homepage = "http://zlib.net" + url = "http://zlib.net/fossils/zlib-1.2.11.tar.gz" + + version("1.2.13", sha256="abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890") # FIXME + version("1.2.11", sha256="c3e5e9fdd5004dcb542feda5ee4f0ff0744628baf8ed2dd5d66f8ca1197cb1a1") + version("1.2.8", sha256="36658cb768a54c1d4dec43c3116c27ed893e88b02ecfcb44f2166f9c0b7f2a0d") + version("1.2.5", sha256="abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890") # FIXME + version("1.2.3", sha256="1795c7d067a43174113fdf03447532f373e1c6c57c08d61d9e4e9be5e244b05e") + variant("pic", default=True, description="test") + + def install(self, spec, prefix): + make("install") +""" + ) diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py index 4e07d5eb154..40e10dcfc79 100644 --- a/lib/spack/spack/test/repo.py +++ b/lib/spack/spack/test/repo.py @@ -65,12 +65,21 @@ def test_repo_unknown_pkg(mutable_mock_repo): mutable_mock_repo.get_pkg_class("builtin.mock.nonexistentpackage") -@pytest.mark.not_on_windows("mtime granularity issues on windows") def test_repo_last_mtime(mock_packages): - latest_mtime = max( - os.path.getmtime(p.module.__file__) for p in spack.repo.PATH.all_package_classes() - ) - assert spack.repo.PATH.last_mtime() == latest_mtime + mtime_with_package_py = [ + (os.path.getmtime(p.module.__file__), p.module.__file__) + for p in spack.repo.PATH.all_package_classes() + ] + repo_mtime = spack.repo.PATH.last_mtime() + max_mtime, max_file = max(mtime_with_package_py) + if max_mtime > repo_mtime: + modified_after = "\n ".join( + f"{path} ({mtime})" for mtime, path in mtime_with_package_py if mtime > repo_mtime + ) + assert ( + max_mtime <= repo_mtime + ), f"the following files were modified while running tests:\n {modified_after}" + assert max_mtime == repo_mtime, f"last_mtime incorrect for {max_file}" def test_repo_invisibles(mutable_mock_repo, extra_repo):