tests: cleanup config:build_stage handling (fixes spack#12651, spack#…

…12798)

lib/spack/spack/config.py

@@ -110,6 +110,9 @@
 #: this is shorter and more readable than listing all choices
 scopes_metavar = '{defaults,system,site,user}[/PLATFORM]'
 
+#: Base name for the (internal) overrides scope.
+overrides_base_name = 'overrides-'
+
 
 def first_existing(dictionary, keys):
     """Get the value of the first key in keys that is in the dictionary."""
@@ -549,11 +552,11 @@ def override(path_or_scope, value=None):
     an internal config scope for it and push/pop that scope.
 
     """
-    base_name = 'overrides-'
     if isinstance(path_or_scope, ConfigScope):
         overrides = path_or_scope
         config.push_scope(path_or_scope)
     else:
+        base_name = overrides_base_name
         # Ensure the new override gets a unique scope name
         current_overrides = [s.name for s in
                              config.matching_scopes(r'^{0}'.format(base_name))]

lib/spack/spack/test/config.py

@@ -628,11 +628,11 @@ def test_add_command_line_scopes(tmpdir, mutable_config):
 @pytest.mark.nomockstage
 def test_nested_override():
     """Ensure proper scope naming of nested overrides."""
-    # WARNING: Base name must match that used in `config.py`s `override()`.
-    base_name = 'overrides-'
+    base_name = spack.config.overrides_base_name
 
     def _check_scopes(num_expected, debug_values):
-        scope_names = [s.name for s in spack.config.config.scopes.values()]
+        scope_names = [s.name for s in spack.config.config.scopes.values() if
+                       s.name.startswith(base_name)]
 
         for i in range(num_expected):
             name = '{0}{1}'.format(base_name, i)
@@ -652,8 +652,7 @@ def _check_scopes(num_expected, debug_values):
 @pytest.mark.nomockstage
 def test_alternate_override(monkeypatch):
     """Ensure proper scope naming of override when conflict present."""
-    # WARNING: Base name must match that used in `config.py`s `override()`.
-    base_name = 'overrides-'
+    base_name = spack.config.overrides_base_name
 
     def _matching_scopes(regexpr):
         return [spack.config.InternalConfigScope('{0}1'.format(base_name))]

lib/spack/spack/test/conftest.py

@@ -16,7 +16,7 @@
 import pytest
 import ruamel.yaml as yaml
 
-from llnl.util.filesystem import remove_linked_tree
+from llnl.util.filesystem import mkdirp, remove_linked_tree
 
 import spack.architecture
 import spack.compilers
@@ -121,34 +121,27 @@ def reset_compiler_cache():
     spack.compilers._compiler_cache = {}
 
 
-@pytest.fixture
-def clear_stage_root(monkeypatch):
-    """Ensure spack.stage._stage_root is not set at test start."""
-    monkeypatch.setattr(spack.stage, '_stage_root', None)
-    yield
-
-
 @pytest.fixture(scope='function', autouse=True)
-def mock_stage(clear_stage_root, tmpdir_factory, request):
+def mock_stage(tmpdir_factory, monkeypatch, request):
     """Establish the temporary build_stage for the mock archive."""
-    # Workaround to skip mock_stage for 'nomockstage' test cases
+    # The approach with this autouse fixture is to set the stage root
+    # instead of using spack.config.override() to avoid configuration
+    # conflicts with dozens of tests that rely on other configuration
+    # fixtures, such as config.
     if 'nomockstage' not in request.keywords:
+        # Set the build stage to the requested path
         new_stage = tmpdir_factory.mktemp('mock-stage')
         new_stage_path = str(new_stage)
 
-        # Set test_stage_path as the default directory to use for test stages.
-        current = spack.config.get('config:build_stage')
-        spack.config.set('config',
-                         {'build_stage': new_stage_path}, scope='user')
+        # Ensure the source directory exists within the new stage path
+        source_path = os.path.join(new_stage_path,
+                                   spack.stage._source_path_subdir)
+        mkdirp(source_path)
 
-        # Ensure the source directory exists
-        source_path = new_stage.join(spack.stage._source_path_subdir)
-        source_path.ensure(dir=True)
+        monkeypatch.setattr(spack.stage, '_stage_root', new_stage_path)
 
         yield new_stage_path
 
-        spack.config.set('config', {'build_stage': current}, scope='user')
-
         # Clean up the test stage directory
         if os.path.isdir(new_stage_path):
             shutil.rmtree(new_stage_path)

lib/spack/spack/test/stage.py

@@ -13,7 +13,7 @@
 
 import pytest
 
-from llnl.util.filesystem import mkdirp, working_dir
+from llnl.util.filesystem import mkdirp, touch, working_dir
 
 import spack.paths
 import spack.stage
@@ -70,6 +70,23 @@
 #
 
 
+@pytest.fixture
+def mock_always_access_path(monkeypatch):
+    """Mock the ability to always access a path (for test coverage)."""
+    def _can_access(path, perms):
+        return True
+
+    monkeypatch.setattr(os, 'access', _can_access)
+    yield
+
+
+@pytest.fixture
+def clear_stage_root(monkeypatch):
+    """Ensure spack.stage._stage_root is not set at test start."""
+    monkeypatch.setattr(spack.stage, '_stage_root', None)
+    yield
+
+
 def check_expand_archive(stage, stage_name, expected_file_list):
     """
     Ensure the expanded archive directory contains the expected structure and
