tests: speed up tests that rely on the database fixture (#12031)

The database and mutable_database fixtures were installing and uninstalling the same specs multiple times to ensure the database for tests has the correct state.

This commit optimizes the procedure by caching the state in an external directory, and copying it in instead of going through the installation or uninstallation again.

The database fixture is meant not to be modified by tests. This commit enforces this invariant by making the database read-only before starting the test.

* Added missing db markers to tests
* Added test for uninstall_by_spec
* `database` fixture now returns a read-only database
* Tests that modify the DB now use `mutable_database` fixture

lib/spack/spack/test/cmd/buildcache.py

@@ -17,5 +17,6 @@
     platform.system().lower() != 'linux',
     reason='implementation for MacOS still missing'
 )
+@pytest.mark.db
 def test_buildcache_preview_just_runs(database):
     buildcache('preview', 'mpileaks')

lib/spack/spack/test/cmd/debug.py

@@ -3,6 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import pytest
+
 import os
 import os.path
 
@@ -12,6 +14,7 @@
 debug = SpackCommand('debug')
 
 
+@pytest.mark.db
 def test_create_db_tarball(tmpdir, database):
     with tmpdir.as_cwd():
         debug('create-db-tarball')

lib/spack/spack/test/cmd/module.py

@@ -19,6 +19,12 @@ def _module_files(module_type, *specs):
     return [writer_cls(spec).layout.filename for spec in specs]
 
 
