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

Fixtures: Modularize fixtures creating AiiDA test instance and profile #5758

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 13, 2022

Fixes #5526
Fixes #4365

The aiida_profile is a fixture that will automatically load an
existing test profile, if specified by the AIIDA_TEST_PROFILE environment
variable, or otherwise create a temporary and isolated test profile from
scratch. While very useful to make it easy to start writing tests
against an AiiDA instance, it hard-coded most if not all of the
configuration.

For example, the storage backend was hardcoded to the core.psql_dos
storage backend. This used to be fine since this was the only storage
backend available, but since aiida-core==2.1 they are pluginnable and
so it becomes necessary to be able to configure them.

The aiida_profile still works the same as in that it automatically
provides a fully loaded AiiDA test instance, and it still loads the
core.psql_dos storage by default, but it does so through the new
aiida_profile_factory fixture. This fixture can be reused in plugin
packages to achieve the same effect, but they can override any setting
of the profile configuration by providing those through the
custom_configuration argument.

These new fixtures make the aiida.manage.tests.main module obsolete.
It was added when the tests were still run with the unittest module
instead of pytest. The module is deprecated as all of its
functionality is completely provided by the new, more modular and more
succinct pytest fixtures.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 13, 2022

I am successfully using these fixtures in aiida-s3 to run with minimal changes against a custom storage backend.

Once we are happy with the interface of the fixtures, I will add documentation to the list of public fixtures

@sphuber sphuber force-pushed the fix/5523/configurable-pytest-fixtures branch from d4acba4 to 83fc907 Compare November 13, 2022 16:19
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Cheers, @sphuber

The main downside I see is that it appears that this change will break the tests in pretty much all AiiDA plugins, which I consider to be quite serious (even if it is easy to fix).

Is there no way to "disable" the fixture for specific tests where it is not needed?

