From a863d1e887822334df4db9120421cd38ded04d3e Mon Sep 17 00:00:00 2001 From: Daniel Hollas Date: Mon, 25 Nov 2024 10:22:42 +0000 Subject: [PATCH 1/2] Tests: add --db-backend pytest option (#6625) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of choosing the database backend (SQLite or Postgres) implicitly via markers, we add an explicit `--db-backend ` option to pytest. ``` --db-backend=DB_BACKEND Database backend to be used for tests ('sqlite', 'psql') ``` - The default backend was changed from `psql` to `sqlite` to make local testing easier. - The tests running in CI are still using Postgres in most places. - The only change in CI should be switching the rabbitmq tests and release tests to use sqlite. Error if the user provides an invalid option ``` ❯ pytest --db-backend=invalid ERROR: Invalid --db-backend option 'invalid' Must be one of: ('sqlite', 'psql') ``` To clarify, the existing presto marker works as before. --- .github/workflows/benchmark.yml | 4 +- .github/workflows/ci-code.yml | 4 +- .github/workflows/nightly.yml | 18 +----- .github/workflows/release.yml | 20 +----- .github/workflows/test-install.yml | 2 +- .github/workflows/tests_nightly.sh | 2 +- tests/conftest.py | 52 +++++++++++++-- tests/storage/psql_dos/conftest.py | 13 ++-- .../django_branch/test_migrate_to_head.py | 9 +++ tests/storage/psql_dos/test_backend.py | 8 +++ tests/test_markers.py | 64 +++++++++++++++++++ 11 files changed, 141 insertions(+), 55 deletions(-) create mode 100644 tests/test_markers.py diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index a2a42687b1..5bb4b61c55 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -56,7 +56,7 @@ jobs: python-version: '3.10' - name: Run benchmarks - run: pytest --benchmark-only --benchmark-json benchmark.json + run: pytest --db-backend psql --benchmark-only --benchmark-json benchmark.json tests/ - name: Store benchmark result uses: aiidateam/github-action-benchmark@v3 @@ -73,4 +73,4 @@ jobs: alert-threshold: 200% comment-on-alert: true fail-on-alert: false - alert-comment-cc-users: '@chrisjsewell,@giovannipizzi' + alert-comment-cc-users: '@giovannipizzi,@agoscinski,@GeigerJ2,@khsrali,@unkcpz' diff --git a/.github/workflows/ci-code.yml b/.github/workflows/ci-code.yml index ff95047bb4..dfa3cc787c 100644 --- a/.github/workflows/ci-code.yml +++ b/.github/workflows/ci-code.yml @@ -104,7 +104,7 @@ jobs: AIIDA_WARN_v3: 1 # Python 3.12 has a performance regression when running with code coverage # so run code coverage only for python 3.9. - run: pytest -v tests -m 'not nightly' ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} + run: pytest -v --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} - name: Upload coverage report if: matrix.python-version == 3.9 && github.repository == 'aiidateam/aiida-core' @@ -139,7 +139,7 @@ jobs: - name: Run test suite env: AIIDA_WARN_v3: 0 - run: pytest -m 'presto' + run: pytest -m 'presto' tests/ verdi: diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index c7ea8cb787..9217e534f8 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -104,19 +104,6 @@ jobs: rabbitmq-version: ['3.11', '3.12', '3.13'] services: - postgres: - image: postgres:16 - env: - POSTGRES_DB: test_aiida - POSTGRES_PASSWORD: '' - POSTGRES_HOST_AUTH_METHOD: trust - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 rabbitmq: image: rabbitmq:${{ matrix.rabbitmq-version }}-management ports: @@ -132,9 +119,6 @@ jobs: with: python-version: '3.11' - - name: Install system dependencies - run: sudo apt update && sudo apt install postgresql - - name: Setup SSH on localhost run: .github/workflows/setup_ssh.sh @@ -145,7 +129,7 @@ jobs: id: tests env: AIIDA_WARN_v3: 0 - run: pytest -sv -m 'requires_rmq' + run: pytest -sv --db-backend sqlite -m 'requires_rmq' tests/ - name: Slack notification # Always run this step (otherwise it would be skipped if any of the previous steps fail) but only if the diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 80f7e35326..524a625afc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -54,19 +54,6 @@ jobs: timeout-minutes: 30 services: - postgres: - image: postgres:10 - env: - POSTGRES_DB: test_aiida - POSTGRES_PASSWORD: '' - POSTGRES_HOST_AUTH_METHOD: trust - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 rabbitmq: image: rabbitmq:3.8.14-management ports: @@ -76,16 +63,11 @@ jobs: steps: - uses: actions/checkout@v4 - - name: Install system dependencies - run: | - sudo apt update - sudo apt install postgresql graphviz - - name: Install aiida-core uses: ./.github/actions/install-aiida-core - name: Run sub-set of test suite - run: pytest -sv -k 'requires_rmq' + run: pytest -sv -m requires_rmq --db-backend=sqlite tests/ publish: diff --git a/.github/workflows/test-install.yml b/.github/workflows/test-install.yml index 2f919a8c99..75371449bb 100644 --- a/.github/workflows/test-install.yml +++ b/.github/workflows/test-install.yml @@ -229,7 +229,7 @@ jobs: env: AIIDA_TEST_PROFILE: test_aiida AIIDA_WARN_v3: 1 - run: pytest --verbose tests -m 'not nightly' + run: pytest -v --db-backend psql tests -m 'not nightly' tests/ - name: Freeze test environment run: pip freeze | sed '1d' | tee requirements-py-${{ matrix.python-version }}.txt diff --git a/.github/workflows/tests_nightly.sh b/.github/workflows/tests_nightly.sh index 10f26f7f15..dbb1b92a8a 100755 --- a/.github/workflows/tests_nightly.sh +++ b/.github/workflows/tests_nightly.sh @@ -13,4 +13,4 @@ verdi -p test_aiida run ${SYSTEM_TESTS}/test_containerized_code.py bash ${SYSTEM_TESTS}/test_polish_workchains.sh verdi daemon stop -AIIDA_TEST_PROFILE=test_aiida pytest -v tests -m 'nightly' +AIIDA_TEST_PROFILE=test_aiida pytest -v --db-backend psql -m nightly tests/ diff --git a/tests/conftest.py b/tests/conftest.py index 2166cc06c0..fb6780d966 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,7 @@ import types import typing as t import warnings +from enum import Enum from pathlib import Path import click @@ -37,7 +38,14 @@ pytest_plugins = ['aiida.tools.pytest_fixtures', 'sphinx.testing.fixtures'] -def pytest_collection_modifyitems(items): +class TestDbBackend(Enum): + """Options for the '--db-backend' CLI argument when running pytest.""" + + SQLITE = 'sqlite' + PSQL = 'psql' + + +def pytest_collection_modifyitems(items, config): """Automatically generate markers for certain tests. Most notably, we add the 'presto' marker for all tests that @@ -47,6 +55,14 @@ def pytest_collection_modifyitems(items): filepath_django = Path(__file__).parent / 'storage' / 'psql_dos' / 'migrations' / 'django_branch' filepath_sqla = Path(__file__).parent / 'storage' / 'psql_dos' / 'migrations' / 'sqlalchemy_branch' + # If the user requested the SQLite backend, automatically skip incompatible tests + if config.option.db_backend is TestDbBackend.SQLITE: + if config.option.markexpr != '': + # Don't overwrite markers that the user already provided via '-m ' cmdline argument + config.option.markexpr += ' and (not requires_psql)' + else: + config.option.markexpr = 'not requires_psql' + for item in items: filepath_item = Path(item.fspath) @@ -68,6 +84,30 @@ def pytest_collection_modifyitems(items): item.add_marker('presto') +def db_backend_type(string): + """Conversion function for the custom '--db-backend' pytest CLI option + + :param string: String provided by the user via CLI + :returns: DbBackend enum corresponding to user input string + """ + try: + return TestDbBackend(string) + except ValueError: + msg = f"Invalid --db-backend option '{string}'\nMust be one of: {tuple(db.value for db in TestDbBackend)}" + raise pytest.UsageError(msg) + + +def pytest_addoption(parser): + parser.addoption( + '--db-backend', + action='store', + default=TestDbBackend.SQLITE, + required=False, + help=f'Database backend to be used for tests {tuple(db.value for db in TestDbBackend)}', + type=db_backend_type, + ) + + @pytest.fixture(scope='session') def aiida_profile(pytestconfig, 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. @@ -77,18 +117,22 @@ def aiida_profile(pytestconfig, aiida_config, aiida_profile_factory, config_psql be run against the main storage backend, which is ``core.sqlite_dos``. """ marker_opts = pytestconfig.getoption('-m') + db_backend = pytestconfig.getoption('--db-backend') - # By default we use RabbitMQ broker and psql_dos storage + # We use RabbitMQ broker by default unless 'presto' marker is specified broker = 'core.rabbitmq' if 'not requires_rmq' in marker_opts or 'presto' in marker_opts: broker = None - if 'not requires_psql' in marker_opts or 'presto' in marker_opts: + if db_backend is TestDbBackend.SQLITE: storage = 'core.sqlite_dos' config = config_sqlite_dos() - else: + elif db_backend is TestDbBackend.PSQL: storage = 'core.psql_dos' config = config_psql_dos() + else: + # This should be unreachable + raise ValueError(f'Invalid DB backend {db_backend}') with aiida_profile_factory( aiida_config, storage_backend=storage, storage_config=config, broker_backend=broker diff --git a/tests/storage/psql_dos/conftest.py b/tests/storage/psql_dos/conftest.py index 16136b8df9..c24db0ac72 100644 --- a/tests/storage/psql_dos/conftest.py +++ b/tests/storage/psql_dos/conftest.py @@ -13,17 +13,12 @@ from aiida.common.exceptions import MissingConfigurationError from aiida.manage.configuration import get_config +STORAGE_BACKEND_ENTRY_POINT = None try: if test_profile := os.environ.get('AIIDA_TEST_PROFILE'): STORAGE_BACKEND_ENTRY_POINT = get_config().get_profile(test_profile).storage_backend - # TODO: The else branch is wrong - else: - STORAGE_BACKEND_ENTRY_POINT = 'core.psql_dos' -except MissingConfigurationError: - # TODO: This is actually not true anymore! - # Case when ``pytest`` is invoked without existing config, in which case it will rely on the automatic test profile - # creation which currently always uses ``core.psql_dos`` for the storage backend - STORAGE_BACKEND_ENTRY_POINT = 'core.psql_dos' +except MissingConfigurationError as e: + raise ValueError(f"Could not parse configuration of AiiDA test profile '{test_profile}'") from e -if STORAGE_BACKEND_ENTRY_POINT != 'core.psql_dos': +if STORAGE_BACKEND_ENTRY_POINT is not None and STORAGE_BACKEND_ENTRY_POINT != 'core.psql_dos': collect_ignore_glob = ['*'] diff --git a/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py b/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py index 69c7e643d9..11f9fb8c3a 100644 --- a/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py +++ b/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py @@ -11,6 +11,15 @@ from aiida.storage.psql_dos.migrator import PsqlDosMigrator +def test_all_tests_marked_as_nightly(request): + """Test that all tests in this folder are tagged with 'nightly' marker""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 2 + assert 'nightly' in own_markers + assert 'requires_psql' in own_markers + + def test_migrate(perform_migrations: PsqlDosMigrator): """Test that the migrator can migrate from the base of the django branch, to the main head.""" perform_migrations.migrate_up('django@django_0001') # the base of the django branch diff --git a/tests/storage/psql_dos/test_backend.py b/tests/storage/psql_dos/test_backend.py index 6fe35ac747..bbc77b1a14 100644 --- a/tests/storage/psql_dos/test_backend.py +++ b/tests/storage/psql_dos/test_backend.py @@ -13,6 +13,14 @@ from aiida.orm import User +def test_all_tests_marked_with_requires_psql(request): + """Test that all tests in this folder are marked with 'requires_psql'""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_psql' + + @pytest.mark.usefixtures('aiida_profile_clean') def test_default_user(): assert isinstance(get_manager().get_profile_storage().default_user, User) diff --git a/tests/test_markers.py b/tests/test_markers.py new file mode 100644 index 0000000000..d9d6e63871 --- /dev/null +++ b/tests/test_markers.py @@ -0,0 +1,64 @@ +"""Tests markers that have custom in conftest.py""" + +import pytest + + +def test_presto_auto_mark(request): + """Test that the presto marker is added automatically""" + own_markers = [marker.name for marker in request.node.own_markers] + assert len(own_markers) == 1 + assert own_markers[0] == 'presto' + + +@pytest.mark.sphinx +def test_presto_mark_and_another_mark(request): + """Test that presto marker is added even if there is an existing marker (except requires_rmq|psql)""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 2 + assert 'presto' in own_markers + assert 'sphinx' in own_markers + + +@pytest.mark.requires_rmq +def test_no_presto_mark_if_rmq(request): + """Test that presto marker is NOT added if the test is mark with "requires_rmq""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_rmq' + + +@pytest.mark.requires_psql +def test_no_presto_mark_if_psql(request): + """Test that presto marker is NOT added if the test is mark with "requires_psql""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_psql' + + +@pytest.mark.nightly +def test_no_presto_mark_if_nightly(request): + """Test that presto marker is NOT added if the test is mark with "requires_psql""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'nightly' + + +@pytest.mark.requires_psql +def test_requires_psql_with_sqlite_impossible(pytestconfig): + db_backend = pytestconfig.getoption('--db-backend') + if db_backend.value == 'sqlite': + pytest.fail('This test should not have been executed with SQLite backend!') + + +def test_daemon_client_fixture_automarked(request, daemon_client): + """Test that any test using ``daemon_client`` fixture is + automatically tagged with 'requires_rmq' mark + """ + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_rmq' From c93fb4f75554802e46fdcb7cf8caf27318ad04d0 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Mon, 25 Nov 2024 16:12:32 +0100 Subject: [PATCH 2/2] Bump mypy version to ~=1.13.0 (#6630) --- pyproject.toml | 2 +- src/aiida/common/lang.py | 2 +- src/aiida/common/pydantic.py | 2 +- src/aiida/engine/processes/calcjobs/calcjob.py | 2 +- src/aiida/engine/processes/workchains/restart.py | 2 +- src/aiida/manage/manager.py | 2 +- src/aiida/orm/nodes/data/array/projection.py | 2 +- src/aiida/orm/nodes/data/list.py | 4 ++-- src/aiida/orm/nodes/data/singlefile.py | 4 +--- src/aiida/plugins/utils.py | 2 +- src/aiida/tools/pytest_fixtures/daemon.py | 2 +- tests/cmdline/groups/test_dynamic.py | 2 +- 12 files changed, 13 insertions(+), 15 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3768fcb05c..6b3f630e1a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -211,7 +211,7 @@ notebook = [ ] pre-commit = [ 'aiida-core[atomic_tools,rest,tests,tui]', - 'mypy~=1.10.0', + 'mypy~=1.13.0', 'packaging~=23.0', 'pre-commit~=3.5', 'sqlalchemy[mypy]~=2.0', diff --git a/src/aiida/common/lang.py b/src/aiida/common/lang.py index ec9fb45ddb..6a8f4d3d5b 100644 --- a/src/aiida/common/lang.py +++ b/src/aiida/common/lang.py @@ -96,4 +96,4 @@ def __init__(self, getter: Callable[[SelfType], ReturnType]) -> None: self.getter = getter def __get__(self, instance: Any, owner: SelfType) -> ReturnType: - return self.getter(owner) + return self.getter(owner) # type: ignore[arg-type] diff --git a/src/aiida/common/pydantic.py b/src/aiida/common/pydantic.py index 633e83428d..2fa3d2bd68 100644 --- a/src/aiida/common/pydantic.py +++ b/src/aiida/common/pydantic.py @@ -41,7 +41,7 @@ class Model(BaseModel): field_info = Field(default, **kwargs) for key, value in (('priority', priority), ('short_name', short_name), ('option_cls', option_cls)): - if value is not None: + if value is not None and field_info is not None: field_info.metadata.append({key: value}) return field_info diff --git a/src/aiida/engine/processes/calcjobs/calcjob.py b/src/aiida/engine/processes/calcjobs/calcjob.py index d5acfca5fc..8ced783a5f 100644 --- a/src/aiida/engine/processes/calcjobs/calcjob.py +++ b/src/aiida/engine/processes/calcjobs/calcjob.py @@ -1062,7 +1062,7 @@ def presubmit(self, folder: Folder) -> CalcInfo: def encoder(obj): if dataclasses.is_dataclass(obj): - return dataclasses.asdict(obj) + return dataclasses.asdict(obj) # type: ignore[arg-type] raise TypeError(f' {obj!r} is not JSON serializable') subfolder = folder.get_subfolder('.aiida', create=True) diff --git a/src/aiida/engine/processes/workchains/restart.py b/src/aiida/engine/processes/workchains/restart.py index ad5cd8a181..34544704f2 100644 --- a/src/aiida/engine/processes/workchains/restart.py +++ b/src/aiida/engine/processes/workchains/restart.py @@ -29,7 +29,7 @@ def validate_handler_overrides( - process_class: 'BaseRestartWorkChain', handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace' + process_class: type['BaseRestartWorkChain'], handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace' ) -> Optional[str]: """Validator for the ``handler_overrides`` input port of the ``BaseRestartWorkChain``. diff --git a/src/aiida/manage/manager.py b/src/aiida/manage/manager.py index 8621b324f4..651190454e 100644 --- a/src/aiida/manage/manager.py +++ b/src/aiida/manage/manager.py @@ -430,7 +430,7 @@ def create_runner(self, with_persistence: bool = True, **kwargs: Any) -> 'Runner if with_persistence and 'persister' not in settings: settings['persister'] = self.get_persister() - return runners.Runner(**settings) + return runners.Runner(**settings) # type: ignore[arg-type] def create_daemon_runner(self, loop: Optional['asyncio.AbstractEventLoop'] = None) -> 'Runner': """Create and return a new daemon runner. diff --git a/src/aiida/orm/nodes/data/array/projection.py b/src/aiida/orm/nodes/data/array/projection.py index e58442d9c5..881ac727c2 100644 --- a/src/aiida/orm/nodes/data/array/projection.py +++ b/src/aiida/orm/nodes/data/array/projection.py @@ -278,7 +278,7 @@ def array_list_checker(array_list, array_name, orb_length): raise exceptions.ValidationError('Tags must set a list of strings') self.base.attributes.set('tags', tags) - def set_orbitals(self, **kwargs): + def set_orbitals(self, **kwargs): # type: ignore[override] """This method is inherited from OrbitalData, but is blocked here. If used will raise a NotImplementedError """ diff --git a/src/aiida/orm/nodes/data/list.py b/src/aiida/orm/nodes/data/list.py index 9fdd6ec866..fc39dd1acd 100644 --- a/src/aiida/orm/nodes/data/list.py +++ b/src/aiida/orm/nodes/data/list.py @@ -81,7 +81,7 @@ def remove(self, value): self.set_list(data) return item - def pop(self, **kwargs): + def pop(self, **kwargs): # type: ignore[override] """Remove and return item at index (default last).""" data = self.get_list() item = data.pop(**kwargs) @@ -89,7 +89,7 @@ def pop(self, **kwargs): self.set_list(data) return item - def index(self, value): + def index(self, value): # type: ignore[override] """Return first index of value..""" return self.get_list().index(value) diff --git a/src/aiida/orm/nodes/data/singlefile.py b/src/aiida/orm/nodes/data/singlefile.py index c0f3797f24..8faefc8cb4 100644 --- a/src/aiida/orm/nodes/data/singlefile.py +++ b/src/aiida/orm/nodes/data/singlefile.py @@ -71,9 +71,7 @@ def open(self, path: FilePath, mode: t.Literal['rb']) -> t.Iterator[t.BinaryIO]: @t.overload @contextlib.contextmanager - def open( # type: ignore[overload-overlap] - self, path: None = None, mode: t.Literal['r'] = ... - ) -> t.Iterator[t.TextIO]: ... + def open(self, path: None = None, mode: t.Literal['r'] = ...) -> t.Iterator[t.TextIO]: ... @t.overload @contextlib.contextmanager diff --git a/src/aiida/plugins/utils.py b/src/aiida/plugins/utils.py index c284b25912..7d1e16a363 100644 --- a/src/aiida/plugins/utils.py +++ b/src/aiida/plugins/utils.py @@ -38,7 +38,7 @@ def __init__(self): def logger(self) -> Logger: return self._logger - def get_version_info(self, plugin: str | type) -> dict[t.Any, dict[t.Any, t.Any]]: + def get_version_info(self, plugin: str | t.Any) -> dict[t.Any, dict[t.Any, t.Any]]: """Get the version information for a given plugin. .. note:: diff --git a/src/aiida/tools/pytest_fixtures/daemon.py b/src/aiida/tools/pytest_fixtures/daemon.py index 74e3620193..2b74e4ce77 100644 --- a/src/aiida/tools/pytest_fixtures/daemon.py +++ b/src/aiida/tools/pytest_fixtures/daemon.py @@ -116,7 +116,7 @@ def test(submit_and_await): from aiida.engine import ProcessState def factory( - submittable: 'Process' | 'ProcessBuilder' | 'ProcessNode', + submittable: type[Process] | ProcessBuilder | ProcessNode | t.Any, state: ProcessState = ProcessState.FINISHED, timeout: int = 20, **kwargs, diff --git a/tests/cmdline/groups/test_dynamic.py b/tests/cmdline/groups/test_dynamic.py index 3c741b8fe2..48c18f0941 100644 --- a/tests/cmdline/groups/test_dynamic.py +++ b/tests/cmdline/groups/test_dynamic.py @@ -17,7 +17,7 @@ class Model(BaseModel): union_type: t.Union[int, float] = Field(title='Union type') without_default: str = Field(title='Without default') with_default: str = Field(title='With default', default='default') - with_default_factory: str = Field(title='With default factory', default_factory=lambda: True) + with_default_factory: str = Field(title='With default factory', default_factory=lambda: True) # type: ignore[assignment] def test_list_options(entry_points):