+@pytest.fixture(scope='module', autouse=True)
+def ensure_module_files_are_there(database):
+    module('dotkit', 'refresh', '-y')
+    module('tcl', 'refresh', '-y')
+
+
 @pytest.fixture(
     params=[
         ['rm', 'doesnotexist'],  # Try to remove a non existing module

lib/spack/spack/test/conftest.py

@@ -406,45 +406,58 @@ def _install(spec):
         _install('externaltest')
 
 
+@pytest.fixture(scope='session')
+def _store_dir_and_cache(tmpdir_factory):
+    """Returns the directory where to build the mock database and
+    where to cache it.
+    """
+    store = tmpdir_factory.mktemp('mock_store')
+    cache = tmpdir_factory.mktemp('mock_store_cache')
+    return store, cache
+
+
 @pytest.fixture(scope='module')
-def database(tmpdir_factory, mock_packages, config):
+def database(tmpdir_factory, mock_packages, config, _store_dir_and_cache):
-    """Creates a mock database with some packages installed note that
+    """Creates a read-only mock database with some packages installed note
-    the ref count for dyninst here will be 3, as it's recycled
+    that the ref count for dyninst here will be 3, as it's recycled
     across each install.
     """
-    # save the real store
     real_store = spack.store.store
+    store_path, store_cache = _store_dir_and_cache
 
-    # Make a fake install directory
+    mock_store = spack.store.Store(str(store_path))
-    install_path = tmpdir_factory.mktemp('install_for_database')
+    spack.store.store = mock_store
 
-    # Make fake store (database and install layout)
+    # If the cache does not exist populate the store and create it
-    tmp_store = spack.store.Store(str(install_path))
+    if not os.path.exists(str(store_cache.join('.spack-db'))):
-    spack.store.store = tmp_store
+        _populate(mock_store.db)
+        store_path.copy(store_cache, mode=True, stat=True)
 
-    _populate(tmp_store.db)
+    # Make the database read-only to ensure we can't modify entries
+    store_path.join('.spack-db').chmod(mode=0o555, rec=1)
 
-    yield tmp_store.db
+    yield mock_store.db
 
-    with tmp_store.db.write_transaction():
+    store_path.join('.spack-db').chmod(mode=0o755, rec=1)
-        for spec in tmp_store.db.query():
-            if spec.package.installed:
-                PackageBase.uninstall_by_spec(spec, force=True)
-            else:
-                tmp_store.db.remove(spec)
-
-    install_path.remove(rec=1)
     spack.store.store = real_store
 
 
 @pytest.fixture(scope='function')
-def mutable_database(database):
+def mutable_database(database, _store_dir_and_cache):
-    """For tests that need to modify the database instance."""
+    """Writeable version of the fixture, restored to its initial state
+    after each test.
+    """
+    # Make the database writeable, as we are going to modify it
+    store_path, store_cache = _store_dir_and_cache
+    store_path.join('.spack-db').chmod(mode=0o755, rec=1)
+
     yield database
-    with database.write_transaction():
+
-        for spec in spack.store.db.query():
+    # Restore the initial state by copying the content of the cache back into
-            PackageBase.uninstall_by_spec(spec, force=True)
+    # the store and making the database read-only
-    _populate(database)
+    store_path.remove(rec=1)
+    store_cache.copy(store_path, mode=True, stat=True)
+    store_path.join('.spack-db').chmod(mode=0o555, rec=1)
 
 
 @pytest.fixture(scope='function')

lib/spack/spack/test/database.py

@@ -19,6 +19,7 @@
 import spack.repo
 import spack.store
 import spack.database
+import spack.package
 import spack.spec
 from spack.test.conftest import MockPackage, MockPackageMultiRepo
 from spack.util.executable import Executable
@@ -411,7 +412,7 @@ def test_010_all_install_sanity(database):
     ) == 1
 
 
-def test_015_write_and_read(database):
+def test_015_write_and_read(mutable_database):
     # write and read DB
     with spack.store.db.write_transaction():
         specs = spack.store.db.query()
@@ -430,10 +431,10 @@ def test_020_db_sanity(database):
     _check_db_sanity(database)
 
 
-def test_025_reindex(database):
+def test_025_reindex(mutable_database):
     """Make sure reindex works and ref counts are valid."""
     spack.store.store.reindex()
-    _check_db_sanity(database)
+    _check_db_sanity(mutable_database)
 
 
 def test_030_db_sanity_from_another_process(mutable_database):
@@ -492,64 +493,64 @@ def test_050_basic_query(database):
     assert len(database.query(end_date=datetime.datetime.max)) == 16
 
 
-def test_060_remove_and_add_root_package(database):
+def test_060_remove_and_add_root_package(mutable_database):
-    _check_remove_and_add_package(database, 'mpileaks ^mpich')
+    _check_remove_and_add_package(mutable_database, 'mpileaks ^mpich')
 
 
-def test_070_remove_and_add_dependency_package(database):
+def test_070_remove_and_add_dependency_package(mutable_database):
-    _check_remove_and_add_package(database, 'dyninst')
+    _check_remove_and_add_package(mutable_database, 'dyninst')
 
 
-def test_080_root_ref_counts(database):
+def test_080_root_ref_counts(mutable_database):
-    rec = database.get_record('mpileaks ^mpich')
+    rec = mutable_database.get_record('mpileaks ^mpich')
 
     # Remove a top-level spec from the DB
-    database.remove('mpileaks ^mpich')
+    mutable_database.remove('mpileaks ^mpich')
 
     # record no longer in DB
-    assert database.query('mpileaks ^mpich', installed=any) == []
+    assert mutable_database.query('mpileaks ^mpich', installed=any) == []
 
     # record's deps have updated ref_counts
-    assert database.get_record('callpath ^mpich').ref_count == 0
+    assert mutable_database.get_record('callpath ^mpich').ref_count == 0
-    assert database.get_record('mpich').ref_count == 1
+    assert mutable_database.get_record('mpich').ref_count == 1
 
     # Put the spec back
-    database.add(rec.spec, spack.store.layout)
+    mutable_database.add(rec.spec, spack.store.layout)
 
     # record is present again
-    assert len(database.query('mpileaks ^mpich', installed=any)) == 1
+    assert len(mutable_database.query('mpileaks ^mpich', installed=any)) == 1
 
     # dependencies have ref counts updated
-    assert database.get_record('callpath ^mpich').ref_count == 1
+    assert mutable_database.get_record('callpath ^mpich').ref_count == 1
-    assert database.get_record('mpich').ref_count == 2
+    assert mutable_database.get_record('mpich').ref_count == 2
 
 
-def test_090_non_root_ref_counts(database):
+def test_090_non_root_ref_counts(mutable_database):
-    database.get_record('mpileaks ^mpich')
+    mutable_database.get_record('mpileaks ^mpich')
-    database.get_record('callpath ^mpich')
+    mutable_database.get_record('callpath ^mpich')
 
     # "force remove" a non-root spec from the DB
-    database.remove('callpath ^mpich')
+    mutable_database.remove('callpath ^mpich')
 
     # record still in DB but marked uninstalled
-    assert database.query('callpath ^mpich', installed=True) == []
+    assert mutable_database.query('callpath ^mpich', installed=True) == []
-    assert len(database.query('callpath ^mpich', installed=any)) == 1
+    assert len(mutable_database.query('callpath ^mpich', installed=any)) == 1
 
     # record and its deps have same ref_counts
-    assert database.get_record(
+    assert mutable_database.get_record(
         'callpath ^mpich', installed=any
     ).ref_count == 1
-    assert database.get_record('mpich').ref_count == 2
+    assert mutable_database.get_record('mpich').ref_count == 2
 
     # remove only dependent of uninstalled callpath record
-    database.remove('mpileaks ^mpich')
+    mutable_database.remove('mpileaks ^mpich')
 
     # record and parent are completely gone.
-    assert database.query('mpileaks ^mpich', installed=any) == []
+    assert mutable_database.query('mpileaks ^mpich', installed=any) == []
-    assert database.query('callpath ^mpich', installed=any) == []
+    assert mutable_database.query('callpath ^mpich', installed=any) == []
 
     # mpich ref count updated properly.
-    mpich_rec = database.get_record('mpich')
+    mpich_rec = mutable_database.get_record('mpich')
     assert mpich_rec.ref_count == 0
 
 
@@ -596,18 +597,18 @@ def test_115_reindex_with_packages_not_in_repo(mutable_database):
         _check_db_sanity(mutable_database)
 
 
-def test_external_entries_in_db(database):
+def test_external_entries_in_db(mutable_database):
-    rec = database.get_record('mpileaks ^zmpi')
+    rec = mutable_database.get_record('mpileaks ^zmpi')
     assert rec.spec.external_path is None
     assert rec.spec.external_module is None
 
-    rec = database.get_record('externaltool')
+    rec = mutable_database.get_record('externaltool')
     assert rec.spec.external_path == '/path/to/external_tool'
     assert rec.spec.external_module is None
     assert rec.explicit is False
 
     rec.spec.package.do_install(fake=True, explicit=True)
-    rec = database.get_record('externaltool')
+    rec = mutable_database.get_record('externaltool')
     assert rec.spec.external_path == '/path/to/external_tool'
     assert rec.spec.external_module is None
     assert rec.explicit is True
@@ -646,3 +647,13 @@ def test_old_external_entries_prefix(mutable_database):
     assert record.path is None
     assert record.spec._prefix is None
     assert record.spec.prefix == record.spec.external_path
+
+
+def test_uninstall_by_spec(mutable_database):
+    with mutable_database.write_transaction():
+        for spec in mutable_database.query():
+            if spec.package.installed:
+                spack.package.PackageBase.uninstall_by_spec(spec, force=True)
+            else:
+                mutable_database.remove(spec)
+    assert len(mutable_database.query()) == 0

lib/spack/spack/test/spec_syntax.py

@@ -310,15 +310,15 @@ def test_multiple_specs_with_hash(self, database):
         assert len(specs) == 2
 
     @pytest.mark.db
-    def test_ambiguous_hash(self, database):
+    def test_ambiguous_hash(self, mutable_database):
         x1 = Spec('a')
         x1._hash = 'xy'
         x1._concrete = True
         x2 = Spec('a')
         x2._hash = 'xx'
         x2._concrete = True
-        database.add(x1, spack.store.layout)
+        mutable_database.add(x1, spack.store.layout)
-        database.add(x2, spack.store.layout)
+        mutable_database.add(x2, spack.store.layout)
 
         # ambiguity in first hash character
         self._check_raises(AmbiguousHashError, ['/x'])