[devops] Run tests with sqlite backend #6425

danielhollas · 2024-05-27T23:44:41Z

The purpose of this PR is twofold:

Being able to run the test suite locally without requiring any services.
Run the full test suite with sqlite_dos as a storage backend

Two new pytest markers are added to the test suite:

requires_pgsql for tests that are known to require PostgreSQL (i.e. do not work with SQLite)
presto which is an alias for not requires_rmq and not requires_pgsql. The tests are marked with presto automagically in tests/conftest.py:pytest_collection_modifyitems

Two new CI jobs are added:

test-presto runs with pytest -m presto
test-sqlite runs with pytest -m not requires_pgsql`.
Perhaps we might retain only one of them?

In exchange for introducing two new ones, I removed the two verdi jobs and moved the corresponding tests (such as verdi devel check-imports after the normal test run. These are super fast and setting them up separately takes much more time then running them. Happy to discuss this though.

sphuber · 2024-05-29T13:21:33Z

You can use this for the config_sqlite_dos fixture:

(aiida-py311) sph@invader:~/code/aiida/env/dev/aiida-core$ git stash show stash@{0} -p
diff --git a/src/aiida/tools/pytest_fixtures/__init__.py b/src/aiida/tools/pytest_fixtures/__init__.py
index 6181183d1..79d027f02 100644
--- a/src/aiida/tools/pytest_fixtures/__init__.py
+++ b/src/aiida/tools/pytest_fixtures/__init__.py
@@ -25,7 +25,7 @@ from .orm import (
     aiida_localhost,
     ssh_key,
 )
-from .storage import config_psql_dos, postgres_cluster
+from .storage import config_psql_dos, config_sqlite_dos, postgres_cluster

 __all__ = (
     'aiida_code_installed',
diff --git a/src/aiida/tools/pytest_fixtures/storage.py b/src/aiida/tools/pytest_fixtures/storage.py
index d76c9b445..e216df664 100644
--- a/src/aiida/tools/pytest_fixtures/storage.py
+++ b/src/aiida/tools/pytest_fixtures/storage.py
@@ -2,6 +2,7 @@

 from __future__ import annotations

+import pathlib
 import typing as t

 import pytest
@@ -85,3 +86,24 @@ def config_psql_dos(
         return storage_config

     return factory
+
+
+
+@pytest.fixture(scope='session')
+def config_sqlite_dos(
+    tmp_path_factory: pytest.TempPathFactory,
+) -> t.Callable[[str | None, str | None, str | None], dict[str, t.Any]]:
+    """Return a profile configuration for the :class:`~aiida.storage.sqlite_dos.backend.SqliteDosStorage`.
+
+    The factory has the following signature to allow further configuring the database that is created:
+
+    :param database_name: Name of the database to be created.
+    :returns: The dictionary with the storage configuration for the ``core.sqlite_dos`` storage plugin.
+    """
+
+    def factory(filepath: str | pathlib.Path | None = None) -> dict[str, t.Any]:
+        return {
+            'filepath': str(filepath or tmp_path_factory.mktemp('test_sqlite_dos_storage'))
+        }
+
+    return factory
diff --git a/tests/conftest.py b/tests/conftest.py
index 55bf01a18..4c7f7f7d0 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -35,7 +35,7 @@ pytest_plugins = ['aiida.tools.pytest_fixtures', 'sphinx.testing.fixtures']


 @pytest.fixture(scope='session')
-def aiida_profile(aiida_config, aiida_profile_factory, config_psql_dos):
+def aiida_profile(aiida_config, aiida_profile_factory, config_psql_dos, config_sqlite_dos):
     """Create and load a profile with ``core.psql_dos`` as a storage backend and RabbitMQ as the broker.

     This overrides the ``aiida_profile`` fixture provided by ``aiida-core`` which runs with ``core.sqlite_dos`` and
@@ -43,7 +43,7 @@ def aiida_profile(aiida_config, aiida_profile_factory, config_psql_dos):
     be run against the main storage backend, which is ``core.sqlite_dos``.
     """
     with aiida_profile_factory(
-        aiida_config, storage_backend='core.psql_dos', storage_config=config_psql_dos(), broker_backend='core.rabbitmq'
+        aiida_config, storage_backend='core.sqlite_dos', storage_config=config_sqlite_dos(), broker_backend='core.rabbitmq'
     ) as profile:
         yield profile

agoscinski · 2024-05-29T13:46:27Z

Thanks for the PR! This would be super useful for running tests in tox locally. Will try it out.

danielhollas · 2024-05-29T13:47:49Z

@agoscinski thanks for taking a look! I will be working on finishing this up today so you might want to wait with testing till tomorrow.

tests/storage/psql_dos/migrations/conftest.py

codecov · 2024-05-29T16:14:46Z

Codecov Report

Attention: Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.80%. Comparing base (ef60b66) to head (4ba5e09).
Report is 112 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/tools/pytest_fixtures/storage.py	92.60%	2 Missing ⚠️
...ida/storage/psql_dos/migrations/utils/integrity.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6425      +/-   ##
==========================================
+ Coverage   77.51%   77.80%   +0.30%     
==========================================
  Files         560      562       +2     
  Lines       41444    41788     +344     
==========================================
+ Hits        32120    32511     +391     
+ Misses       9324     9277      -47

Flag	Coverage Δ
presto	`73.14% <70.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhollas · 2024-05-29T21:03:13Z

