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

CI(pytest): Run tests marked need_solo_run separately #3879

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jun 17, 2024

One probable explanation of some flaky pytest failures is that a small number of tests use more resources since they expect they would be the only module running. Or, for example, testing with multiple number of processes, while already multiple pytest runners are executing tests.

This ended up stalling the other tests, and we regularly encountered failures to different tests files.

The solution here is identifying which tests have this behaviour, and should be run separately from the tests run in parallel, with a single runner. Inspired by pytest’s docs on markers, the needs_solo_run marker name was chosen and applied to two tests (10 items).
The slow marker from pytest’s docs is kept as I intend to mark some tests that are way slower than expected with it.

With these two steps running with parallel (pytest-xdist) and without, we end up with a somewhat constant time execution, no timeouts, but having to do the test collection phase twice (currently about 10 more seconds). It also allows to reduce the timeout to 30 seconds without any problem, as in any case, NO TESTS should take near 30 seconds to setup and execute, especially if they are unit tests.

@github-actions github-actions bot added CI Continuous integration Python Related code is in Python libraries labels Jun 17, 2024
@echoix
Copy link
Member Author

echoix commented Jun 18, 2024

I've been reanalyzing the problem here.

@echoix
Copy link
Member Author

echoix commented Jun 19, 2024

Edit: Since the real fix to deadlocks/forking of multiprocessing+subprocess is somewhat substantial, I'm loosening back the timeout requirements. This still improves the situation, but some parts of the test fixtures need to be reworked to be able to use the "spawn" multiprocessing startup mode, that will be default in 3.14. Spawn also requires the code to work correctly with pickling, which currently fails with our code base. Spawn startup method for multiprocessing most probably seems like the real fix for some of the issues. Fork (still the default on linux) isn't available for Windows, and problematic on macOS (and not default since an emergency change for 3.8).

There is one usage in a fixture, "run_in_subprocess", (that isn't subprocesss, but multiprocessing.Process), that makes multiple tests fail when using the spawn startup.

So some more changes are needed to make these pytest work for Windows and macOS, which will be required at some point, and for linux, for python 3.14 support.

The original intent of the run_in_subprocess fixture can be replaced once #3694 is merged, which requires #3002 for python 3.8+ support offered by Jammy.

@echoix
Copy link
Member Author

echoix commented Jun 19, 2024

But this is still an improvement, as two problematic functions that use more processes aren't stalling the tests running in parallel. It should be merged when possible.

@echoix echoix requested a review from wenzeslaus June 19, 2024 12:20
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

This is great. Thank you for the effort figuring this out!

@echoix echoix enabled auto-merge (squash) June 19, 2024 13:10
@echoix echoix merged commit afd7610 into OSGeo:main Jun 19, 2024
27 checks passed
@echoix echoix added this to the 8.5.0 milestone Jun 19, 2024
cyliang368 pushed a commit to cyliang368/grass that referenced this pull request Jun 22, 2024
* pytest: Create slow and needs_solo_run markers

* python: Apply needs_solo_run marker to two pygrass grid tests

* CI(pytest): Run tests marked need_solo_run separately with a single runner

* pytest: Revert timeout to 300 seconds
@a0x8o a0x8o mentioned this pull request Jun 27, 2024
kritibirda26 pushed a commit to kritibirda26/grass that referenced this pull request Jun 29, 2024
* pytest: Create slow and needs_solo_run markers

* python: Apply needs_solo_run marker to two pygrass grid tests

* CI(pytest): Run tests marked need_solo_run separately with a single runner

* pytest: Revert timeout to 300 seconds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants