spec_helper: improve parallel test handling. #18639
Conversation
MikeMcQuaid
commented
Oct 26, 2024
MikeMcQuaid
commented
- Clarify the comment of why we have SimpleCov special logic for parallel tests
- use a nicer ParallelTests API for checking which process to output the coverage format on
- Clarify the comment of why we have SimpleCov special logic for parallel tests - use a nicer ParallelTests API for checking which process to output the coverage format on
MikeMcQuaid
force-pushed
the
improve_parallel_tests
branch
from
6ec8d79
to
92fee90
Compare
| if RUBY_PLATFORM[/darwin/] && ENV["TEST_ENV_NUMBER"] | ||
| SimpleCov.at_exit do | ||
| result = SimpleCov.result | ||
| result.format! if ParallelTests.number_of_running_processes <= 1 | ||
| result.format! if ParallelTests.last_process? |
There was a problem hiding this comment.
I'm not very familiar with the parallel_tests gem but according to the docs it looks like this .last_process? method indicates that it's the last started process not the last finished process. I assume we want to test for the last finished process so that we format the coverage result for all test processes. If that's the case, we should keep the old logic.
There was a problem hiding this comment.
Yeah this would need ParallelTests.wait_for_other_processes_to_finish for this to work which internally just does number_of_running_processes <= 1 anyway but with the downside of sleep calls.
Seems the original is more correct as is.
There was a problem hiding this comment.
The bit that's weird here is the RUBY_PLATFORM[/darwin/]. Why not do this for all OS? I suspect that part of the check came from a time when coverage was macOS only.
There was a problem hiding this comment.
I'm not very familiar with the
parallel_testsgem but according to the docs
@apainintheneck I am very familiar with it. I wrote this code (3c270b3). I also reread all the relevant SimpleCov code in making this change.
.last_process?method indicates that it's the last started process not the last finished process
Yes but: this doesn't matter. We don't need to run this on the last running process, just any single process.
Yeah this would need
ParallelTests.wait_for_other_processes_to_finishfor this to work which internally just doesnumber_of_running_processes <= 1anyway but with the downside of sleep calls.
@Bo98 did you test this locally? I did and it seemed to work as expected. Similarly, I don't understand why the coverage metrics on this PR would work if this were the case.
I'm going to just leave this as-is, I'm not sure it's worth anyone's time to further discuss this cleanup. I don't think the review process here has been optimal.
The bit that's weird here is the
RUBY_PLATFORM[/darwin/]. Why not do this for all OS? I suspect that part of the check came from a time when coverage was macOS only.
@Bo98 Feel free to open a PR for this if desired.
There was a problem hiding this comment.
I did and it seemed to work as expected. Similarly, I don't understand why the coverage metrics on this PR would work if this were the case.
Looks like that's because SimpleCov already has ParallelTests support and has the ParallelTests.wait_for_other_processes_to_finish call I suggested adding: https://github.com/simplecov-ruby/simplecov/blob/b6c2d4208a5fa395ce09d7e1d3b074f680ee29b0/lib/simplecov.rb#L279
I'll clarify the comment to specify the problem more directly to avoid future confusion here - I thought it meant that SimpleCov didn't support parallel tests at all. Probably worth upstreaming this diff at some point.
There was a problem hiding this comment.
@Bo98 Thanks for getting this over the line!
Bo98
force-pushed
the
improve_parallel_tests
branch
2 times, most recently
from
c7a47b1
to
a08b0f4
Compare
Bo98
force-pushed
the
improve_parallel_tests
branch
from
a08b0f4
to
46cb9ec
Compare
