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

Tests: Make PsqlDosStorage profile unload test more robust #6115

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 5, 2023

The test_unload_profile test verifies that if a loaded profile is unloaded, it properly relinquishes of the session that is maintained by sqlalchemy. It did so by checking that after unloading, there were no sessions being referenced. However, this would fail sometimes, because another session may still be held on to, even though that session had nothing to do with the test.

A more robust test is simply to check that after unloading, there is exactly one less session being held on to, after having called the garbage collector at the beginning of the test to clear any lingering unrelated sessions.

The `test_unload_profile` test verifies that if a loaded profile is
unloaded, it properly relinquishes of the session that is maintained by
sqlalchemy. It did so by checking that after unloading, there were no
sessions being referenced. However, this would fail sometimes, because
another session may still be held on to, even though that session had
nothing to do with the test.

A more robust test is simply to check that after unloading, there is
exactly one less session being held on to.
@sphuber sphuber force-pushed the fix/test-unload-profile branch from f504396 to 684580b Compare September 5, 2023 15:43
@sphuber sphuber requested a review from unkcpz September 5, 2023 16:04
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.

Interesting, looks all good! How do you know adding a gc will fix it?

@sphuber
Copy link
Contributor Author

sphuber commented Sep 5, 2023

Interesting, looks all good! How do you know adding a gc will fix it?

Because that was what was causing the test to fail. The test checked that after unloading, the WeakValueDictionary session container of sqlalchemy was empty. But this was often not the case because there were still other references to a session in there. But these were not related to the test. They probably correspond to sessions created in other tests that had been closed, but the garbage collector had not yet had the time to run. Values in a WeakValueDictionary only dissappear once they no longer have a ref and the garbage collector is run.

@sphuber sphuber merged commit 1c72eac into aiidateam:main Sep 5, 2023
@sphuber sphuber deleted the fix/test-unload-profile branch September 5, 2023 20:09
sphuber added a commit that referenced this pull request Nov 14, 2023
The `test_unload_profile` test verifies that if a loaded profile is
unloaded, it properly relinquishes of the session that is maintained by
sqlalchemy. It did so by checking that after unloading, there were no
sessions being referenced. However, this would fail sometimes, because
another session may still be held on to, even though that session had
nothing to do with the test.

A more robust test is simply to check that after unloading, there is
exactly one less session being held on to.

Cherry-pick: 1c72eac
sphuber added a commit that referenced this pull request Nov 15, 2023
The `test_unload_profile` test verifies that if a loaded profile is
unloaded, it properly relinquishes of the session that is maintained by
sqlalchemy. It did so by checking that after unloading, there were no
sessions being referenced. However, this would fail sometimes, because
another session may still be held on to, even though that session had
nothing to do with the test.

A more robust test is simply to check that after unloading, there is
exactly one less session being held on to.

Cherry-pick: 1c72eac
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