Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add improved more configurable versions of pytest fixtures #6341

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 7, 2024

The pytest fixtures are intended to help plugin packages to easily
write unit tests. Arguably the most important fixture is the
aiida_profile fixture which automatically provides a ready-to-go
profile. The downside is that it uses the core.psql_dos storage
backend, which was historically the only available storage.

Now there are other storage plugins available. Not only would it be
useful to allow a user to easily configure which storage plugin to use
for the test profile, it would make sense to change the default from
core.psql_dos to a storage plugin that doesn't require a PostgreSQL
database. Sure, the plugin currently uses pgtest to create a test
cluster on the fly so the test database is created on the fly and
doesn't affect production databases, however, it still does require the
PostgreSQL libraries to be installed, or the pg_ctl binary won't be
found and the fixture fails.

The aiida_profile fixture now instead uses the core.sqlite_dos by
default and configures to use no broker, which also means that RabbitMQ
is no longer needed. This makes it possible to run the tests by default
without any services running, making it much easier to get started
running tests on any environment that just has aiida-core installed.

@sphuber sphuber force-pushed the feature/improved-profile-fixtures branch 12 times, most recently from ad35abd to 68a3dbf Compare April 8, 2024 21:26
@sphuber sphuber marked this pull request as ready for review April 9, 2024 05:37
@sphuber sphuber force-pushed the feature/improved-profile-fixtures branch from 418c6ad to 724582b Compare April 9, 2024 08:42
@sphuber
Copy link
Contributor Author

sphuber commented Apr 9, 2024

This is now ready for review. I have been testing the new interface with some plugins to confirm that it is relatively easy to customize the various fixtures (mostly the ones that provide the test profile):

  • aiida-pseudo: Use the new aiida_profile as is, allowing to get rid of having the PostgreSQL and RabbitMQ services altogether
  • aiida-shell: Runs tests with the daemon so customizes aiida_profile to configure RabbitMQ as the broker:
@pytest.fixture(scope='session', autouse=True)
def aiida_profile(aiida_config, aiida_profile_factory):
    """Create and load a profile with RabbitMQ as broker."""
    with aiida_profile_factory(aiida_config, broker_backend='core.rabbitmq') as profile:

        yield profile
  • aiida-s3: Uses the aiida_profile_factory to easily create test profiles with different storage plugins.
@pytest.fixture(scope='session')
def psql_s3_profile(aiida_config, aiida_profile_factory, config_psql_s3) -> t.Generator[Profile, None, None]:
    """Return a test profile configured for the :class:`aiida_s3.storage.psql_s3.PsqlS3Storage`."""
    with aiida_profile_factory(
        aiida_config,
        storage_backend='s3.psql_s3',
        storage_config=config_psql_s3()
    ) as profile:
        yield profile

@danielhollas @unkcpz @mbercx @khsrali @GeigerJ2

@danielhollas danielhollas self-requested a review April 9, 2024 09:07
@danielhollas
Copy link
Collaborator

This is super awesome, really looking forward to this being integrated. 👏

I am going to try it on aiidalab-widgets-base tests. @sphuber I see that the fixtures moved to a new location but are still available in the old one (deprecated). It there any migration needed or is this essentially drop in and all I need to do is to install aiida-core from this branch?

@sphuber
Copy link
Contributor Author

sphuber commented Apr 9, 2024

It there any migration needed or is this essentially drop in and all I need to do is to install aiida-core from this branch?

There are some minor differences in a few fixtures but mostly drop in replacements. The biggest change is that aiida_local_code_factory is removed and replaced by aiida_code and aiida_code_installed. Where aiida_local_code_factory would always created an InstalledCode, the aiida_code requires to specify which subclass to create. The aiida_code_installed does this automatically but the args typically passed to aiida_local_code_factory, the entry_point and executable, should now be passed as default_calc_job_plugin and filepath_executable. This lines up with the actual interface of the InstalledCode model

There are a few reasons I decided to (for now at least) place the fixtures in a new module:

  • Easy to deprecate the old ones and not break it
  • The new location makes more sense, having them in aiida.manage.tests always seemed wrong to me
  • Took the chance to separate them in individual modules to make it easier to develop/maintain instead of one single massive module

@danielhollas
Copy link
Collaborator

I am testing the new fixtures in aiidalab-widgets-base in this PR:

aiidalab/aiidalab-widgets-base#582

I've run into one issue: When I don't provide a broker, the submit function does not work. I for some reason assumed that when there is no broker the submit function will kind of behave like run, but that is clearly not the case. Assuming that this is intended behaviour, I guess one would need to monkeypatch submit in tests?

@sphuber
Copy link
Contributor Author

sphuber commented Apr 11, 2024