@@ -177,82 +194,32 @@ def get_stage_path(stage, stage_name):
 
 
 @pytest.fixture
-def bad_stage_path():
-    """Temporarily ensure there is no accessible path for staging."""
-    current = spack.config.get('config:build_stage')
-    spack.config.set('config', {'build_stage': '/no/such/path'}, scope='user')
-    yield
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
-@pytest.fixture
-def non_user_path_for_stage(monkeypatch):
-    """Temporarily use a Linux-standard non-user path for staging. """
-    def _can_access(path, perms):
-        return True
-
-    current = spack.config.get('config:build_stage')
-    spack.config.set('config', {'build_stage': [_non_user_root]}, scope='user')
-    monkeypatch.setattr(os, 'access', _can_access)
-    yield
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
-@pytest.fixture
-def instance_path_for_stage():
-    """
-    Temporarily use the "traditional" spack instance stage path for staging.
-
-    Note that it can be important for other tests that the previous settings be
-    restored when the test case is over.
-    """
-    current = spack.config.get('config:build_stage')
-    base = canonicalize_path(os.path.join('$spack', 'test-stage'))
+@pytest.mark.nomockstage
+def instance_path_for_stage(clear_stage_root):
+    """Use the spack instance stage path for the stage root."""
+    base = canonicalize_path(os.path.join('$spack', '.spack-test-stage'))
     mkdirp(base)
-    path = tempfile.mkdtemp(dir=base)
-    spack.config.set('config', {'build_stage': path}, scope='user')
-    yield
-    spack.config.set('config', {'build_stage': current}, scope='user')
-    shutil.rmtree(base)
+    test_path = tempfile.mkdtemp(dir=base)
 
+    with spack.config.override('config:build_stage',  test_path):
+        yield spack.stage.get_stage_root()
 
-@pytest.fixture
-def tmp_path_for_stage(tmpdir):
-    """
-    Use a temporary test directory for staging.
-
-    Note that it can be important for other tests that the previous settings be
-    restored when the test case is over.
-    """
-    current = spack.config.get('config:build_stage')
-    spack.config.set('config', {'build_stage': [str(tmpdir)]}, scope='user')
-    yield tmpdir
-    spack.config.set('config', {'build_stage': current}, scope='user')
+    shutil.rmtree(base)
 
 
 @pytest.fixture
