Bugfix: restore ability to run virtual tests by tldahlgren · Pull Request #35380 · spack/spack

tldahlgren · 2023-02-08T03:16:40Z

Fixes #35519 (at least should)

This PR restores the ability to run stand-alone tests on virtual provider packages, including the ability to "inherit" tests from the virtual packages.

Also fixes an issue reported by @jfinney10 on slack:

$ spack -d test run
[...snip...]
  File "$spack/lib/spack/spack/repo.py", line 1153, in dump_provenance
    spec.package.patches.values()):
  File "$spack/lib/spack/spack/spec.py", line 1520, in package
    assert self.concrete, "Spec.package can only be called on concrete specs"
AssertionError: Spec.package can only be called on concrete specs

TODO

Stand-alone testing
Ensure pulled all of the relevant code from (local) Stand-alone testing: Make recipe support and processing spack-/pytest-like #34236

tldahlgren · 2023-02-10T23:46:01Z

Not clear why the libdwarf source stage directory was missing for the macOS check (https://github.com/spack/spack/actions/runs/4140755709/jobs/7159751165).

tldahlgren · 2023-02-16T17:24:26Z

@tgamblin @alalazo Looks like all of the checks are now passing. This PR restores spack test's running of stand-alone tests defined in virtual packages while removing errors related to the fact those specs aren't concretized.

tldahlgren · 2023-02-17T01:05:58Z

Grrr. I'm seeing positive coverage for all of the affected files and don't seem to be able to force re-running the check.

alalazo

Just a few initial comments and questions. There's a conflict to be solved.

alalazo · 2023-02-21T11:16:04Z

lib/spack/spack/install_test.py

+        cls = spec.package.__class__
+    except AssertionError:
+        try:
+            cls = spec.package_class


Are there cases where these two might be different?

I don't think so. I was thinking the majority of time the Spec is concrete and readily retrieved from spec.package.

When it isn't, such as for virtual packages, then the call spec.package_class that makes multiple calls (e.g., spack.repo.path.get_pkg_class(), importing the module, and more) is performed.

alalazo · 2023-02-21T11:19:37Z

lib/spack/spack/install_test.py

+    if isinstance(spec_or_pkg, spack.spec.Spec):
+        spec = spec_or_pkg
+        cls = package_class(spec)
+        if not cls:
+            tty.debug("Skipping {0}: no package class found".format(spec.name))
+            return []
+    elif isinstance(spec_or_pkg, spack.package_base.PackageBase):
+        pkg = spec_or_pkg
+        cls = pkg.__class__
+        if add_virtuals:
+            methods = []
+            v_names = virtuals(pkg)
+            test_specs = [pkg.spec] + [spack.spec.Spec(v_name) for v_name in sorted(v_names)]
+            for spec in test_specs:
+                methods.extend(test_functions(spec, names=names))
+            return methods
+
+    elif inspect.isclass(spec_or_pkg):
+        cls = spec_or_pkg
+
+    else:
+        raise ValueError("Cannot retrieve test methods for {0}".format(spec_or_pkg))


Is it worth extracting this part into its own function? If I understand correctly, this gets as input either a spec object or a package object or a package class and returns a package class.

Good point.

That needed another pass over the logic anyway.

Extracted into its own function.

alalazo · 2023-02-21T11:21:59Z

lib/spack/spack/package_base.py

+def package_directory(cls):
+    """Returns the path to the package class directory."""
+    return os.path.abspath(os.path.dirname(cls.module.__file__))


Why was extracting this function necessary? I think package_dir would work on package classes too.

Can't recall. Let me look into it.

tldahlgren · 2023-02-22T20:18:40Z

I need to update my local black (or some setting) because it is not reporting the same style issues reported here. There was a style fix reported yesterday that I made but it seems like a new layout was required today.

adamjstewart · 2023-02-22T22:09:34Z

black 23 is more strict than black 22, I recently updated CI to use black 23 and updated all existing code.

alalazo

I have test driven the PR and was able to:

$ spack -v test mpich
[ ... ]
$ spack test results --logs 4lb3fp6vigamkxjn56bxkdvvxtpwmkep
==> Results for test suite '4lb3fp6vigamkxjn56bxkdvvxtpwmkep':
==> test specs:
==>   mpich-4.1-yva65xh PASSED
==> Testing package mpich-4.1-yva65xh
==> [2023-02-27-18:11:01.638157] Installing /home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/.spack/test to /home/culpo/.spack/test/4lb3fp6vigamkxjn56bxkdvvxtpwmkep/mpich-4.1-yva65xh/cache/mpich
==> [2023-02-27-18:11:01.902862] test: generate finalized file
==> [2023-02-27-18:11:01.903060] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpicc' '/home/culpo/.spack/test/4lb3fp6vigamkxjn56bxkdvvxtpwmkep/mpich-4.1-yva65xh/cache/mpich/test/mpi/init/finalized.c' '-Wall' '-g' '-o' 'finalized'
PASSED
==> [2023-02-27-18:11:02.020309] test: run finalized example
==> [2023-02-27-18:11:02.020624] './finalized'
 No Errors
PASSED
==> [2023-02-27-18:11:03.382000] test: generate sendrecv file
==> [2023-02-27-18:11:03.382351] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpicc' '/home/culpo/.spack/test/4lb3fp6vigamkxjn56bxkdvvxtpwmkep/mpich-4.1-yva65xh/cache/mpich/test/mpi/basic/sendrecv.c' '-Wall' '-g' '-o' 'sendrecv'
PASSED
==> [2023-02-27-18:11:03.530278] test: run sendrecv example
==> [2023-02-27-18:11:03.530589] './sendrecv'
Simple Send/Recv test.
Two processes needed.
PASSED
==> [2023-02-27-18:11:04.027395] test: mpicc: expect command status in [0]
==> [2023-02-27-18:11:04.027744] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpicc' '-o' 'mpi_hello_c' '/home/culpo/.spack/test/4lb3fp6vigamkxjn56bxkdvvxtpwmkep/mpich-4.1-yva65xh/data/mpi/mpi_hello.c'
PASSED
==> [2023-02-27-18:11:04.154281] test: mpirun: expect command status in [0]
==> [2023-02-27-18:11:04.154603] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpirun' '-np' '1' 'mpi_hello_c'
Hello world! From rank 0 of 1
PASSED
==> [2023-02-27-18:11:04.647879] test: mpif90: expect command status in [0]
==> [2023-02-27-18:11:04.648273] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpif90' '-o' 'mpi_hello_f' '/home/culpo/.spack/test/4lb3fp6vigamkxjn56bxkdvvxtpwmkep/mpich-4.1-yva65xh/data/mpi/mpi_hello.f'
PASSED
==> [2023-02-27-18:11:04.769080] test: mpirun: expect command status in [0]
==> [2023-02-27-18:11:04.769346] '/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mpich-4.1-yva65xhfzl7xertquaahwt3rg5rwpr6q/bin/mpirun' '-np' '1' 'mpi_hello_f'
 Hello world! From rank           0 of            1
PASSED
==> [2023-02-27-18:11:05.232167] Completed testing

============================= 1 passed of 1 specs ==============================

I left a few minor comments on the code, but from a user perspective ✔️

alalazo · 2023-02-27T16:56:24Z

lib/spack/spack/install_test.py

+def package_class(spec_or_pkg):
+    """Return the package class for the spec or package.
+
+    Args:
+        spec_or_pkg (spack.spec.Spec or spack.package_base.PackageBase): spec or
+            package (class)
+
+    Raises: ValueError: If not given a spec, package, or class
+    """


We can add type annotation like this:

diff --git a/lib/spack/spack/install_test.py b/lib/spack/spack/install_test.py index 416d502acc..ddbc717195 100644 --- a/lib/spack/spack/install_test.py +++ b/lib/spack/spack/install_test.py @@ -8,6 +8,7 @@ import os import re import shutil +from typing import Optional, Type, Union import llnl.util.filesystem as fs import llnl.util.tty as tty @@ -111,14 +112,19 @@ def get_test_suite(name): return names[0] -def package_class(spec_or_pkg): +PkgClass = Type["spack.package_base.PackageBase"] + + +def package_class( + spec_or_pkg: "Union[Spec, spack.package_base.PackageBase, PkgClass]", +) -> Optional[PkgClass]: """Return the package class for the spec or package. Args: - spec_or_pkg (spack.spec.Spec or spack.package_base.PackageBase): spec or - package (class) + spec_or_pkg: spec or package (class) - Raises: ValueError: If not given a spec, package, or class + Raises: + ValueError: If not given a spec, package, or class """ if isinstance(spec_or_pkg, spack.spec.Spec): spec = spec_or_pkg

so that mypy will check a bit more of our code base.

I need to get into the habit of adding the annotations.

alalazo · 2023-02-27T16:58:16Z

lib/spack/spack/install_test.py

+    elif isinstance(spec_or_pkg, spack.package_base.PackageBase):
+        cls = spec_or_pkg.__class__
+
+    elif inspect.isclass(spec_or_pkg):


Would this:

Suggested change

elif inspect.isclass(spec_or_pkg):

elif issubclass(spec_or_pkg, spack.package_base.PackageBase):

work too?

alalazo · 2023-02-27T17:02:00Z

lib/spack/spack/install_test.py

+                tty.debug("{0}: Cannot retrieve class from spec: {1}".format(spec.name, str(e)))
+                return None
+
+    elif isinstance(spec_or_pkg, spack.package_base.PackageBase):


I think we never import spack.package_base in this Python module (and importing it at the module level will probably lead to circular dependency issues).

alalazo · 2023-02-27T17:28:12Z

lib/spack/spack/install_test.py

+    if add_virtuals and isinstance(spec_or_pkg, spack.package_base.PackageBase):
+        pkg = spec_or_pkg
+        v_names = virtuals(pkg)
+        test_specs = [pkg.spec] + [spack.spec.Spec(v_name) for v_name in sorted(v_names)]
+        methods = []
+        for spec in test_specs:
+            methods.extend(test_functions(spec, add_virtuals=False, names=names))
+        return methods


(Minor) If I read this one correctly here we are expanding virtual packages and recursing with the add_virtuals argument flipped, so to fall through and hit the code below. Would it be easy to avoid recursion and separate:

Expansion of virtual specs

Extraction of tests functions from list of specs

? If it seems complicated, let's leave it for another PR.

alalazo · 2023-02-27T17:29:27Z

lib/spack/spack/install_test.py

+        return methods
+
+    cls = package_class(spec_or_pkg)
+    if not cls:


(Minor) I think this might be slightly safer for future refactoring:

Suggested change

if not cls:

if cls is None:

alalazo · 2023-02-27T17:31:37Z

lib/spack/spack/install_test.py

+        source = (inspect.getsource(test_fn)).splitlines()[1:]
+        lines = (ln.strip() for ln in source)
+        statements = [ln for ln in lines if not ln.startswith("#")]
+        empty = len(statements) > 0 and statements[0] == "pass"


Curious why we need to inspect sources. Is it to avoid printing to screen? Or something else?

Ppl have expressed concerns about distinguishing between having a test method and having one that actually does something so inspecting to make sure non-empty (non-pass) helps make that distinction.

alalazo · 2023-02-27T17:38:18Z

lib/spack/spack/repo.py

+        except AssertionError as e:
+            # virtual specs won't have patches to install
+            tty.debug(
+                "Could not copy patch files for virtual package {0}: {1}".format(spec.name, str(e))
+            )


Can we do this more explicitly with a conditional upfront? This is for 2 main reasons:

We might catch another assertion in the code

Assertion might be turned off from the Python interpreter

I was thinking I'd be more pythonic 😉 but I can certainly check in advance.

Co-authored-by: Massimiliano Culpo <[email protected]>

tldahlgren · 2023-03-24T00:25:28Z

Closing this PR since the changes originated in #34236, which has refactoring that causes redundant work iterating on this and the other PR.

spackbot-app bot added commands core PR affects Spack core functionality tests General test capability(ies) labels Feb 8, 2023

tldahlgren force-pushed the bugfix_run_virtual_tests branch from 5d6a2c8 to c2ad1d9 Compare February 8, 2023 21:27

tldahlgren marked this pull request as ready for review February 8, 2023 21:48

tldahlgren force-pushed the bugfix_run_virtual_tests branch from c2ad1d9 to ec003c3 Compare February 8, 2023 23:29

tldahlgren force-pushed the bugfix_run_virtual_tests branch from 54e8f03 to 71bcdeb Compare February 10, 2023 23:50

tldahlgren requested review from alalazo, scheibelp and tgamblin February 13, 2023 19:52

tldahlgren mentioned this pull request Feb 16, 2023

spack test: Spec.package can only be called on concrete specs #35519

Closed

3 tasks

tldahlgren force-pushed the bugfix_run_virtual_tests branch from 20fc891 to 7604e2b Compare February 17, 2023 01:06

alalazo reviewed Feb 21, 2023

View reviewed changes

alalazo self-assigned this Feb 21, 2023

tldahlgren added 5 commits February 21, 2023 18:19

Bugfix: restore ability to run virtual tests

e354a7f

Add more unit tests

170ae1c

Removed has_test_method and tests; added more unit tests

f4209e3

Fix handling of package-less virtuals

04522ab

Addressed feedback: fix refactorings

18a594e

tldahlgren force-pushed the bugfix_run_virtual_tests branch from 7604e2b to 18a594e Compare February 22, 2023 02:34

Style fixes: tightened imports and code (not reported by my black)

0328984

tldahlgren force-pushed the bugfix_run_virtual_tests branch from 3364372 to 0328984 Compare February 22, 2023 20:16

Fix name variable in debug message

6c33d9c

alalazo requested changes Feb 27, 2023

View reviewed changes

Type annotation changes and checks; unit test updates

47faaca

Co-authored-by: Massimiliano Culpo <[email protected]>

tldahlgren closed this Mar 24, 2023

tldahlgren mentioned this pull request Apr 7, 2023

Stand-alone testing: Make recipe support and processing spack-/pytest-like #34236

Merged

tldahlgren deleted the bugfix_run_virtual_tests branch August 27, 2024 01:08

	elif inspect.isclass(spec_or_pkg):
	elif issubclass(spec_or_pkg, spack.package_base.PackageBase):

Conversation

tldahlgren commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tldahlgren commented Feb 10, 2023

Uh oh!

tldahlgren commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tldahlgren commented Feb 17, 2023

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tldahlgren commented Feb 22, 2023

Uh oh!

adamjstewart commented Feb 22, 2023

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tldahlgren commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tldahlgren commented Feb 8, 2023 •

edited

Loading

tldahlgren commented Feb 16, 2023 •

edited

Loading