Fix legacy pytest fixtures #6904

danielhollas · 2025-06-04T23:09:23Z

Closes #6881.

This fixes a tricky bug in the legacy pytest fixtures introduced in #6610.
Due to convoluted reasons explained below, this bug apparently only manifested for the daemon_client fixtures, which were failing to start the deamon with the error:

aiida.engine.daemon.client.DaemonException: The daemon failed to start with error:
/home/runner/work/aiida-core/aiida-core/src/aiida/manage/configuration/settings.py:109: UserWarning: Creating AiiDA configuration folder `/tmp/pytest-of-runner/pytest-0/config0/.aiida`.
  warnings.warn(f'Creating AiiDA configuration folder `{path}`.')
Usage: verdi [OPTIONS] COMMAND [ARGS]...
Try 'verdi --help' for help.

Error: Invalid value for '-p' / '--profile': profile `db643428-7e94-487b-8022-17afc87e5204` does not exist

In the first commit of this PR, I've added a new CI job that specifically tests these fixtures, and you can see that they are failing: https://github.com/aiidateam/aiida-core/actions/runs/15454591396/job/43504227763

So the problem was that since #6610, the temporary config folder was set differently than before: Instead of for example /tmp/pytest/config/.aiida it would be /tmp/pytest/config/.
This happened to work fine for most cases, but not for the daemon fixtures, because:

These fixtures use the DaemonClient.start_daemon method to start the daemon with the given test profile and configuration.

aiida-core/src/aiida/engine/daemon/client.py

Line 516 in e768b70

