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

Make postgres_cluster and config_psql_dos fixtures configurable #6254

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jan 17, 2024

The postgres_cluster and config_psql_dos fixtures are made available publicly in order to help plugin packages with unit testing. Particularly, these fixtures should make it easy to generate test profiles using the core.psql_dos storage plugin, generating the postgres database in a test cluster that is created on-the-fly.

The problem was that the interface did not actually allow to configure anything, such as the database name, but this was hardcoded. This would work fine in case it was used just once, but the fixtures could not be used to create a second profile, as the database name would already exist.

The postgres_cluster fixture is turned into a factory, allowing the database name, username and password to be customized. The unbound method create_database is added to the PGTest instance that it returns to easily create a new database.

The config_psql_dos fixture now actually forwards the relevant part of the custom_configuration to the postgres_cluster.create_database call.

Finally, the aiida_profile_factory fixture factory now makes the custom_configuration argument actually optional, as originally was intended.

@sphuber
Copy link
Contributor Author

sphuber commented Jan 31, 2024

@unkcpz would you care to have a look please?

@unkcpz unkcpz self-requested a review January 31, 2024 09:33
Copy link
Member

@unkcpz unkcpz 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. The changes looks all good.

@unkcpz
Copy link
Member

unkcpz commented Jan 31, 2024

When I review the PR, it reminds me a warning exception in AiiDAlab test, I can confirm it comes from the pytest_fixture and from aiida_profile. Maybe not related to the PR but I assume you have expertise to spot why it happened:

It is a dummy test in aiidalab-widgets-base only load the aiida.manage.tests.pytest_fixtures. It appears after we turn on pytest option of show warning as error.

After I comment out the aiida_profile fixture, this warning disappeared.

tests/test_a.py::test_a Exception ignored in: <_io.FileIO name=6 mode='rb' closefd=True>
Traceback (most recent call last):
  File "/home/jyu/micromamba/envs/aiidalab-dev/lib/python3.9/site-packages/_pytest/unraisableexception.py", line 59, in __exit__
    del self.unraisable
ResourceWarning: unclosed file <_io.BufferedReader name=6>
Exception ignored in: <_io.FileIO name=8 mode='rb' closefd=True>
Traceback (most recent call last):
  File "/home/jyu/micromamba/envs/aiidalab-dev/lib/python3.9/site-packages/_pytest/unraisableexception.py", line 59, in __exit__
    del self.unraisable
ResourceWarning: unclosed file <_io.BufferedReader name=8>

The `postgres_cluster` and `config_psql_dos` fixtures are made available
publicly in order to help plugin packages with unit testing.
Particularly, these fixtures should make it easy to generate test
profiles using the `core.psql_dos` storage plugin, generating the
postgres database in a test cluster that is created on-the-fly.

The problem was that the interface did not actually allow to configure
anything, such as the database name, but this was hardcoded. This would
work fine in case it was used just once, but the fixtures could not be
used to create a second profile, as the database name would already
exist.

The `postgres_cluster` fixture is turned into a factory, allowing the
database name, username and password to be customized. The unbound
method `create_database` is added to the `PGTest` instance that it
returns to easily create a new database.

The `config_psql_dos` fixture now actually forwards the relevant part of
the `custom_configuration` to the `postgres_cluster.create_database`
call.

Finally, the `aiida_profile_factory` fixture factory now makes the
`custom_configuration` argument actually optional, as originally was
intended.
@sphuber sphuber force-pushed the fix/pytest-fixtures-postgres-cluster branch from 2d97f43 to ac89e08 Compare January 31, 2024 17:17
@sphuber
Copy link
Contributor Author

sphuber commented Jan 31, 2024

When I review the PR, it reminds me a warning exception in AiiDAlab test, I can confirm it comes from the pytest_fixture and from aiida_profile. Maybe not related to the PR but I assume you have expertise to spot why it happened

I have seen these warnings before as well, but haven't had the time to investigate it yet. The aiida_profile fixture does depend on some other fixtures that create temporary files, for example for the config.json it creates, and these may not be properly cleaned up. But I would have to look in depth. Don't have an immediate hunch now what it might be.

@sphuber sphuber merged commit 35d7ca6 into aiidateam:main Jan 31, 2024
19 checks passed
@sphuber sphuber deleted the fix/pytest-fixtures-postgres-cluster branch January 31, 2024 18:29
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.

None yet

2 participants