@agoscinski to fix the SSH errors in your local setup, take a look at the setup_ssh.sh script that I extracted from .github/workflows/setup.sh: https://github.com/aiidateam/aiida-core/pull/6425/files#diff-55611b6e2b8ad89832bc6695fffc24acdcc8f34bfb2d086abf01b110cfa33e01

Note that before running ssh-keyscan I had to restart the SSH Daemon with systemctl restart sshd. You might need to do something similar on your system.

danielhollas · 2024-05-29T21:09:22Z

tests/engine/test_process_function.py

@@ -439,6 +439,10 @@ def test_launchers():
    assert result == get_true_node()
    assert isinstance(node, orm.CalcFunctionNode)

+
+@pytest.mark.requires_rmq


This test fails in rabbitmq tests, I don't know why. Perhaps something else from setup.sh is needed?

FAILED tests/engine/test_process_function.py::test_submit_launchers - kiwipy.exceptions.UnroutableError: ('NO_ROUTE', 'aiida-d201dba33ae749d8a63dce82958ca171.process.queue')

https://github.com/aiidateam/aiida-core/actions/runs/9289159100/job/25562353186?pr=6425

It's now working, although I have no idea what I changed that made this work.

This is just a transient problem that props up now and again on GHA.

danielhollas · 2024-05-29T21:10:18Z

tests/orm/test_querybuilder.py

@sphuber do all these requires_pgsql make sense to you in this query builder test suite?

Kind of. Either has datetime comparisons, uses the has_key or contains filters, or relies on comparing literal SQL, which is of course not going to be identical between psql and sqlite. Don't understand why the test_iterall_* tests would take so long. That seems like a smell that might be worth looking into further. These tests are also related to transaction handling, so again the transaction implementation might be causing some trouble

src/aiida/tools/pytest_fixtures/daemon.py

danielhollas · 2024-05-29T21:12:44Z

@sphuber @agoscinski apart from two small TODOs this is now ready for review / testing.

EDIT: Both TODOs are now resolved.

sphuber · 2024-05-29T21:18:14Z

tests/orm/nodes/test_node.py

+    # TODO: Why is this failing for SQLite??
+    # sqlalchemy.orm.exc.ObjectDeletedError: Instance '<DbNode at 0x7f6a68845990>' has been deleted,
+    # or its row is otherwise not present.
+    @pytest.mark.requires_pgsql


My first guess would be that it might have something to do with the backend.transaction() call on 863. Not sure if this is directly inherited from the psql_dos implementation but transactions may work different in sqlite. I would start looking there first

I won't have time for digging into this probably. I'll create a separate issue.