-def tmp_build_stage_dir(tmpdir):
-    """Establish the temporary build_stage for the mock archive."""
-    test_stage_path = tmpdir.join('stage')
+def tmp_build_stage_dir(tmpdir, clear_stage_root):
+    """Use a temporary test directory for the stage root."""
+    test_path = str(tmpdir.join('stage'))
+    with spack.config.override('config:build_stage', test_path):
+        yield tmpdir, spack.stage.get_stage_root()
 
-    # Set test_stage_path as the default directory to use for test stages.
-    current = spack.config.get('config:build_stage')
-    spack.config.set('config',
-                     {'build_stage': [str(test_stage_path)]}, scope='user')
-
-    yield (tmpdir, test_stage_path)
-
-    spack.config.set('config', {'build_stage': current}, scope='user')
+    shutil.rmtree(test_path)
 
 
 @pytest.fixture
-def mock_stage_archive(clear_stage_root, tmp_build_stage_dir, request):
-    """
-    Create the directories and files for the staged mock archive.
-
-    Note that it can be important for other tests that the previous settings be
-    restored when the test case is over.
-    """
+def mock_stage_archive(tmp_build_stage_dir):
+    """Create the directories and files for the staged mock archive."""
     # Mock up a stage area that looks like this:
     #
     # tmpdir/                test_files_dir
@@ -265,7 +232,7 @@ def mock_stage_archive(clear_stage_root, tmp_build_stage_dir, request):
     #
     def create_stage_archive(expected_file_list=[_include_readme]):
         tmpdir, test_stage_path = tmp_build_stage_dir
-        test_stage_path.ensure(dir=True)
+        mkdirp(test_stage_path)
 
         # Create the archive directory and associated file
         archive_dir = tmpdir.join(_archive_base)
@@ -744,72 +711,77 @@ def test_resolve_paths(self):
             can_paths[1] = os.path.join(can_paths[1], user)
         assert spack.stage._resolve_paths(paths) == can_paths
 
-    def test_get_stage_root_bad_path(self, clear_stage_root, bad_stage_path):
+    @pytest.mark.nomockstage
+    def test_get_stage_root_bad_path(self, clear_stage_root):
         """Ensure an invalid stage path root raises a StageError."""
-        with pytest.raises(spack.stage.StageError,
-                           match="No accessible stage paths in"):
-            assert spack.stage.get_stage_root() is None
+        with spack.config.override('config:build_stage', '/no/such/path'):
+            with pytest.raises(spack.stage.StageError,
+                               match="No accessible stage paths in"):
+                assert spack.stage.get_stage_root() is None
 
         # Make sure the cached stage path values are unchanged.
         assert spack.stage._stage_root is None
 
+    @pytest.mark.nomockstage
     def test_get_stage_root_non_user_path(self, clear_stage_root,
-                                          non_user_path_for_stage):
+                                          mock_always_access_path):
         """Ensure a non-user stage root includes the username."""
         # The challenge here is whether the user has access to the standard
         # non-user path.  If not, the path should still appear in the error.
         try:
-            path = spack.stage.get_stage_root()
-            assert getpass.getuser() in path.split(os.path.sep)
+            with spack.config.override('config:build_stage', _non_user_root):
+                path = spack.stage.get_stage_root()
+
+                assert getpass.getuser() in path.split(os.path.sep)
+
+                # Ensure cached stage path values are changed appropriately.
+                assert spack.stage._stage_root == path
 
-            # Make sure the cached stage path values are changed appropriately.
-            assert spack.stage._stage_root == path
         except OSError as e:
             expected = os.path.join(_non_user_root, getpass.getuser())
             assert expected in str(e)
 
             # Make sure the cached stage path values are unchanged.
             assert spack.stage._stage_root is None
 
