Tests: Only reset database connection at end of suite #5641
Conversation
|
You should get this to also trigger https://github.com/aiidateam/aiida-core/blob/main/.github/workflows/tests_nightly.sh, i.e. remove skipping certain tests, as obviously we need to make sure this works for the entire test suite |
|
@chrisjsewell things now seem to work. I just did the minimal changes necessary, but the current interface may be a little clunky. Now rebased #5639 to see if these changes fix the problem there as well. |
|
@chrisjsewell can you have a look at this one? Just hit the too many clients error again: https://github.com/sphuber/aiida-core/actions/runs/3088574677/jobs/4995218599 EDIT: and another one: https://github.com/aiidateam/aiida-core/actions/runs/3089249073/jobs/4996654548 |
There was a problem hiding this comment.
Thanks @sphuber, I'll approve because if the tests work then that's the main thing 👍
(perhaps you could add docstrings to the new Manager methods)
In addition, it can result in too many clients to the database being active if the session is not cleaned up properly.
However, I feel there is still a potential underlying issue here, as to where/why the session is not being cleaned properly
It might be good to at least have a test somewhere (I don't think there is one at present), that checks the session is properly cleaned on profile switching?
|
Thanks for the review. I will add the docstrings still, and remove the trigger of the nightly tests. But I will leave adding the profile switching test for another PR, in order to not block this. |
The `aiida_profile_clean` fixture is a function scoped fixture that provides a fully configured profile with a storage backend that is empty, with the exception of a single default user instance. To empty the database before the start of the test, it calls the method `clear_profile` on the `TestManager` which calls through to the `ProfileManager` which calls `reset_profile` on the `StorageBackend`. However, in addition to resetting the database, it also resets all database connections in the session, which is not really necessary. In addition, it can result in too many clients to the database being active if the session is not cleaned up properly. We remove the `reset_profile` call from `clear_profile` and instead move it to the `destroy_all` call. This ensures that the database connections are not completely reset in between tests anymore, however, all is nicely cleaned up at the end of the test session. Instead of the `reset_profile`, we still call `reset_communicator` and `reset_runner` as these need to be reset for the next test run to function properly.
sphuber
force-pushed
the
fix/5640/too-many-active-clients
branch
from
33b913e
to
51fe7f0
Compare
Fixes #5640
The
aiida_profile_cleanfixture is a function scoped fixture that provides a fully configured profile with a storage backend that is empty, with the exception of a single default user instance.To empty the database before the start of the test, it calls the method
clear_profileon theTestManagerwhich calls through to theProfileManagerwhich callsreset_profileon theStorageBackend. However, in addition to resetting the database, it also resets all database connections in the session, which is not really necessary. In addition, it can result in too many clients to the database being active if the session is not cleaned up properly.We remove the
reset_profilecall fromclear_profileand instead move it to thedestroy_allcall. This ensures that the database connections are not completely reset in between tests anymore, however, all is nicely cleaned up at the end of the test session.