Fix tests modifying package.py files (#49093)

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,9 +245,7 @@ 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
@@ -273,12 +264,15 @@ def add_versions_to_package(pkg: PackageBase, version_lines: str, is_batch: bool
             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

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")
+"""
+    )

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()
+    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 spack.repo.PATH.last_mtime() == latest_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):