I've run into one issue: When I don't provide a broker, the submit function does not work. I for some reason assumed that when there is no broker the submit function will kind of behave like run, but that is clearly not the case. Assuming that this is intended behaviour, I guess one would need to monkeypatch submit in tests?

This is indeed intended. When there is no broker, all functionality requiring communication is not supported. This essentially means running and submitting to the daemon. The code will warn the user of this in various places such as verdi status.

If you need the daemon for your tests, you can just configure a profile with RabbitMQ as a broker. You can do as I did for aiida-shell and override the aiida_profile auto-use fixture to define one:

@pytest.fixture(scope='session', autouse=True)
def aiida_profile(aiida_config, aiida_profile_factory):
    """Create and load a profile with RabbitMQ as broker."""
    with aiida_profile_factory(aiida_config, broker_backend='core.rabbitmq') as profile:
        yield profile

That should be all that is necessary

Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nits and suggestions in docs for now, will continue review later.

docs/source/howto/plugins_develop.rst Outdated Show resolved Hide resolved
docs/source/howto/plugins_develop.rst Outdated Show resolved Hide resolved
docs/source/topics/plugins.rst Show resolved Hide resolved
docs/source/topics/plugins.rst Outdated Show resolved Hide resolved
docs/source/topics/plugins.rst Outdated Show resolved Hide resolved
docs/source/topics/plugins.rst Outdated Show resolved Hide resolved
@sphuber sphuber force-pushed the feature/improved-profile-fixtures branch from 030a472 to 890f95d Compare April 11, 2024 20:05
@sphuber
Copy link
Contributor Author

sphuber commented Apr 12, 2024

@danielhollas did you finish the review and can this be merged? Or did you want to give another pass?

@danielhollas
Copy link
Collaborator

I didn't got through the meaty part yet, will do later today. But it would be good to get another pair of eyes from somebody more experienced actually using these fixtures, especially around their API.

@sphuber
Copy link
Contributor Author

sphuber commented Apr 12, 2024

I didn't got through the meaty part yet, will do later today. But it would be good to get another pair of eyes from somebody more experienced actually using these fixtures, especially around their API.

I am not sure that there actually is anyone that has been using these extensively ^^ So not sure who to ask

Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, last set of comments / questions, mostly around fixture docstrings.

Cheers, great work on this, can't wait to start using this. The only sad thing is that plugins that want to test both new and old aiida-core version will not be able to easily take advantage of this.

I am not sure that there actually is anyone that has been using these extensively ^^ So not sure who to ask

Haha, fair enough :D

src/aiida/tools/pytest_fixtures/orm.py Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/orm.py Outdated Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/orm.py Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/orm.py Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/orm.py Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/configuration.py Outdated Show resolved Hide resolved

@pytest.fixture(scope='class')
def aiida_profile_clean_class(aiida_profile):
"""Return a loaded temporary AiiDA profile where the data storage is cleaned before yielding.
Copy link
Collaborator

@danielhollas danielhollas Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording (or something like it) seems a bit clearer for users who might not be that familiar with pytest fixtures. Same for aiida_profile_clean

Suggested change
"""Return a loaded temporary AiiDA profile where the data storage is cleaned before yielding.
"""Return a loaded temporary AiiDA profile where the data storage is cleaned before the start of the test.

@sphuber sphuber force-pushed the feature/improved-profile-fixtures branch from fba7153 to 97ee138 Compare April 15, 2024 20:54
The `pytest` fixtures are intended to help plugin packages to easily
write unit tests. Arguably the most important fixture is the
`aiida_profile` fixture which automatically provides a ready-to-go
profile. The downside is that it uses the `core.psql_dos` storage
backend, which was historically the only available storage.

Now there are other storage plugins available. Not only would it be
useful to allow a user to easily configure which storage plugin to use
for the test profile, it would make sense to change the default from
`core.psql_dos` to a storage plugin that doesn't require a PostgreSQL
database. Sure, the plugin currently uses `pgtest` to create a test
cluster on the fly so the test database is created on the fly and
doesn't affect production databases, however, it still does require the
PostgreSQL libraries to be installed, or the `pg_ctl` binary won't be
found and the fixture fails.

The `aiida_profile` fixture now instead uses the `core.sqlite_dos` by
default and configures to use no broker, which also means that RabbitMQ
is no longer needed. This makes it possible to run the tests by default
without any services running, making it much easier to get started
running tests on any environment that just has `aiida-core` installed.
@sphuber sphuber force-pushed the feature/improved-profile-fixtures branch from 97ee138 to 18e6686 Compare April 16, 2024 09:16
@sphuber sphuber merged commit e3a6046 into aiidateam:main Apr 16, 2024
20 checks passed
@sphuber
Copy link
Contributor Author

sphuber commented Apr 16, 2024

Thanks as always for the review @danielhollas !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants