Fix Ruby 4 CGI flaky test - #23364
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to stabilise the test suite under Ruby 4 by filtering out the specific CGI removal compatibility warning that can be emitted when plist ends up loading CGI, while keeping other unexpected warnings visible so stderr-based expectations still catch regressions.
Changes:
- Add a
:cgiwarning category pattern toWarnings::COMMON_WARNINGS. - Add a unit test asserting the Ruby 4 CGI warning can be ignored.
- Wrap each RSpec example run in
Warnings.ignore(:cgi)inspec_helper.rbto prevent flaky failures caused by that warning.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/warnings.rb | Adds a new common warning category (:cgi) to be ignored via Warnings.ignore. |
| Library/Homebrew/test/warnings_spec.rb | Adds a spec ensuring the Ruby 4 CGI warning is suppressed when ignored. |
| Library/Homebrew/test/spec_helper.rb | Ignores the :cgi warning around each example execution to avoid Ruby 4 flakiness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
82e369f to
300bd80
Compare
dduugg
left a comment
There was a problem hiding this comment.
This is a genuine narrowly-scoped fix rather than a papered-over incompatibility, which is what I wanted to check given the branch name. Reviewing the current head, which no longer touches warnings.rb, so the earlier comment on that file looks superseded.
I traced the cause: vendor/portable-ruby/4.0.6/lib/ruby/4.0.0/cgi.rb is now a shim doing warn <<-WARNING, uplevel: Gem::BUNDLED_GEMS.uplevel if $VERBOSE, routed through Warning.warn, so the prepended Warnings::Filter is the only thing that can suppress it. The regex is anchored to that exact message rather than being a broad filter, so it will not swallow unrelated warnings, and warnings_spec.rb keeps its existing coverage. Production is unaffected either way, since brew.sh runs with HOMEBREW_RUBY_WARNINGS=-W1 and the shim only fires when $VERBOSE is set.
Two optional notes, neither blocking.
The suppression is thread-local
Library/Homebrew/test/spec_helper.rb:353
Warnings uses Thread.current.thread_variable_set, and thread variables are not inherited by child threads (confirmed against the vendored 4.0.6). So a first require "plist" reached from a thread spawned by Utils.parallel_map, via the SystemConfig.dump_verbose_config to host_software_config to MacOS::Xcode.detect_version route, would not be covered.
Nothing fails today, because every spec that reaches dump_verbose_config stubs the section that would pull in plist (test/cmd/config_spec.rb:86, :104 and :115). So this only bites if someone later adds a spec exercising an unstubbed dump_verbose_config on macOS.
The load-time idiom already used at spec_helper.rb:21 would close it for free:
Warnings.ignore(/CGI library is removed from Ruby 4\.0\./) { require "cgi" }The require marks cgi loaded, so plist's later require "cgi" is a no-op and the warning cannot fire again.
The new example is a tautology with respect to the regex
Library/Homebrew/test/warnings_spec.rb:8
The example emits its own copy of the message, so by construction it only asserts that the literal and the regex agree. If the upstream shim reworded the text, spec_helper.rb:353 would stop matching the real cgi.rb heredoc while this example stayed green.
It is not worthless, since it fails loudly if someone deletes or reorders the Warnings.ignore wrapper, which is the realistic regression for an 8-line harness change. And the drift risk is bounded, because the message lives in pinned portable-ruby and can only change on a deliberate bump.
If you want it to check wording against pattern, shelling out to HOMEBREW_RUBY_PATH -w -e 'require "cgi"' and asserting its stderr matches the same regex would do it. Otherwise it might belong outside RSpec.describe Warnings, since it asserts spec_helper behaviour rather than the Warnings API and is the only example in the file not touching described_class.
- Ignore the compatibility warning emitted when `plist` lazily loads CGI. - Keep unexpected warnings visible to stderr expectations.
300bd80 to
7cd7933
Compare
plistlazily loads CGI.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?OpenAI GPT 5.6 sol xhigh with local review and testing.