Enable strict typing in Homebrew::TestBot, redux#21539
Merged
MikeMcQuaid merged 3 commits intomainfrom Feb 9, 2026
Merged
Conversation
This reverts commit 6bab83c.
- test_formulae: use [] instead of fetch for @downloaded_artifacts so new SHAs use the hash default (empty array) and avoid KeyError - junit: require rexml/document, xmldecl, cdata in #initialize so REXML is loaded before use (fixes uninitialized constant on Linux CI) - test_cleanup: pass HOMEBREW_REPOSITORY.to_s to checkout_branch_if_needed, reset_if_needed, clean_if_needed (fixes Pathname type error when tap set) - Add test_formulae_spec to prevent KeyError regression for new SHA access - Add junit_spec to prevent REXML NameError regression - Add test_cleanup_spec to prevent Pathname regression
Contributor
There was a problem hiding this comment.
Pull request overview
This PR switches Homebrew::TestBot internals over to typed: strict Sorbet typing, updating method signatures and call sites accordingly, and adds a few regression specs to keep CI failures from recurring.
Changes:
- Convert
Library/Homebrew/test_bot/**classes/modules fromtyped: truetotyped: strict, addingsigs andT.letannotations. - Remove now-unnecessary RBI shims for
TestRunnerandTestFormulae. - Add/adjust RSpec coverage for
TestFormulaeartifact downloading andJunitXML generation, and update existingSetupspec to use the correct args type.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/test_bot/test_runner.rbi | Removed RBI shim for TestRunner. |
| Library/Homebrew/test_bot/test_runner.rb | Converted to typed: strict; added signatures and stricter typing around test orchestration. |
| Library/Homebrew/test_bot/test_formulae.rbi | Removed RBI shim for TestFormulae. |
| Library/Homebrew/test_bot/test_formulae.rb | Converted to typed: strict; added signatures/annotations and adjusted nil-handling. |
| Library/Homebrew/test_bot/test_cleanup.rb | Converted to typed: strict; added signatures and tightened string conversions for command execution. |
| Library/Homebrew/test_bot/test.rb | Converted to typed: strict; added signatures and typed ivars for step tracking/execution. |
| Library/Homebrew/test_bot/tap_syntax.rb | Converted to typed: strict; added signatures and tap nil-handling. |
| Library/Homebrew/test_bot/step.rb | Converted to typed: strict; added signatures/typed ivars and tightened step execution typing. |
| Library/Homebrew/test_bot/setup.rb | Converted to typed: strict; added method signature. |
| Library/Homebrew/test_bot/junit.rb | Converted to typed: strict; added signatures and ensured REXML is required before use. |
| Library/Homebrew/test_bot/formulae_detect.rb | Converted to typed: strict; added signatures and adjusted tap/git handling. |
| Library/Homebrew/test_bot/formulae_dependents.rb | Converted to typed: strict; added signatures and typed state for dependent testing. |
| Library/Homebrew/test_bot/formulae.rb | Converted to typed: strict; added signatures/typed ivars and tightened step access (fetch(-1), T.must). |
| Library/Homebrew/test_bot/cleanup_before.rb | Converted to typed: strict; added method signature. |
| Library/Homebrew/test_bot/cleanup_after.rb | Converted to typed: strict; added method signatures. |
| Library/Homebrew/test_bot/bottles_fetch.rb | Added signatures for accessor and helpers. |
| Library/Homebrew/test_bot.rb | Converted to typed: strict; added signatures for module functions. |
| Library/Homebrew/test/test_bot/test_formulae_spec.rb | New regression spec for artifact download bookkeeping behavior. |
| Library/Homebrew/test/test_bot/setup_spec.rb | Updated requires/args type for the strict-typed TestBot command classes. |
| Library/Homebrew/test/test_bot/junit_spec.rb | New regression specs ensuring REXML is loaded and XML output includes failures correctly. |
Files not reviewed (2)
- Library/Homebrew/test_bot/test_formulae.rbi: Language not supported
- Library/Homebrew/test_bot/test_runner.rbi: Language not supported
Comments suppressed due to low confidence (2)
Library/Homebrew/test_bot/test_formulae.rb:304
Utils.safe_popen_read(git, ...)passesgitandrepositorywithout ensuring they’re Strings. Withtyped: strict,gitisT.nilable(String)(fromTest) andrepositoryis aPathname, so this will fail Sorbet and may misbehave ifgitis evernil. Please useT.must(git)/git.to_sandrepository.to_shere (and in otherUtils.*popen_readcall sites in this class).
bottle_header = "Bottle cache hit"
bottle_commit_details = if @fetched_refs&.include?(bottle_revision)
Utils.safe_popen_read(git, "-C", repository, "show", "--format=reference", bottle_revision)
else
Library/Homebrew/test_bot/test.rb:95
Test#testtakes a splat (def test(*arguments, ...)) but thesigtypesargumentsas a singleString. With Sorbet, the rest arg should be typed asT::Array[String](and ideally ensure the array contains strings before passing toStep.new). As-is, this will fail type-checking and can hide non-String arguments flowing intoStep/system_command.
sig {
params(
arguments: String,
named_args: T.nilable(T.any(String, T::Array[String])),
env: T::Hash[String, String],
verbose: T::Boolean,
ignore_failures: T::Boolean,
report_analytics: T::Boolean,
).returns(Step)
}
def test(*arguments, named_args: nil, env: {}, verbose: @verbose, ignore_failures: false,
report_analytics: false)
step = Step.new(
arguments,
named_args:,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b06d44e to
5a8c0df
Compare
6 tasks
MikeMcQuaid
approved these changes
Feb 9, 2026
Member
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Will merge once we've released 5.0.14.
Member
|
There is a type error similar before, |
Member
|
Got new error when testing deps: https://github.com/Homebrew/homebrew-core/actions/runs/21823005299/job/62963158249?pr=266550 |
6 tasks
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.
brew lgtm(style, typechecking and tests) with your changes locally?See #21506 for the previous attempt.
I've added fixes for the resulting CI failures observed by @botantony and @chenrui333 in a second commit. I've also included vibe-coded specs to prevent future regressions (confirming that the specs fail without the fixes in that commit).
The test coverage is light for TestBot Test subclasses. I can add vibe-coded specs for the other classes, if reviewers prefer. I would like to self-merge at a time that I can be available to fix other potential test failures.