That is fine, but we will want to prioritize this because to me it suggests there might be a bug lurking with node deletion for sqlite profiles. It is very likely that this hasn't been tested properly yet.

Opened #6436

.github/workflows/ci-code.yml

requirements/requirements-py-3.10.txt

danielhollas · 2024-05-30T19:21:00Z

tests/cmdline/commands/test_presto.py

-@pytest.mark.parametrize('with_broker', (True, False))
-def test_presto(run_cli_command, with_broker, monkeypatch):
-    """Test the ``verdi presto``."""
+def test_presto_without_rmq(pytestconfig, run_cli_command, monkeypatch):


I think it ended up being cleaner to split this test into two instead of parametrizing it.

Meh, I see a lot of code duplication, but fine, I will let it slide :D

Here's the alternative

@pytest.mark.usefixtures('empty_config') @pytest.mark.parametrize('with_broker', (pytest.param(True, marks=pytest.mark.requires_rmq), False)) def test_presto(pytestconfig, run_cli_command, with_broker, monkeypatch): """Test the ``verdi presto``.""" from aiida.brokers.rabbitmq import defaults if not with_broker: # Patch the RabbitMQ detection function to pretend it could not find the service monkeypatch.setattr(defaults, 'detect_rabbitmq_config', lambda: None) result = run_cli_command(verdi_presto) assert 'Created new profile `presto`.' in result.output with profile_context('presto', allow_switch=True) as profile: assert profile.name == 'presto' localhost = Computer.collection.get(label='localhost') assert localhost.is_configured if with_broker: assert profile.process_control_backend == 'core.rabbitmq' else: assert profile.process_control_backend is None

All I see is if statements and complexity. 😛 (but honestly both feel fine)

tests/conftest.py

danielhollas · 2024-05-30T19:29:54Z

Why does every PR that touched GHA end up with tens of commits? 😭

@sphuber this should now be ready. There might be some controversial changes to the test suite, happy to discuss. (see the updated OP)

sphuber

Thanks a lot @danielhollas Seems that the Lord finally answered your prayers and made the tests pass. Changes seem mostly good, just a few smaller comments

.github/workflows/ci-code.yml

sphuber · 2024-05-30T19:43:58Z

.github/workflows/ci-code.yml

+      run: pytest -m 'presto'
+
+
+  tests-sqlite:


You already mentioned in the OP that having this one as well might be overkill. What does this add on top of the tests and tests-presto? It is really just adding tests being run with SQLite and RMQ, right? I doubt there would be many situations where this combination is relevant. So I would be tempted to leave this one out for now.

It is really just adding tests being run with SQLite and RMQ, right? I doubt there would be many situations where this combination is relevant.

Indeed. The idea was to not really test this combination per se, but to increase the code coverage when running with sqlite backend, under the assumption that many of the tests requiring rmq will also exercise the DB backend. Another minor advantage would be that it would ensure that the not requires_psql marker keeps working.

I've temporarily enabled code coverage for presto and sqlite runs. The overall coverage for full tests suite is 77.8%, for presto it's 73.1% and 76.5% for sqlite.

#6425 (comment)

I am fine either way. We could also achieve similar outcome by running the nightly rabbitmq tests with sqlite backend. WDYT?

I will remove tests-sqlite for now since it seems like little coverage gain for a significant amount of resource cost.

sphuber · 2024-05-30T19:46:44Z

src/aiida/tools/pytest_fixtures/storage.py

