Set MPI providers for test config by scheibelp · Pull Request #16003 · spack/spack

scheibelp · 2020-04-10T23:49:46Z

For tests that use the real Spack package repository, the config needs to avoid using MPI providers that are not intended to be installed by Spack. Without this, it is possible that Spack tests which concretize the MPI virtual will end up trying to use an implementation that it shouldn't (e.g. one that is always provided externally).

See: #15666 (and specifically the tests for it at https://travis-ci.org/github/spack/spack/jobs/672203234)

…s to avoid using providers that are not intended to be installed by Spack

… some issue with that

…ts in concretize are failing

… right locations (was omitting the final directory and only using the root)

…se an incompatible arch will be chosen and the compiler pref will be ignored

… implies some issue with that" This reverts commit 37719a9.

scheibelp · 2020-04-17T21:37:11Z

Extra fixes based on follow-up errors:

mutable_config was not initializing the scope roots to the right directories
~~mutable_config was not clearing config caches~~ (this has since been removed - the use_configuration fixture replaces the configuration entirely)
The current_host fixture in the concretize.py tests was not being executed in the context of mutable_config, and was polluting the config cache for other tests
One test in concretize_preference was depending on cross test config pollution to succeed, I have explicitly updated the initial spec it constructs to arrive at the appropriate result
One test in concretize.py was clearing a nonexistent cache (PackagePrefs._packages_config_cache) - which created a red herring

Additional notes

Any test using config.override should use mutable_config (since this can change the contents of the in-memory config cache)
Tests will not be able to use config.clear_caches() with the config fixture, since that removes the _builtin scope with default settings. If the config fixture were refactored it would be possible to do this but I don't think it makes sense (semantically it is a bit contradictory to have an implied non-mutable set-up fixture that then requires teardown on the part of the user). (the use_configuration fixture should generally resolve these issues, so long as each test that uses the config fixture doesn't actually modify it - in that case it will change the config for all other tests using that fixture)

Based on that, my additional suggestion (which may be appropriate for this PR) is to raise an error if a user tries to call config.override when the config fixture is in use (or perhaps even better, whenever the mutable_config fixture is not in use).

…d to initialize scopes every time it is used if I do that

…efactored to initialize scopes every time it is used if I do that" This reverts commit d31be68.

…ot expected)

alalazo

LGTM, I just have a few questions/comments.

lib/spack/spack/test/conftest.py

lib/spack/spack/test/concretize.py

lib/spack/spack/cmd/compiler.py

alalazo · 2020-05-01T18:21:08Z

@scheibelp Don't know what went wrong, but apparently this PR is not passing unit tests on develop. See https://travis-ci.org/github/spack/spack/jobs/682049262

scheibelp · 2020-05-01T18:23:46Z

OK - I don't have time to fix this so I'll have to revert it and get back to fixing whatever other test pollution is occurring that is resulting in this next week.

…)" This was breaking tests on develop.

…16420) This was breaking tests on develop.

…pack#16003)" (spack#16420)" This reverts commit 40df44e.