-    def test_get_stage_root_tmp(self, clear_stage_root, tmp_path_for_stage):
-        """Ensure a temp path stage root is a suitable temp path."""
-        assert spack.stage._stage_root is None
-
-        tmpdir = tmp_path_for_stage
-        path = spack.stage.get_stage_root()
-        assert path == str(tmpdir)
-        assert 'test_get_stage_root_tmp' in path
+    @pytest.mark.nomockstage
+    @pytest.mark.parametrize(
+        'path,purged', [('stage-1234567890abcdef1234567890abcdef', True),
+                        ('stage-abcdef12345678900987654321fedcba', True),
+                        ('stage-a1b2c3', False)])
+    def test_stage_purge(self, tmpdir, clear_stage_root, path, purged):
+        """Test purging of stage directories."""
+        stage_dir = tmpdir.join('stage')
+        stage_path = str(stage_dir)
 
-        # Make sure the cached stage path values are changed appropriately.
-        assert spack.stage._stage_root == path
+        test_dir = stage_dir.join(path)
+        test_dir.ensure(dir=True)
+        test_path = str(test_dir)
 
-        # Add then purge a few directories
-        dir1 = tmpdir.join('stage-1234567890abcdef1234567890abcdef')
-        dir1.ensure(dir=True)
-        dir2 = tmpdir.join('stage-abcdef12345678900987654321fedcba')
-        dir2.ensure(dir=True)
-        dir3 = tmpdir.join('stage-a1b2c3')
-        dir3.ensure(dir=True)
+        with spack.config.override('config:build_stage', stage_path):
+            stage_root = spack.stage.get_stage_root()
+            assert stage_path == stage_root
 
-        spack.stage.purge()
-        assert not os.path.exists(str(dir1))
-        assert not os.path.exists(str(dir2))
-        assert os.path.exists(str(dir3))
+            spack.stage.purge()
 
-        # Cleanup
-        shutil.rmtree(str(dir3))
+            if purged:
+                assert not os.path.exists(test_path)
+            else:
+                assert os.path.exists(test_path)
+                shutil.rmtree(test_path)
 
-    def test_get_stage_root_in_spack(self, clear_stage_root,
-                                     instance_path_for_stage):
+    def test_get_stage_root_in_spack(self, instance_path_for_stage):
         """Ensure an instance path stage root is a suitable path."""
-        assert spack.stage._stage_root is None
 
-        path = spack.stage.get_stage_root()
+        path = instance_path_for_stage
         assert 'spack' in path.split(os.path.sep)
 
-        # Make sure the cached stage path values are changed appropriately.
+        # Make sure the cached stage path value was changed appropriately.
         assert spack.stage._stage_root == path
 
+        # Also make sure the directory exists.
+        assert os.path.isdir(spack.stage._stage_root)
+
     def test_stage_constructor_no_fetcher(self):
         """Ensure Stage constructor with no URL or fetch strategy fails."""
         with pytest.raises(ValueError):
@@ -879,34 +851,20 @@ def test_diystage_preserve_file(self, tmpdir):
             _file.read() == _readme_contents
 
 
-@pytest.fixture
-def tmp_build_stage_nondir(tmpdir):
-    """Establish the temporary build_stage pointing to non-directory."""
-    test_stage_path = tmpdir.join('stage', 'afile')
-    test_stage_path.ensure(dir=False)
-
-    # Set test_stage_path as the default directory to use for test stages.
-    current = spack.config.get('config:build_stage')
-    stage_dir = os.path.dirname(str(test_stage_path))
-    spack.config.set('config', {'build_stage': [stage_dir]}, scope='user')
-
-    yield test_stage_path
-
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
+@pytest.mark.nomockstage
 def test_stage_create_replace_path(tmp_build_stage_dir):
     """Ensure stage creation replaces a non-directory path."""
     _, test_stage_path = tmp_build_stage_dir
-    test_stage_path.ensure(dir=True)
+    mkdirp(test_stage_path)
 
-    nondir = test_stage_path.join('afile')
-    nondir.ensure(dir=False)
+    nondir = os.path.join(test_stage_path, 'afile')
+    touch(nondir)
     path = str(nondir)
 
     stage = Stage(path, name='')
-    stage.create()  # Should ensure the path is converted to a dir
+    stage.create()
 
+    # Ensure the stage path is "converted" to a directory
     assert os.path.isdir(stage.path)