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

Implement tmp_path #74

Merged

Conversation

Matthew-Jennings
Copy link
Contributor

@Matthew-Jennings Matthew-Jennings commented Apr 4, 2024

Hi @alexzwanenburg,

I was just trying to run through the your test suite using pytest-xdist to accelerate the tests by distributing them over multiple processes. I noticed that you were manually creating and trying to handle cleanup of temporary test directories. This doesn't work well with multiprocessing, since some tests were using the same manually-created test dirs and running into PermissionErrors when trying to delete temp files that were being accessed by another process.

PyTest includes a wonderful temporary directory feature when setup and cleanup are neatly handled in the background. A big plus is that they play nicely with pydist-xdist - no PermissionErrors.

I've taken the liberty of implementing PyTest's tmp_path into your test suite, replacing the manual system.

If you do adopt the changes, I'd recommend merging this before merging #70 and adapting #70 to these changes along with the changes I proposed in my PR review.

Note that using pytest-xdist with n=14 brought the total test suite runtime down from 20:12 to 03:35!

To use it yourself, all you need to do is run:

pip install pytest-xdist
pytest -n <numprocesses>....<other pytest options> (e.g. pytest -n 14 -v --color=yes)

Delete unused variables.
Add .coveragerc for compatibility with pytest-xdist.
@alexzwanenburg alexzwanenburg self-requested a review April 4, 2024 11:58
@alexzwanenburg alexzwanenburg changed the base branch from master to dev2.2.1 April 4, 2024 14:19
Copy link
Member

@alexzwanenburg alexzwanenburg left a comment

Choose a reason for hiding this comment

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

Merge with 2.2.1 prior to merging with the master branch.
Add minor changes to use os.path.join for OS-independent paths after merging.

@alexzwanenburg alexzwanenburg merged commit 220146e into oncoray:dev2.2.1 Apr 4, 2024
@Matthew-Jennings Matthew-Jennings deleted the mj-implement-pytest-tmppath branch April 4, 2024 21:10
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