This fixes some errors with setting up test configuration. These errors do not cause current Spack tests to fail but do create red herring issues elsewhere (see #15666). Fixing these errors leads to more errors in tests that depended on the original misconfigured state, so those are also addressed here. This is an update to #16003 which accounts for some unit tests with conflicting config/mutable_config fixtures. These conflicts were not exposed until the mutable_config fixture was fixed. Details are included below. The change which builds on #16003 is prefixed with "(new)". * For tests that use the real Spack package repository, the config needs to avoid using MPI providers that are not intended to be installed by Spack. Without this, it is possible that Spack tests which concretize the MPI virtual will end up trying to use an implementation that it shouldn't (e.g. one that is always provided externally). See #15666 for an example. * The mutable_config test fixture was not initializing the scope roots to the right directories (so the resulting config was empty). * The current_host fixture in the concretize.py tests was using the config fixture rather than mutable_config, and was polluting the config cache for other tests. * One test in concretize.py was clearing a nonexistent cache (PackagePrefs._packages_config_cache). This reference has been removed. * The test 'test_preferred_compilers' was was depending on cross test config pollution to succeed. The initial spec before concretization has been updated to updated to be explicit about the desired result. * (new) For tests that use install_mockery and mutable_config, replace install_mockery with a separate install_mockery_mutable_config fixture that is exactly the same as install_mockery but uses the mutable_config fixture to avoid conflicts.

for tests that use the real spack package repository, the config need…

0a42067

…s to avoid using providers that are not intended to be installed by Spack

This comment has been minimized.

Sign in to view

adamjstewart added configuration mpi tests General test capability(ies) labels Apr 11, 2020

scheibelp added 10 commits April 16, 2020 18:10

remove arch memoizations for now (no effect but the complaint implies…

37719a9

… some issue with that

remove cache clearing for cache that no longer exists

ab83275

swap to mutable_config; mutable_config should clear config cache; tes…

ad2e068

…ts in concretize are failing

remove fixture from test where it wasn't used

3af9aa6

mutable_config was not initializing ConfigScope roots to point to the…

902ccc8

… right locations (was omitting the final directory and only using the root)

preference test needs to specify non-default os/target, since otherwi…

13ce412

…se an incompatible arch will be chosen and the compiler pref will be ignored

Revert "remove arch memoizations for now (no effect but the complaint…

ead9659

… implies some issue with that" This reverts commit 37719a9.

add explanatory comment

b785c76

add notes on latest findings

d2d6f96

switch concretization tests back to using mutable_config

dc10950

scheibelp added 7 commits April 17, 2020 14:38

remove unused import

4742854

remove clear_caches since 'config' fixture would need to be refactore…

d31be68

…d to initialize scopes every time it is used if I do that

Revert "remove clear_caches since 'config' fixture would need to be r…

3486013

…efactored to initialize scopes every time it is used if I do that" This reverts commit d31be68.

make compiler_list sorting robust to case of 'target is None'

b50ea57

flake edit

8694bd9

dev_build cmd tests fail if mutable_config clears the config cache (n…

c604652

…ot expected)

remove debugging comments

7d00836

scheibelp requested a review from alalazo April 21, 2020 02:47

tgamblin self-requested a review April 21, 2020 21:12

alalazo reviewed Apr 22, 2020

View reviewed changes

lib/spack/spack/test/conftest.py Show resolved Hide resolved

lib/spack/spack/test/concretize.py Show resolved Hide resolved

lib/spack/spack/cmd/compiler.py Show resolved Hide resolved

alalazo approved these changes Apr 27, 2020

View reviewed changes

scheibelp merged commit 31d12d3 into spack:develop May 1, 2020

scheibelp added a commit to scheibelp/spack that referenced this pull request May 1, 2020

Revert "Testing: fix unintended interactions between tests (spack#16003…

76fc3df

…)" This was breaking tests on develop.

scheibelp mentioned this pull request May 1, 2020

Revert "Testing: fix unintended interactions between tests (#16003)" #16420

Merged

scheibelp added a commit that referenced this pull request May 1, 2020

Revert "Testing: fix unintended interactions between tests (#16003)" (#…

40df44e

…16420) This was breaking tests on develop.

scheibelp added a commit to scheibelp/spack that referenced this pull request May 2, 2020

Revert "Revert "Testing: fix unintended interactions between tests (s…

d1e6e07

…pack#16003)" (spack#16420)" This reverts commit 40df44e.

This was referenced May 2, 2020

Testing: fix unintended interactions between tests, part 2 #16429

Merged

[wip] charmpp : fix build prefix, introduce new property charmarch #15666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set MPI providers for test config#16003

Set MPI providers for test config#16003
scheibelp merged 18 commits intospack:developfrom
scheibelp:bugfix/set-mpi-providers-for-tests

scheibelp commented Apr 10, 2020

Uh oh!

This comment has been minimized.

scheibelp commented Apr 17, 2020 •

edited

Loading

Uh oh!

alalazo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alalazo commented May 1, 2020

Uh oh!

scheibelp commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

scheibelp commented Apr 10, 2020

Uh oh!

This comment has been minimized.

scheibelp commented Apr 17, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

alalazo commented May 1, 2020

Uh oh!

scheibelp commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scheibelp commented Apr 17, 2020 •

edited

Loading