def start_daemon(
The daemon process is started using a subprocess call to verdi daemon start-circus

aiida-core/src/aiida/engine/daemon/client.py

Line 140 in e768b70

command = [self._verdi_bin, '-p', self.profile.name, 'daemon', 'start-circus', str(number_workers)]
To ensure that the verdi command picks up the correct configuration and profile, the subprocess call sets the AIIDA_PATH variable to the current config folder.

aiida-core/src/aiida/engine/daemon/client.py

Line 225 in e768b70

env['AIIDA_PATH'] = get_config().dirpath
The launched verdi command parses the AIIDA_PATH in aiida.manage.configurations.settings._get_configuration_directory_from_envvar . This function however has a rather confusing logic where if AIIDA_PATH is set to a path that does not end with .aiida/ folder, the .aiida folder gets appended automatically.
💥

The smoking gun was there all along --- this innocent warning

UserWarning: Creating AiiDA configuration folder /tmp/pytest-of-runner/pytest-0/config0/.aiida

was telling us what had happened: The verdi command thought the config folder was /tmp/pytest-of-runner/pytest-0/config0/.aiida, whereas we've set it to /tmp/pytest-of-runner/pytest-0/config0/. It then went on to create and empty configuration, and then promptly failed as it did not contain the given profile.

The fix and tests here are intentionally minimal to speed up review for 2.7.0 release.
There are couple of follow-ups:

I added more tests for both legacy and new pytest fixtures in Add more pytest fixture tests #6882
Added type checking to legacy fixtures in Fix typing errors in src/aiida/manage/tests module #6903.
I think the logic in _get_configuration_directory_from_envvar should be extended so that this problem would not appear in the first place. I'll open a separate issue for that.

codecov · 2025-06-04T23:15:27Z

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.08%. Comparing base (ec9d53e) to head (229c35a).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/manage/tests/test_pytest_fixtures.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6904      +/-   ##
==========================================
+ Coverage   78.61%   79.08%   +0.48%     
==========================================
  Files         564      565       +1     
  Lines       43116    43126      +10     
==========================================
+ Hits        33890    34103     +213     
+ Misses       9226     9023     -203

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

danielhollas · 2025-06-05T01:13:11Z

src/aiida/manage/tests/pytest_fixtures.py

@@ -177,7 +177,7 @@ def aiida_instance(
            current_profile = configuration.get_profile()
            current_path_variable = os.environ.get(settings.DEFAULT_AIIDA_PATH_VARIABLE, None)

-        dirpath_config = tmp_path_factory.mktemp('config')
+        dirpath_config = tmp_path_factory.mktemp('config') / settings.DEFAULT_CONFIG_DIR_NAME


settings.DEFAULT_CONFIG_DIR_NAME is .aiida

.github/workflows/ci-code.yml

agoscinski

I don't fully understand, don' we have tests/manage/tests/test_pytest_fixtures.py for this purpose? Do we need another test file in src (plus workflow D:)?

.github/workflows/ci-code.yml

danielhollas · 2025-06-05T08:22:28Z

Good point. That test file doesn't work as intended, because its part of the tests suite, it actually utilizes the new pytest fixtures (which have the same names for the most part), not the old ones. That's why to prevent this from happening I think it's better to to put the test file in src, although not ideal.

Please have a look at #6882 where I removed the file and moved the tests, it might make more sense. Happy to do it here as well.

agoscinski

Thanks for clarification. I see the need to run the pytest with the --noconftest option to test it in isolation. But then wouldn't it be better to add this as a additional step in the ci-code workflow? Especially, to collect for these files also code-coverage? Or is there an additional dependency when running the test from the tests folder even with the --noconftest option specified?

danielhollas · 2025-06-06T21:28:42Z

But then wouldn't it be better to add this as a additional step in the ci-code workflow?

As explained in the comment, I specifically want to avoid running the .github/workflows/setup.sh, which sets up a test profile and whole lot of other things.

I tried running the fixture tests before the setup.sh is called, but ran into weird DB issues when the setup.sh runs after that, and it also made the already complicated workflow more complicated so I abandonded that idea.

Especially, to collect for these files also code-coverage?

Not sure what you mean, I am collecting the code coverage in the new job.

Or is there an additional dependency when running the test from the tests folder even with the --noconftest option specified?

Sorry, I guess I am conflating concerns about having a new CI job, and having a test file outside of tests/ folder. The problem of having it in test folder is that while you can run it separately via --noconftest option, you would also need to ensure that these tests are skipped when running a normal test suite, otherwise you could likely get clashes between the old and new fixtures. All in all I think it is much easier to keep the old tests comppletely separate (and I also think its fine given that they are deprecated)

danielhollas · 2025-06-06T21:32:08Z

To clarify, I am happy if somebody wants to make this better as a follow-up, but I have already spent too much time on this (the original PR #6882 has 39 commits) so I'd suggest to merge this to unblock this for the 2.7.0 release and make an TODO issue.

Fix config path in legacy aiida_instance pytest fixture Update .github/workflows/ci-code.yml Co-authored-by: Alexander Goscinski <[email protected]>

agoscinski · 2025-06-11T06:36:53Z

I want to merge this as a merge commit with the two separate commits. @danielhollas could you check the 2 commit messages if I got it correct? I did not change the content of the PR

agoscinski

just for the notifier, everything okay, see message above.

This bug was introduced in PR aiidateam#6610, where the temporary config folder path was changed unintentionally. Previously, it was set to `/tmp/pytest-.../config/.aiida`, but the new setup used `/tmp/pytest-.../config/`. As a result, the daemon_client fixture failed to detect the profile, leading to subtle issues. This bug was masked by two factors: - The CI setup (`setup.sh`) which preconfigures AiiDA profiles. - The fixture definitions in `tests/conftest.py`, which interfered with the intended isolation of aiida_instance. A follow-up commit will address this entanglement by clearly separating fixture configuration and test logic.

Previously, legacy pytest fixtures were only tested via `tests/manage/tests/test_pytest_fixtures.py`. These tests were inadvertently affected by: - Fixtures in `tests/conftest.py`, which caused test contamination. - The CI environment, where AiiDA profiles were already created via setup.sh. To ensure true isolation: - The `--no-conftest` flag is now used to prevent loading `conftest.py`. - A new CI job in the ci-code workflow has been added to run these tests independently. - The test file has been relocated to the src directory to avoid interference from the main test suite.

danielhollas · 2025-06-11T12:46:51Z

Commit messages looks great, thank you!

The pytest fixture tests need to be isolated from the regular test suite to test if they can work independently from aiida-core test suite. Having them in the test suite risks them to hiddenly reuse configurations as it introduced the bug aiidateam#6904. Moving them to src means however that we ship the tests with releases which we also want to avoid. We therefore move them to the .github folder.

The pytest fixture tests need to be isolated from the regular test suite to verify that they can function independently of the aiida-core test suite. Including them in the main test suite risks inadvertently reusing configurations, which previously led to bug PR #6904 fixed. However, moving these tests to the `src` would result in them being included in distribution packages, which is undesirable. Therefore, we relocate them to the `.github` directory, ensuring they remain part of the repository but are excluded from the shipped package.

danielhollas commented Jun 5, 2025

View reviewed changes

.github/workflows/ci-code.yml Show resolved Hide resolved

danielhollas requested review from agoscinski and unkcpz June 5, 2025 01:40

danielhollas marked this pull request as ready for review June 5, 2025 01:40

agoscinski reviewed Jun 5, 2025

View reviewed changes

.github/workflows/ci-code.yml Show resolved Hide resolved

.github/workflows/ci-code.yml Outdated Show resolved Hide resolved

agoscinski added this to aiida-core v2.7.0 Jun 5, 2025

danielhollas requested a review from agoscinski June 5, 2025 15:20

danielhollas moved this to In review in aiida-core v2.7.0 Jun 5, 2025

danielhollas self-assigned this Jun 5, 2025

agoscinski reviewed Jun 6, 2025

View reviewed changes

danielhollas requested a review from agoscinski June 9, 2025 16:16

agoscinski force-pushed the fix-legacy-fixtures-minimal branch from 58cd7e5 to 4d17c35 Compare June 11, 2025 06:35

agoscinski requested changes Jun 11, 2025

View reviewed changes

danielhollas added 2 commits June 11, 2025 14:31

agoscinski force-pushed the fix-legacy-fixtures-minimal branch from 4d17c35 to 229c35a Compare June 11, 2025 12:31

agoscinski approved these changes Jun 11, 2025

View reviewed changes

agoscinski merged commit 48991ab into aiidateam:main Jun 11, 2025
12 checks passed

github-project-automation bot moved this from In review to Done in aiida-core v2.7.0 Jun 11, 2025

danielhollas mentioned this pull request Jun 12, 2025

Add more pytest fixture tests #6882

Open

agoscinski mentioned this pull request Jun 24, 2025

Move pytest fixture test to .github folder #6921

Merged

danielhollas deleted the fix-legacy-fixtures-minimal branch July 24, 2025 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix legacy pytest fixtures #6904

Fix legacy pytest fixtures #6904

Uh oh!

danielhollas commented Jun 4, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 4, 2025 •

edited

Loading

Uh oh!

danielhollas Jun 5, 2025

Uh oh!

Uh oh!

agoscinski left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

danielhollas commented Jun 5, 2025

Uh oh!

agoscinski left a comment •

edited

Loading

Uh oh!

danielhollas commented Jun 6, 2025 •

edited

Loading

Uh oh!

danielhollas commented Jun 6, 2025

Uh oh!

agoscinski commented Jun 11, 2025

Uh oh!

agoscinski left a comment •

edited

Loading

Uh oh!

danielhollas commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

Fix legacy pytest fixtures #6904

Fix legacy pytest fixtures #6904

Uh oh!

Conversation

danielhollas commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danielhollas Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agoscinski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielhollas commented Jun 5, 2025

Uh oh!

agoscinski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhollas commented Jun 6, 2025

Uh oh!

agoscinski commented Jun 11, 2025

Uh oh!

agoscinski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

danielhollas commented Jun 4, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading

agoscinski left a comment •

edited

Loading

agoscinski left a comment •

edited

Loading

danielhollas commented Jun 6, 2025 •

edited

Loading

agoscinski left a comment •

edited

Loading