def aiida_profile():
"""Set up AiiDA test profile for the duration of the tests.
@pytest.fixture(scope='session')
def postgres_cluster(
Copy link
Member

@ltalirz ltalirz Nov 13, 2022

Choose a reason for hiding this comment

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

it looks to me like some of the code below duplicates what is already in the TemporaryProfileManager in main.py. Can we avoid that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in a way that I can see that will make things more clear. The problem of the TemporaryProfileManager is that it doesn't properly separate concerns but mixes various concepts: The concept of a profile, but hard-codes the integration with a disk repository and a PostgreSQL cluster for a database. The whole point is to properly decouple this so that parts can be easily replaced where needed. I think it is better to deprecate the entire module as I have done, then try and keep bits around and make things complicated. I would have even removed it entirely instead of deprecating it, because the main use case is for plugin to use aiida_profile, but I am not sure if code out there used these classes directly.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 13, 2022

Is there no way to "disable" the fixture for specific tests where it is not needed?

That's the problem, since it is autouse=True and session-scoped, it will be loaded as soon as the tests run. This means everything is created and loaded instantly and for example in aiida-s3 I don't even get the chance to load another type of profile.

But I will experiment some more with aiida-s3 to see how we can switch between profiles with different backends.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 13, 2022

Ok, I think I may have found a way, but it will require some refactoring of the PsqlDosBackend and the PsqlDostoreMigrator. Need to clean things up though, but will do that later. In short, we should put this on hold until I have cleaned things up.

@sphuber sphuber force-pushed the fix/5523/configurable-pytest-fixtures branch 3 times, most recently from 2de1930 to b5da615 Compare November 17, 2022 17:05
@sphuber sphuber requested a review from ltalirz November 17, 2022 21:26
@sphuber
Copy link
Contributor Author

sphuber commented Nov 17, 2022

@ltalirz with #5760 merged, I have rebased this PR and it is now ready for review. The aiida_profile fixture remains autouse=True.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber !

I assume the relevant code is copied from the aiida.manage.tests, so I did not check all the logic down to the last detail.
If there is some change implementation that you'd like me to look at in particular, let me know.

Before I approve, could you please have a look at conftest.py? It seems to contain a number of fixtures that are very similar to the ones now present in the pytest_fixtures.py (profile_factory, isolated_config, ...).

It seems to me we may be able to get rid of those?

The rationale for placing a fixture into conftest.py vs. the pytest_fixtures.py is that the former is only for aiida-core, while fixtures in the latter become available to all plugins, and should thus be considered public API.

Edit: perhaps we should mention this rationale in the module docstrings of these two modules.

.github/system_tests/pytest/test_memory_leaks.py Outdated Show resolved Hide resolved
.github/system_tests/pytest/test_pytest_fixtures.py Outdated Show resolved Hide resolved
@sphuber
Copy link
Contributor Author

sphuber commented Nov 17, 2022

I assume the relevant code is copied from the aiida.manage.tests, so I did not check all the logic down to the last detail. If there is some change implementation that you'd like me to look at in particular, let me know.

Actually, the idea is the same, but I didn't salvage much of the code, or at the least it has been rewritten for a large part. That being said, I have tested them against the aiida-s3 tests, so reasonably confident that they work out of the box.

Before I approve, could you please have a look at conftest.py? It seems to contain a number of fixtures that are very similar to the ones now present in the pytest_fixtures.py (profile_factory, isolated_config, ...).
It seems to me we may be able to get rid of those?

These are actually quite different. They only provide a simple AiiDA instance (.aiida folder with config.json) and are used internally just to test functionality that operates on the config file: setting options, adding/removing profiles etc. This is not something that plugin packages should have to do. And the config/profiles that are being generated are "inert" as in they don't configure a working storage backend with the required services, so it couldn't be used for running tests anyway. There may be a slight bit of overlap that can be reduced, but that would only be for internal use, which I can take care of later.

The rationale for placing a fixture into conftest.py vs. the pytest_fixtures.py is that the former is only for aiida-core, while fixtures in the latter become available to all plugins, and should thus be considered public API.

Exactly right.

Edit: perhaps we should mention this rationale in the module docstrings of these two modules.

I have updated the module docstrings accordingly

Finally, in the added commit I have improved/corrected the docstrings of the fixtures and added them to the fixture documentation with instructions and examples on how to use them.

@sphuber sphuber requested a review from ltalirz November 17, 2022 23:13
@ltalirz
Copy link
Member

ltalirz commented Nov 17, 2022

Thanks!

In that case I should still have a quick look at the logic and check whether there are any changes with respect to the one in the previous test manager (tomorrow).

Also, I noticed that the system tests are still using the test manager module, and so there are a couple of AiidaDeprecationWarnings in the test logs. Perhaps it makes sense to move these over?

@sphuber
Copy link
Contributor Author

sphuber commented Nov 18, 2022

Also, I noticed that the system tests are still using the test manager module, and so there are a couple of AiidaDeprecationWarnings in the test logs. Perhaps it makes sense to move these over?

These are actually testing the test manager module itself. Since we are just deprecating it and not removing them yet, it seems best to keep the tests around as well.

@sphuber sphuber force-pushed the fix/5523/configurable-pytest-fixtures branch 5 times, most recently from e671273 to c0a81a3 Compare November 18, 2022 12:00
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber , just some final comments

def postgres_cluster(
database_name: str | None = None,
database_username: str | None = None,
database_password: str | None = None
Copy link
Member

Choose a reason for hiding this comment

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

with pgtest this variable is superfluous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shall I just get rid of it entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Postgres class still requires passing an explicit password. Is the idea that when using pgtest that any password will do? So should I just remove the argument from postgres_cluster and pass a random password to Postgres.create_dbuser?

Copy link
Member

Choose a reason for hiding this comment

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

The Postgres class still requires passing an explicit password.

I see.

Is the idea that when using pgtest that any password will do? So should I just remove the argument from postgres_cluster and pass a random password to Postgres.create_dbuser?

I would guess that even an empty password should do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. But does it even make sense to expose the other variables? Would a user ever want to specify an explicit username? If so, then the same could hold for the database, couldn't it? So either we maybe remove both and just keep the database name or we keep all three. But maybe the database name should also not be configurable if the whole point is that it should just create a temporary one. Not sure if you would ever need to have to specify an explicit one

yield configuration.CONFIG
finally:
if reset:
if current_path_variable:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure, but if AIIDA_PATH is empty at first, won't this result in AIIDA_PATH being str(dirpath_config) after the test completes?

Perhaps it does not matter for the test suite because the variable is always overridden, but it would seem cleaner to restore the environment to its previous state after the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think you are right and I agree that restoring to original state, even if that means unsetting, is the correct behavior

"""
config = aiida_instance
configuration = {
'default_user_email': '[email protected]',
Copy link
Member

Choose a reason for hiding this comment

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

should this hardcoded default be made available as a (private) constant at the module level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Not sure it matters though, because it people want to override it, they pass it in the custom_configuration. They wouldn't have to start messing around with a module constant.

@ltalirz
Copy link
Member

ltalirz commented Nov 18, 2022

All of the remaining points are optional - feel free to proceed as you see fit (no need for further review from my side)

@sphuber sphuber force-pushed the fix/5523/configurable-pytest-fixtures branch from c0a81a3 to 22be5c2 Compare November 18, 2022 16:26
The `aiida_profile` is a fixture that will automatically load an
existing test profile, if specified by the `AIIDA_TEST_PROFILE` environment
variable, or otherwise create a temporary and isolated test profile from
scratch. While very useful to make it easy to start writing tests
against an AiiDA instance, it hard-coded most if not all of the
configuration.

For example, the storage backend was hardcoded to the `core.psql_dos`
storage backend. This used to be fine since this was the only storage
backend available, but since `aiida-core==2.1` they are pluginnable and
so it becomes necessary to be able to configure them.

The `aiida_profile` still works the same as in that it automatically
provides a fully loaded AiiDA test instance, and it still loads the
`core.psql_dos` storage by default, but it does so through the new
`aiida_profile_factory` fixture. This fixture can be reused in plugin
packages to achieve the same effect, but they can override any setting
of the profile configuration by providing those through the
`custom_configuration` argument.

These new fixtures make the `aiida.manage.tests.main` module obsolete.
It was added when the tests were still run with the `unittest` module
instead of `pytest`. The module is deprecated as all of its
functionality is completely provided by the new, more modular and more
succinct pytest fixtures.
@sphuber sphuber force-pushed the fix/5523/configurable-pytest-fixtures branch from 22be5c2 to 60054e1 Compare November 18, 2022 16:36
@sphuber sphuber merged commit da5103e into aiidateam:main Nov 18, 2022
@sphuber sphuber deleted the fix/5523/configurable-pytest-fixtures branch November 18, 2022 17:10
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.

Make the pytest fixtures more configurable and reusable Refactor the aiida.manage.tests fixture manager code
2 participants