Use Base.Semaphore to control test execution parallelism#119
Use Base.Semaphore to control test execution parallelism#119
Conversation
Replace the fixed worker-task-per-slot model with a semaphore-based approach: one task per test, with a Base.Semaphore(jobs) limiting concurrency and a Channel-based worker pool for reuse. This decouples the number of tasks from the parallelism level and simplifies the control flow (no inner while loop, tests array is immutable). Co-authored-by: Claude <noreply@anthropic.com> Made-with: Cursor
| for p in workers | ||
| tests_to_start = Threads.Atomic{Int}(length(tests)) | ||
| for test in tests |
There was a problem hiding this comment.
The core change I was interested in was changing this for loop from an iteration over the workers, to an iteration over the tests: the idea for #77 is to have different semaphores for different subsets of tests, and here we iterate over the different subsets (and associated semaphores). The whole execution cycle would then require only this change on this single line, the rest would remain as is.
I still need to fully work out the user-facing changes for designating the serial tests for #77. I have some bad ideas in mind, but I want to decouple them from this refactoring, which I believe is still worth on its own right for making the code easier to follow.
|
Uhm, the test failure is interesting:
The error disappeared after a rerun, I'd say it was a glitch? 😬 Edit: tests on all platforms always passed in the following 3 full reruns. |
This is a refactoring of the code, to then enable #77 in a follow up PR (CC @christiangnrd).
The idea of using a semaphore is mine, initial implementation is from Claude, but I made a few manual refinements afterwards (some of them folded in the first commit). Overall summary of the changes:
I'm mostly happy about the result, it's very close to what I had in mind, and the net diff is relatively small (+48/-22), so this shouldn't be too hard to review.
#118 was useful because it detected that the case of no tests to run wasn't handled correctly, so yay for the extra tests.