Skip to content

Improved wrappers and OpenMPI handling in Python MPI tests#3467

Merged
heplesser merged 4 commits intonest:masterfrom
heplesser:better_conftest
Apr 15, 2025
Merged

Improved wrappers and OpenMPI handling in Python MPI tests#3467
heplesser merged 4 commits intonest:masterfrom
heplesser:better_conftest

Conversation

@heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 12, 2025

This PR fixes the problem we had with subprocess and mpirun. Thus, mpitests can now be run with conftest.py and the usual "missing" decorators can be applied to MPI tests.

See also #3466.

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Apr 12, 2025
@github-project-automation github-project-automation bot moved this to In progress in PyNEST-NG Apr 12, 2025
@heplesser heplesser marked this pull request as ready for review April 12, 2025 08:30
@heplesser heplesser requested a review from JanVogelsang April 13, 2025 20:36
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

One suggestion you may consider (maybe in a future PR): As I understand the documentation, you can use the pytest_ignore_collect hook in conftest.py to skip tests in a specified directory based on a condition. This should work well for the MPI tests since they are in a dedicated directory, and would save you from having to decorate every MPI test function with skipif_incompatible_mpi.

@heplesser
Copy link
Contributor Author

Nice, LGTM!

One suggestion you may consider (maybe in a future PR): As I understand the documentation, you can use the pytest_ignore_collect hook in conftest.py to skip tests in a specified directory based on a condition. This should work well for the MPI tests since they are in a dedicated directory, and would save you from having to decorate every MPI test function with skipif_incompatible_mpi.

Thanks for the hint, I had been looking for some way to skip entire directories. But I think it is not exactly what I would like, because if I understand this right, it would completely ignore the directory, so the test placed in it would be invisible in the test report. I would like them to be reported as skipped, so one is aware of the fact that tests were not run.

@nicolossus
Copy link
Member

Nice, LGTM!
One suggestion you may consider (maybe in a future PR): As I understand the documentation, you can use the pytest_ignore_collect hook in conftest.py to skip tests in a specified directory based on a condition. This should work well for the MPI tests since they are in a dedicated directory, and would save you from having to decorate every MPI test function with skipif_incompatible_mpi.

Thanks for the hint, I had been looking for some way to skip entire directories. But I think it is not exactly what I would like, because if I understand this right, it would completely ignore the directory, so the test placed in it would be invisible in the test report. I would like them to be reported as skipped, so one is aware of the fact that tests were not run.

Good point. This would skip collecting the tests, so they will probably not show up in the summary. Another alternative could be to skip the tests at module level with pytest.skip(reason, allow_module_level=True), see https://docs.pytest.org/en/stable/how-to/skipping.html#skipping-test-functions

@heplesser heplesser merged commit 4d5e65a into nest:master Apr 15, 2025
24 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in PyNEST-NG Apr 15, 2025
@jessica-mitchell jessica-mitchell added T: Maintenance Work to keep up the quality of the code and documentation. and removed T: Enhancement New functionality, model or documentation labels Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

Comments