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.

Sign up for GitHub

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

spec_helper: improve parallel test handling. #18639

Merged
merged 2 commits into from
Oct 27, 2024
Merged

spec_helper: improve parallel test handling. #18639

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92fee90
spec_helper: improve parallel test handling.
MikeMcQuaid Oct 26, 2024
46cb9ec
spec_helper: clarify comment and scope to all OS
Bo98 Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Library/Homebrew/test/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,14 @@
]
SimpleCov.formatters = SimpleCov::Formatter::MultiFormatter.new(formatters)

if RUBY_PLATFORM[/darwin/] && ENV["TEST_ENV_NUMBER"]
# Needed for outputting coverage reporting only once for parallel_tests.
# Otherwise, "Coverage report generated" will get spammed for each process.
if ENV["TEST_ENV_NUMBER"]
SimpleCov.at_exit do
result = SimpleCov.result
result.format! if ParallelTests.number_of_running_processes <= 1
# `SimpleCov.result` calls `ParallelTests.wait_for_other_processes_to_finish`
# internally for you on the last process.
result.format! if ParallelTests.last_process?
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Bo98 Bo98 Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Bo98 Bo98 Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not very familiar with the parallel_tests gem 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_finish for this to work which internally just does number_of_running_processes <= 1 anyway 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.

Copy link
Member

@Bo98 Bo98 Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 Thanks for getting this over the line!

end
end
end
Expand Down
Loading