Skip to content

Conversation

@mbercx
Copy link
Collaborator

@mbercx mbercx commented Nov 5, 2025

Add the parser tests that were previously in the EPW support branch of aiida-quantumespresso.

For setting up a localhost computer, we rely on the aiida_localhost test fixture provided in the aiida.tools.pytest_fixtures module, made available via the pytest_plugins variable. Originally the idea was to make the aiida-quantumespresso fixtures similarly importable, but closer inspection revealed improvements can still be made, and the fixtures were currently not general enough to work for other plugin packages.

Instead, we add two fixtures here:

  • files_path: this simply returns the path to the test files directory. For the tests added here, the files correspond to the output files for which we want to test the EpwParser.
  • parse_from_files: a simplified version of the fixtures defined in aiida-quantumespresso. Parses the output files in the corresponding test_name directory.

The tests that we had in the EPW support branch have also been adapted to rely on these fixtures.

@mbercx mbercx requested a review from ymzhang0 November 5, 2025 07:27
@mbercx
Copy link
Collaborator Author

mbercx commented Nov 5, 2025

Notes:

  • the CI isn't added yet, so you can't see if the tests work unless you add them locally. I would add the CI first by updating the package template.
  • The current data regression files have been copied directly from the EPW support branch on aiida-quantumespresso, to verify that the parsing works the same. However, I'll add a second commit where I adapt the data being regression tested to reduce the lines of code added.

@mbercx mbercx force-pushed the tests/add-parser branch 2 times, most recently from 9be0ff9 to 9f82330 Compare November 5, 2025 08:55
Copy link
Collaborator

@ymzhang0 ymzhang0 left a comment

Choose a reason for hiding this comment

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

Very nice and robust tests, I think. I left 2 comments. In general, I would like to approve of this PR.

@mbercx
Copy link
Collaborator Author

mbercx commented Nov 5, 2025

Ah, I just realised you already approved the PR when giving your comments. ^^ I suppose since you marked the second comment as resolved, I can go ahead and merge? Will already rebase.

Add the parser tests that were previously in the EPW support branch of
`aiida-quantumespresso`.

For setting up a `localhost` computer, we rely on the `aiida_localhost` test fixture
provided in the `aiida.tools.pytest_fixtures` module, made available via the
`pytest_plugins` variable. Originally the idea was to make the `aiida-quantumespresso`
fixtures similarly importable, but closer inspection revealed improvements can still be
made, and the fixtures were currently not general enough to work for other plugin
packages.

Instead, we add two fixtures here:

- `files_path`: this simply returns the path to the test files directory. For the tests
  added here, the files correspond to the output files for which we want to test the
  `EpwParser`.
- `parse_from_files`: a simplified version of the fixtures defined in
  `aiida-quantumespresso`. Parses the output files in the corresponding `test_name`
  directory.

The tests that we had in the EPW support branch have also been adapted to rely on these
fixtures.
@mbercx
Copy link
Collaborator Author

mbercx commented Nov 5, 2025

There. Also squashed the commits into one final commit that I will then add to main with "Rebase and merge".

@mbercx mbercx requested a review from ymzhang0 November 5, 2025 21:32
@ymzhang0
Copy link
Collaborator

ymzhang0 commented Nov 5, 2025

Yes. I just had a look at the changes. think the tests are quite good now.

@mbercx mbercx merged commit 8edbe89 into aiidaplugins:main Nov 5, 2025
3 checks passed
@mbercx mbercx deleted the tests/add-parser branch November 5, 2025 21:53
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