@@ -52,6 +53,8 @@ def create_database(
        cluster = PGTest()
        cluster.create_database = create_database
        yield cluster
+    except OSError:


When does this happen?

This is what I am getting (don't have Postgres globally installed

OSError: b'initdb: error: file "/opt/conda/envs/aiida-core-services/share/postgres.bki" does not exist\ninitdb: hint: This might mean you have a corrupted installation or identified the wrong directory with the invocation option -L.\npg_ctl: database system initialization failed\n'

sphuber · 2024-05-30T19:56:38Z

src/aiida/tools/pytest_fixtures/storage.py

+        if not postgres_cluster:
+            msg = 'Could not initialize PostgreSQL cluster'


Why not raise this in the postgres_cluster fixture itself when you catch the OSError?

Good point. I guess I was trying to reduce the depth of stacktrace that pytest vomits when this happens. But it could hide what is the actual reason for the error. I've changed it.

I take that back --- this was a load-bearing hack, because this fixture is evaluated eagerly and before we could decide we want sqlite_dos backend in aiida_profile. I'd need to think about a nicer solution than this since this.

a load-bearing hack

🤣 I love it
That makes sense that you have to fix it this way, but I wonder what will happen if someone uses the postgres_cluster fixture. They will just get None instead of an exception which won't be immediately obvious. Please just comment the reason for the OSError and add a line in the docstring when it will return None.

Yeah, since this is a public fixture the hack is really not ideal. Since this is a new fixture that hasn't been released yet, should we try to change its API so that the cluster is created only when really needed?

If you have an idea of how, then yes I think that would be preferable

I tried to refactor by encapsulating the cluster in a class / object. LMK if it makes sense.
If the only thing that users of this fixtures are supposed to interact with is the create_database method, then I think this new approach makes sense.

tests/cmdline/commands/test_calcjob.py

sphuber · 2024-05-30T20:07:48Z

tests/engine/test_process_function.py

@@ -439,6 +439,10 @@ def test_launchers():
    assert result == get_true_node()
    assert isinstance(node, orm.CalcFunctionNode)

+
+@pytest.mark.requires_rmq


This is just a transient problem that props up now and again on GHA.

sphuber · 2024-05-30T20:09:03Z

tests/orm/nodes/test_node.py

+    # TODO: Why is this failing for SQLite??
+    # sqlalchemy.orm.exc.ObjectDeletedError: Instance '<DbNode at 0x7f6a68845990>' has been deleted,
+    # or its row is otherwise not present.
+    @pytest.mark.requires_pgsql


That is fine, but we will want to prioritize this because to me it suggests there might be a bug lurking with node deletion for sqlite profiles. It is very likely that this hasn't been tested properly yet.

tests/restapi/test_statistics.py

tests/tools/archive/test_schema.py

tests/conftest.py

The unit test suite currently automatically creates a test profile using the `core.psql_dos` storage plugin. The reason is mostly historical as originally the `core.psql_dos` storage was the only. The downside of this storage plugin, however, is that it requires a PostgreSQL database. This typically requires a PostgreSQL service to be running, making it more difficult to get a working development environment. This was made slightly easier through the use of `pgtest`, which would create a PSQL cluster on-the-fly, but this would still require postgres libraries to be installed. Nowadays, storage plugins are customizable and the `core.sqlite_dos` replaces PostgreSQL with the serviceless SQLite. The dependencies for these are a lot easier to install, making it perfect for testing. However, the test suite automatically configures a `core.psql_dos` profile and some tests are not compatible with SQLite since they directly depend on functionality that is only supported by PSQL and not SQLite. In this commit, changes are made that allow running the test with SQLite. Two new pytest markers are added to the test suite: * `requires_pgsql`: for tests that are known to require PostgreSQL (i.e. do not work with SQLite) * `presto`: an alias for `not requires_rmq and not requires_pgsql`. The tests are marked with `presto` automagically in `tests/conftest.py:pytest_collection_modifyitems` A new CI job is added `test-presto` that runs the tests with the `presto` marker.

sphuber

Thanks a lot @danielhollas

The unit test suite currently automatically creates a test profile using the `core.psql_dos` storage plugin. The reason is mostly historical as originally the `core.psql_dos` storage was the only. The downside of this storage plugin, however, is that it requires a PostgreSQL database. This typically requires a PostgreSQL service to be running, making it more difficult to get a working development environment. This was made slightly easier through the use of `pgtest`, which would create a PSQL cluster on-the-fly, but this would still require postgres libraries to be installed. Nowadays, storage plugins are customizable and the `core.sqlite_dos` replaces PostgreSQL with the serviceless SQLite. The dependencies for these are a lot easier to install, making it perfect for testing. However, the test suite automatically configures a `core.psql_dos` profile and some tests are not compatible with SQLite since they directly depend on functionality that is only supported by PSQL and not SQLite. In this commit, changes are made that allow running the test with SQLite. Two new pytest markers are added to the test suite: * `requires_pgsql`: for tests that are known to require PostgreSQL (i.e. do not work with SQLite) * `presto`: an alias for `not requires_rmq and not requires_pgsql`. The tests are marked with `presto` automagically in `tests/conftest.py:pytest_collection_modifyitems` A new CI job is added `test-presto` that runs the tests with the `presto` marker.

danielhollas force-pushed the sqlite-tests branch from 1d32135 to ce0a9a4 Compare May 29, 2024 05:05

danielhollas changed the title ~~WIP: [devops] Run tests with sqlite backend~~ [devops] Run tests with sqlite backend May 29, 2024

agoscinski self-assigned this May 29, 2024

danielhollas commented May 29, 2024

View reviewed changes

tests/storage/psql_dos/migrations/conftest.py Show resolved Hide resolved

danielhollas marked this pull request as ready for review May 29, 2024 15:45

danielhollas commented May 29, 2024

View reviewed changes

src/aiida/tools/pytest_fixtures/daemon.py Outdated Show resolved Hide resolved

danielhollas requested review from sphuber and agoscinski May 29, 2024 21:12

sphuber reviewed May 29, 2024

View reviewed changes

danielhollas commented May 30, 2024

View reviewed changes

.github/workflows/ci-code.yml Show resolved Hide resolved

danielhollas commented May 30, 2024

View reviewed changes

requirements/requirements-py-3.10.txt Show resolved Hide resolved

danielhollas commented May 30, 2024

View reviewed changes

tests/conftest.py Show resolved Hide resolved

danielhollas requested a review from sphuber May 30, 2024 19:29

sphuber requested changes May 30, 2024

View reviewed changes

danielhollas mentioned this pull request May 31, 2024

Feature parity between PostgreSQL and SQLite backends #6435

Open

danielhollas assigned danielhollas and unassigned agoscinski May 31, 2024

danielhollas mentioned this pull request May 31, 2024

test_delete_through_backend fails with SQLite: sqlalchemy.orm.exc.ObjectDeletedError #6436

Closed

sphuber force-pushed the sqlite-tests branch 2 times, most recently from dc002b4 to 9fae660 Compare June 4, 2024 15:35

sphuber force-pushed the sqlite-tests branch from 9fae660 to 4ba5e09 Compare June 4, 2024 18:42

sphuber self-requested a review June 4, 2024 20:21

sphuber approved these changes Jun 4, 2024

View reviewed changes

sphuber merged commit 0dc8bbc into aiidateam:main Jun 4, 2024
39 checks passed

danielhollas deleted the sqlite-tests branch June 4, 2024 22:45

agoscinski mentioned this pull request Jun 5, 2024

Devops: Add tox environment that runs tests with sqlite backend #6450

Merged

		if not postgres_cluster:
		msg = 'Could not initialize PostgreSQL cluster'

[devops] Run tests with sqlite backend #6425

[devops] Run tests with sqlite backend #6425

Conversation

danielhollas commented May 27, 2024 • edited Loading

sphuber commented May 29, 2024

agoscinski commented May 29, 2024 • edited Loading

danielhollas commented May 29, 2024

codecov bot commented May 29, 2024 • edited Loading

Codecov Report

danielhollas commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber May 29, 2024 • edited Loading

Choose a reason for hiding this comment

danielhollas commented May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas May 31, 2024 • edited Loading

Choose a reason for hiding this comment

danielhollas commented May 30, 2024

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

danielhollas commented May 27, 2024 •

edited

Loading

agoscinski commented May 29, 2024 •

edited

Loading

codecov bot commented May 29, 2024 •

edited

Loading

sphuber May 29, 2024 •

edited

Loading

danielhollas commented May 29, 2024 •

edited

Loading

danielhollas May 31, 2024 •

edited

Loading