Fix wrong hash in spliced specs, and improve unit-tests #48574
Merged
alalazo merged 17 commits intospack:developfrom Jan 15, 2025
Merged
Fix wrong hash in spliced specs, and improve unit-tests #48574alalazo merged 17 commits intospack:developfrom
alalazo merged 17 commits intospack:developfrom
Conversation
spackbot-app
bot
added
commands
core
Member
Author
|
The PR is still a draft, partially because I think I found a bug, at least in the tests. Applying this diff: diff --git a/lib/spack/spack/test/abi_splicing.py b/lib/spack/spack/test/abi_splicing.py
index 39cea68511..890d11099e 100644
--- a/lib/spack/spack/test/abi_splicing.py
+++ b/lib/spack/spack/test/abi_splicing.py
@@ -54,7 +54,8 @@ def test_spec_reuse(spec_str, install_specs, mutable_config):
def test_splice_installed_hash(install_specs, mutable_config):
- install_specs(
+ """Tests splicing the dependency of an installed spec, for another installed spec"""
+ splice_t, splice_h = install_specs(
"splice-t@1 ^[email protected]+compat ^[email protected]",
"[email protected]+compat ^[email protected]",
)
@@ -64,8 +65,12 @@ def test_splice_installed_hash(install_specs, mutable_config):
goal_spec = Spec("splice-t@1 ^[email protected]+compat ^[email protected]")
with pytest.raises(SolverError):
goal_spec.concretized()
+
_enable_splicing()
- assert goal_spec.concretized().satisfies(goal_spec)
+ concrete = goal_spec.concretized()
+ assert concrete.build_spec.satisfies(splice_t)
+ assert not concrete.satisfies(splice_t)
+ assert concrete["splice-h"].satisfies(splice_h)
the EDIT: See bug report #48578 |
alalazo
force-pushed
the
maintainers/remove-code-duplication-in-abi-splicing-tests
branch
from
9a54ef5 to
fb205c7
Compare
alalazo
changed the title
abi_splicing unit tests
JohnGouwar
suggested changes
Jan 15, 2025
Contributor
JohnGouwar
left a comment
JohnGouwar
left a comment
There was a problem hiding this comment.
The changes to the copy in splicing look good, there is just one change in test_double_splice that is needed to make the test pass.
Also, use mutable_config directly, instead of the module wrapper for the global CONFIG.set
Spec.dependencies can already query by name.
Also, remove an unused argument of Spec._dup
alalazo
force-pushed
the
maintainers/remove-code-duplication-in-abi-splicing-tests
branch
from
d7ccb89 to
7481d0b
Compare
JohnGouwar
approved these changes
Jan 15, 2025
Contributor
JohnGouwar
left a comment
JohnGouwar
left a comment
There was a problem hiding this comment.
All these changes look good to me; they definitely make the unit-tests better and cleaner. test_double_splice is having problems due to some instability in the topo ordering of multi-rooted specs, but that may be a deeper fix.
3 tasks
tgamblin
approved these changes
Jan 15, 2025
alalazo
deleted the
maintainers/remove-code-duplication-in-abi-splicing-tests
branch
3 tasks
mtaillefumier
pushed a commit
to mtaillefumier/spack
that referenced
this pull request
Jan 20, 2025
Modifications: * Fix a severe bug where a spliced spec has the same hash as the original build spec * Removed CacheManager in favor of install_mockery. The latter sets up a temporary store, and removes it at the end of the test. * Deleted a couple of helper functions e.g. _has_dependencies * Checked that SolverException is raised, rather than Exception (more specific) * Extended, and renamed the splicing_setup fixture (now called install_specs) * Added more specific assertions in each test One test is currently flaky, due to some instability when sorting multiple specs. It's currently marked xfail
Member
|
This does not apply well for backporting, so i'm dropping it. Feel free to fix splicing for 0.23 by cherry-picking the relevant commits from develop and opening a PR against the |
teaguesterling
pushed a commit
to teaguesterling/spack
that referenced
this pull request
Feb 5, 2025
Modifications: * Fix a severe bug where a spliced spec has the same hash as the original build spec * Removed CacheManager in favor of install_mockery. The latter sets up a temporary store, and removes it at the end of the test. * Deleted a couple of helper functions e.g. _has_dependencies * Checked that SolverException is raised, rather than Exception (more specific) * Extended, and renamed the splicing_setup fixture (now called install_specs) * Added more specific assertions in each test One test is currently flaky, due to some instability when sorting multiple specs. It's currently marked xfail
mrmundt
pushed a commit
to mrmundt/spack
that referenced
this pull request
Feb 17, 2025
Modifications: * Fix a severe bug where a spliced spec has the same hash as the original build spec * Removed CacheManager in favor of install_mockery. The latter sets up a temporary store, and removes it at the end of the test. * Deleted a couple of helper functions e.g. _has_dependencies * Checked that SolverException is raised, rather than Exception (more specific) * Extended, and renamed the splicing_setup fixture (now called install_specs) * Added more specific assertions in each test One test is currently flaky, due to some instability when sorting multiple specs. It's currently marked xfail
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #48578
I did a minor clean-up of the unit-tests while reading the code related to splicing.
Modifications:
CacheManagerin favor ofinstall_mockery. The latter sets up a temporary store, and removes it at the end of the test, and is used everywhere else in the tests._has_dependenciesExceptionin testssplicing_setup(now calledinstall_specs)assert concretize_one(abstract).satisfies(abstract)