RepoSplit/tests: flag/update more unit tests away from relying on the builtin repository by tldahlgren · Pull Request #48930 · spack/spack

tldahlgren · 2025-02-07T01:46:00Z

This PR flags code that will need to be revisited when the package repository is actually split off from the core. All but about four to six unit tests relying on the builtin repository have been converted to use builtin.mock. This was accomplished initially by renaming builtin so it would not be found when spack unit-test was run and, later, changing test/data/config/repos.yaml to use the mock repository to pick up more unit tests.

In my testing on linux, it appeared that only four of the five test_load_json_specfiles checks won't work if the builtin repository is "removed". These tests rely on checks relying on version-specific JSON files based on the builtin HDF5 package. A mock version of that package wasn't added until v0.16. Instead of continuing to address these last four failing tests, this PR added a TODO note indicating the need to address the tests in a subsequent PR.

TODO

Determine why test/compilers/libraries.py succeeds on its own but tests fail for lack of gcc when run all(?) unit tests (i.e., What are the sources of test setup overlap?)
Investigate/resolve other new post-compilers-as-nodes test failures
Determine if there is a better solution to repository package symlinks from mock and .test repos to builtin for selected packages

… mock_packages

…tests

alalazo

I started reviewing this PR, and left a few comments. In general, I think there are many parts that can be extracted into their own small PR, and merged.

alalazo · 2025-05-03T03:08:26Z

lib/spack/spack/test/compilers/libraries.py

+@pytest.fixture
+def mock_gcc(mutable_config):
+    # Data for a compiler if there are none available
+    _gcc_compiler_definition = {
+        "compiler": {
+            "spec": "[email protected]",
+            "paths": {
+                "cc": "/usr/bin/gcc-9",
+                "cxx": "/usr/bin/g++-9",
+                "f77": "/usr/bin/gfortran-9",
+                "fc": "/usr/bin/gfortran-9",
+            },
+            "flags": {},
+            "operating_system": "ubuntu18.04",
+            "target": "x86_64",
+            "modules": [],
+            "environment": {},
+            "extra_rpaths": [],
+        }
+    }
+
+    compilers_before = spack.config.get("compilers", [])
+    supported_compilers = spack.compilers.config.supported_compilers()
+
+    # Ensure have at least one gcc compiler
+    with spack.config.override("compilers", [_gcc_compiler_definition]):
+        compilers = spack.compilers.config.all_compilers()
+        assert compilers, "No compilers available"
+
+        compilers.sort(key=lambda x: (x.name == "gcc", x.version))
+        # Deepcopy is used to avoid more boilerplate when changing the "extra_attributes"
+        return copy.deepcopy(compilers[-1])


Why did we change the mock here to use the deprecated compiler: configuration?

I started that from something else and neglected to update it. Thanks for pointing it out!

alalazo · 2025-05-03T03:11:52Z

lib/spack/spack/test/compilers/libraries.py

    @pytest.mark.not_on_windows("Not supported on Windows")
