Fix SimpleCov coverage for parallel runs - #23358
Merged
Merged
Conversation
We recently updated SimpleCov from 0.22.0 to 1.0.1 but 1.0 involves some changes to how coverage data is merged/finalized when run in parallel and our preexisting setup doesn't quite work as expected. We addressed API deprecations when upgrading and `brew tests --coverage` works but it produces incomplete coverage data (i.e., the percentages are much lower than expected). This appears to be due to a couple of issues: * In SimpleCov 1.0, `finalize_merge false` causes `SimpleCov.result` to return early, so workers will return their result without waiting for and merging results from other workers. We use it explicitly in `.simplecov` but it would also be set by default for parallel runs when a custom `coverage_dir` is set, as in our case. I think the expectation is that we would use `SimpleCov.collate` but our logic in `spec_helper.rb` doesn't. * We use `result.format! if ParallelTests.last_process?` in the `SimpleCov.at_exit` logic in `spec_helper.rb` but SimpleCov 1.0 switched to using `first_process?` (to align with `parallel_tests`), so that no longer works as expected. This addresses the issue by using `finalize_merge SimpleCov.final_result_process?` and removing the now-unnecessary `SimpleCov.at_exit` logic in `spec_helper.rb`, as the default behavior in 1.0 already does what we want. `finalize_merge true` technically works but it's notably slower for no added benefit (i.e., only one worker needs to merge and format the results). This also removes the `parallel = false if args.coverage? && files.length < Hardware::CPU.cores` workaround in `dev-cmd/tests.rb`, as the related issues have been resolved in 1.0.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates Homebrew’s test coverage setup to work correctly with SimpleCov 1.0.x when RSpec tests are executed in parallel, ensuring coverage results are merged/finalized by the appropriate process and removing no-longer-needed workaround logic.
Changes:
- Adjust SimpleCov configuration to set
finalize_mergedynamically usingSimpleCov.final_result_process?for parallel runs. - Remove custom
SimpleCov.at_exithandling fromtest/spec_helper.rbthat attempted to gate formatting to a single parallel worker. - Remove the
--coverageparallel-disable workaround indev-cmd/tests.rbfor small test sets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Library/Homebrew/.simplecov |
Updates SimpleCov parallel merge finalization logic for correct coverage aggregation in 1.0.x. |
Library/Homebrew/test/spec_helper.rb |
Removes now-unnecessary parallel-only SimpleCov.at_exit formatting guard. |
Library/Homebrew/dev-cmd/tests.rb |
Removes coverage-related parallel execution workaround in the brew tests command. |
Comments suppressed due to low confidence (1)
Library/Homebrew/dev-cmd/tests.rb:106
- This change removes the previous guard that forced
parallel = falsefor coverage runs with fewer spec files than CPU cores. Given this is a behavior change inbrew tests(affecting whether we invokeparallel_rspecvsrspecunder--coverage), it would be good to add/adjust an RSpec example intest/dev-cmd/tests_spec.rbthat exercises#runwith--coverageand a small file set and asserts we still runparallel_rspec(or, if intended otherwise, codifies the new expected behavior).
parallel_rspec_log_name = "parallel_runtime_rspec"
parallel_rspec_log_name = "#{parallel_rspec_log_name}.generic" if args.generic?
parallel_rspec_log_name = "#{parallel_rspec_log_name}.online" if args.online?
parallel_rspec_log_name = "#{parallel_rspec_log_name}.log"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
p-linnane
approved these changes
Jul 29, 2026
MikeMcQuaid
approved these changes
Jul 30, 2026
MikeMcQuaid
left a comment
Member
There was a problem hiding this comment.
Thanks, love a fix that's mostly deleting!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?I used Claude Opus 5 (High) to identify the issue and implement the fixes. I reviewed the changes (having read the upstream SimpleCov documentation and changelog), made some tweaks, and compared
brew tests --coverageoutput to confirm the fix (comparing the output for 0.22.0, themainbranch, and this PR branch).We recently updated SimpleCov from 0.22.0 to 1.0.1 but 1.0 involves some changes to how coverage data is merged/finalized when run in parallel and our preexisting setup doesn't quite work as expected. We addressed API deprecations when upgrading and
brew tests --coverageworks but it produces incomplete coverage data (i.e., the percentages are much lower than expected).This appears to be due to a couple of issues:
finalize_merge falsecausesSimpleCov.resultto return early, so workers will return their result without waiting for and merging results from other workers. We use it explicitly in.simplecovbut it would also be set by default for parallel runs when a customcoverage_diris set, as in our case. I think the expectation is that we would useSimpleCov.collatebut our logic inspec_helper.rbdoesn't.result.format! if ParallelTests.last_process?in theSimpleCov.at_exitlogic inspec_helper.rbbut SimpleCov 1.0 switched to usingfirst_process?(to align withparallel_tests), so that no longer works as expected.This addresses the issue by using
finalize_merge SimpleCov.final_result_process?and removing the now-unnecessarySimpleCov.at_exitlogic inspec_helper.rb, as the default behavior in 1.0 already does what we want.finalize_merge truetechnically works but it's notably slower for no added benefit (i.e., only one worker needs to merge and format the results).This also removes the
parallel = false if args.coverage? && files.length < Hardware::CPU.coresworkaround indev-cmd/tests.rb, as the related issues have been resolved in 1.0.I tested this by running
brew tests --coverageusing the commit before the SimpleCov 1.0 upgrade (09ad50f, which uses 0.22.0) and comparing that to the output onmain(using 1.0.1). After implementing these changes, I ran it again and compared the previous 0.22.0 output to the output from this PR branch and confirmed that the coverage was generally what we would expect. I noticed a few small differences in the coverage but that may be attributable to how SimpleCov behaves in 1.0 (though that's something we can address in a follow-up PR, if needed).