-    def test_compile_dummy_c_source(self, mock_gcc, monkeypatch, language, flagname):
+    def test_compile_dummy_c_source(
+        self, mock_packages, mock_gcc, monkeypatch, language, flagname


Instead of adding mock_packages everywhere, did we consider adding it only once at the class level:

@pytest.mark.usefixtures("mock_packages") class TestCompilerPropertyDetector:

?

IIRC Unmarked unit tests failed. I'll revisit.

alalazo · 2025-05-03T03:17:00Z

lib/spack/spack/bootstrap/clingo.py

        return self._external_spec(result)

    def _external_spec(self, initial_spec) -> "spack.spec.Spec":
+        # TODO/RepoSplit: Convert to namespace of new or bootstrap pkg repo


Why would we need to convert? I assume we won't change the "builtin" namespace? Or maybe I'm reading the comment wrong?

I added the comment as a placeholder for a quick search that pre-dates Harmen's work.

alalazo · 2025-05-03T03:18:39Z

lib/spack/spack/repo.py

    For instance:

-        python_package_for_repo('builtin') == 'spack.pkg.builtin'
+        python_package_for_repo('builtin.mock') == 'spack.pkg.builtin.mock'


We don't really need to change the example in the docstring, do we?

Fair enough. Things were still "up in the air" at the time I made the change. Plus I wasn't sure how we wanted or were going to reference what we currently refer to as builtin.

lib/spack/spack/test/bootstrap.py

lib/spack/spack/test/cmd/pkg.py

alalazo · 2025-05-03T03:57:50Z

lib/spack/spack/test/cmd/providers.py

And another...

alalazo · 2025-05-03T04:01:14Z

lib/spack/spack/test/compilers/conversion.py


 @pytest.fixture()
-def mock_compiler(mock_executable):
+def mock_gcc_yaml(mock_executable):


Let's continue to call it mock_compiler, or if we want to be more specific mock_legacy_compiler or legacy_compiler_yaml. The compiler part refers to the deprecated compiler: section of the configuration that these tests are about.

Switching/switched to legacy_compiler_yaml since the change was motivated by my I adding a different mock_compilers to conftest.py.

lib/spack/spack/test/concretization/core.py

alalazo · 2025-05-03T04:06:01Z

var/spack/repos/builtin.mock/packages/cray-mpich/package.py

+from spack.package import *
+
+
+class CrayMpich(Package):


Why was it necessary to add this package? Not clear from the description? Is there some test that strictly depend on it?

Yes, the three starting test/package_class.py::test_possible_dependencies.

Going over the commits, it was initially needed for test/cray_manifest.py tests.

This does not appear to be needed anymore within the contexts of running the test modules test/cmd/dependencies.py, test/cray_manifest.py, and test_package_class.py independently when the builtin repository isn't available.

Note: The exception to this finding and others previously flagged is that, in general, there are still dependencies on compiler-wrappers and or gcc-runtime being symbolically linked to their builtin repository packages.

tldahlgren · 2025-05-05T17:04:33Z

@alalazo Thank you for the detailed feedback!

tldahlgren · 2025-05-14T00:06:55Z

Closing since nearly all independent changes have been moved to other pull requests.

spackbot-app bot added core PR affects Spack core functionality new-version tests General test capability(ies) labels Feb 7, 2025

tldahlgren changed the title ~~[WIP] Reposplit: flag/update more unit tests~~ [WIP] RepoSplit/tests: flag/update more unit tests away from relying on the builtin repository Feb 7, 2025

tldahlgren mentioned this pull request Feb 7, 2025

Discussion: Move built-in packages to a separate repository #47480

Closed

15 tasks

tldahlgren force-pushed the reposplit_more_tests branch from 7f078d0 to 293e4dc Compare March 7, 2025 01:59

spackbot-app bot added the commands label Mar 7, 2025

tldahlgren force-pushed the reposplit_more_tests branch from 293e4dc to 8f2fa0c Compare March 7, 2025 17:15

tldahlgren marked this pull request as ready for review March 7, 2025 18:33

spackbot-app bot added the virtual-dependencies label Mar 19, 2025

tldahlgren force-pushed the reposplit_more_tests branch 3 times, most recently from 33f161a to bd1b26e Compare March 25, 2025 01:41

tldahlgren mentioned this pull request Mar 25, 2025

RepoSplit/core: flag core uses of builtin #48927

Closed

tldahlgren changed the title ~~[WIP] RepoSplit/tests: flag/update more unit tests away from relying on the builtin repository~~ RepoSplit/tests: flag/update more unit tests away from relying on the builtin repository Mar 25, 2025

tldahlgren requested review from alalazo and haampie March 25, 2025 23:35

tldahlgren force-pushed the reposplit_more_tests branch from c8e65ea to 6031143 Compare April 1, 2025 00:47

tldahlgren added this to the v1.0.0 milestone Apr 2, 2025

tldahlgren force-pushed the reposplit_more_tests branch from 6031143 to a72eb64 Compare April 4, 2025 22:17

tldahlgren marked this pull request as draft April 11, 2025 00:37

tldahlgren force-pushed the reposplit_more_tests branch from 11e947c to b14d2b2 Compare April 11, 2025 01:02

spackbot-app bot added the dependencies label Apr 11, 2025

tldahlgren force-pushed the reposplit_more_tests branch from 54e4a19 to ace6037 Compare April 28, 2025 16:42

spackbot-app bot added the stand-alone-tests Stand-alone (or smoke) tests for installed packages label Apr 28, 2025

tldahlgren added 5 commits May 2, 2025 18:03

RepoSplit/test: flat test_config_audits, add mock openssl

268c514

RepoSplit/test: flag use of builtin namespace for consideration

d6d32cd

RepoSplit/tests: override repo in single file scope tests

dc891d1

RepoSplit/tests: test_optimization_flags for mixed toolchain requires…

eeff5e2

… mock_packages

RepoSplit/tests: test/cmd/versions.py needs mock_packages for 7 unit …

c355765

…tests

spackbot-app bot added the new-variant label May 3, 2025

tldahlgren marked this pull request as ready for review May 3, 2025 01:05

alalazo reviewed May 3, 2025

View reviewed changes

tldahlgren assigned alalazo May 5, 2025

tldahlgren closed this May 14, 2025

tldahlgren deleted the reposplit_more_tests branch June 2, 2025 23:48

Conversation

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

Uh oh!

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!

tldahlgren May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tldahlgren commented May 5, 2025

Uh oh!

tldahlgren commented May 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tldahlgren commented Feb 7, 2025 •

edited

Loading

tldahlgren May 5, 2025 •

edited

Loading

alalazo May 3, 2025 •

edited

Loading

tldahlgren May 7, 2025 •

edited

Loading