Enable strict typing in Homebrew::TestBot (take 3)

Library/Homebrew/test/test_bot/bottles_fetch_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require "test_bot"
+require "dev-cmd/test-bot"
+
+RSpec.describe Homebrew::TestBot::BottlesFetch do
+  describe "#run!" do
+    it "accepts Utils::Bottles::Tag objects from the bottle collector" do
+      # Regression test: bottle_specification.collector.tags returns Utils::Bottles::Tag objects,
+      # not Symbols. The fetch_bottles! signature must accept Tag, not Symbol.
+      fetch = described_class.new(tap: nil, git: nil, dry_run: true, fail_fast: false, verbose: false)
+      fetch.testing_formulae = ["some-formula"]
+      tag = Utils::Bottles::Tag.new(system: :sequoia, arch: :arm64)
+      allow(fetch).to receive(:formulae_by_tag).and_return({ tag => Set["some-formula"] })
+      allow(fetch).to receive(:cleanup_during!)
+
+      fetch.run!(args: instance_double(Homebrew::Cmd::TestBotCmd::Args))
+
+      expect(fetch.steps.last).to be_passed
+      expect(fetch.steps.last.command).to include("--bottle-tag=#{tag}")
+    end
+  end
+end

Library/Homebrew/test/test_bot/formulae_spec.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require "test_bot"
+
+RSpec.describe Homebrew::TestBot::Formulae do
+  describe "#testing_portable_ruby?" do
+    it "returns false (not nil) when tap is nil" do
+      # Regression test: without `!!`, tap&.core_tap? returns nil when tap is nil,
+      # and `nil && ...` evaluates to nil, violating the T::Boolean return type.
+      Dir.mktmpdir do |tmpdir|
+        output_paths = {
+          bottle:                     Pathname.new("#{tmpdir}/bottle.txt"),
+          linkage:                    Pathname.new("#{tmpdir}/linkage.txt"),
+          skipped_or_failed_formulae: Pathname.new("#{tmpdir}/skipped.txt"),
+        }
+        formulae = described_class.new(
+          tap: nil, git: "git", dry_run: true, fail_fast: false, verbose: false,
+          output_paths:
+        )
+
+        result = formulae.send(:testing_portable_ruby?)
+        expect(result).to be(false)
+      end
+    end
+  end
+
+  describe "#verify_local_bottles" do
+    it "returns false (not nil) when testing portable ruby" do
+      # Regression test: the early return for portable ruby must be `return false`,
+      # not bare `return` (which returns nil), to satisfy the T::Boolean return type.
+      Dir.mktmpdir do |tmpdir|
+        output_paths = {
+          bottle:                     Pathname.new("#{tmpdir}/bottle.txt"),
+          linkage:                    Pathname.new("#{tmpdir}/linkage.txt"),
+          skipped_or_failed_formulae: Pathname.new("#{tmpdir}/skipped.txt"),
+        }
+        formulae = described_class.new(
+          tap: CoreTap.instance, git: "git", dry_run: true, fail_fast: false, verbose: false,
+          output_paths:
+        )
+        formulae.testing_formulae = ["portable-ruby"]
+
+        result = formulae.send(:verify_local_bottles)
+        expect(result).to be(false)
+      end
+    end
+  end
+end

Library/Homebrew/test/test_bot/junit_spec.rb

@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+require "test_bot"
+
+RSpec.describe Homebrew::TestBot::Junit do
+  # Regression test: Junit requires REXML before use. Without the require calls in #initialize,
+  # environments that don't load REXML elsewhere (e.g. Linux CI) raise
+  # "uninitialized constant Homebrew::TestBot::Junit::REXML".
+  describe "#initialize and #build and #write" do
+    it "loads REXML and produces valid JUnit XML without NameError" do
+      start_time = Time.utc(2024, 1, 15, 12, 0, 0)
+      step = instance_double(
+        Homebrew::TestBot::Step,
+        command_short: "audit",
+        status:        :passed,
+        time:          1.5,
+        start_time:    start_time,
+        passed?:       true,
+        command:       ["brew", "audit", "foo"],
+      )
+      test = instance_double(Homebrew::TestBot::Test, steps: [step])
+
+      junit = described_class.new([test])
+      junit.build(filters: ["audit"])
+
+      Dir.mktmpdir do |tmpdir|
+        path = "#{tmpdir}/junit.xml"
+        junit.write(path)
+
+        expect(File).to exist(path)
+        content = File.read(path)
+        expect(content).to include("<?xml")
+        expect(content).to include("testsuites")
+        expect(content).to include("testcase")
+        expect(content).to include("name='audit'")
+      end
+    end
+
+    it "includes failure element when a step did not pass" do
+      start_time = Time.utc(2024, 1, 15, 12, 0, 0)
+      step = instance_double(
+        Homebrew::TestBot::Step,
+        command_short: "test",
+        status:        :failed,
+        time:          2.0,
+        start_time:    start_time,
+        passed?:       false,
+        command:       ["brew", "test", "foo"],
+      )
+      test = instance_double(Homebrew::TestBot::Test, steps: [step])
+
+      junit = described_class.new([test])
+      junit.build(filters: ["test"])
+
+      Dir.mktmpdir do |tmpdir|
+        path = "#{tmpdir}/junit.xml"
+        junit.write(path)
+
+        content = File.read(path)
+        expect(content).to include("<failure ")
+        expect(content).to include("failed")
+      end
+    end
+  end
+end

Library/Homebrew/test/test_bot/setup_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require "test_bot"
+require "dev-cmd/test-bot"
 
 RSpec.describe Homebrew::TestBot::Setup do
   subject(:setup) { described_class.new }
@@ -11,7 +11,7 @@
         .exactly(3).times
         .and_return(instance_double(Homebrew::TestBot::Step, passed?: true))
 
-      expect(setup.run!(args: instance_double(Homebrew::CLI::Args)).passed?).to be(true)
+      expect(setup.run!(args: instance_double(Homebrew::Cmd::TestBotCmd::Args)).passed?).to be(true)
     end
   end
 end

Library/Homebrew/test/test_bot/test_cleanup_spec.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "dev-cmd/test-bot"
+
+RSpec.describe Homebrew::TestBot::CleanupAfter do
+  # Regression test: checkout_branch_if_needed, reset_if_needed, and clean_if_needed
+  # expect a String (repository path). Passing HOMEBREW_REPOSITORY (Pathname) would cause
+  # "Parameter 'repository': Expected type String, got type Pathname" in strict typing.
+  describe "#run!" do
+    it "passes a String to checkout_branch_if_needed, reset_if_needed, and clean_if_needed when tap is set" do
+      cleanup = described_class.new(
+        tap:       CoreTap.instance,
+        git:       "git",
+        dry_run:   true,
+        fail_fast: false,
+        verbose:   false,
+      )
+
+      # Stub to avoid actual filesystem and process operations.
+      allow(FileUtils).to receive(:chmod_R)
+      allow(cleanup).to receive(:info_header)
+      allow(cleanup).to receive(:delete_or_move)
+      allow(cleanup).to receive(:test)
+      allow(cleanup).to receive_messages(repository:   Pathname.new("/nonexistent_#{SecureRandom.hex(8)}"),
+                                         quiet_system: false)
+      allow(Keg).to receive(:must_be_writable_directories).and_return([])
+      allow(Pathname).to receive(:glob).and_return([])
+
+      expect(cleanup).to receive(:checkout_branch_if_needed).with(String)
+      expect(cleanup).to receive(:reset_if_needed).with(String)
+      expect(cleanup).to receive(:clean_if_needed).with(String)
+
+      args = double(test_default_formula?: false, local?: false)
+      with_env("HOMEBREW_GITHUB_ACTIONS" => nil, "GITHUB_ACTIONS" => nil) do
+        cleanup.run!(args:)
+      end
+    end
+  end
+end

Library/Homebrew/test/test_bot/test_formulae_spec.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require "test_bot"
+require "utils/github/artifacts"
+
+RSpec.describe Homebrew::TestBot::TestFormulae do
+  subject(:test_formulae) do
+    described_class.new(tap: nil, git: nil, dry_run: false, fail_fast: false, verbose: false)
+  end
+
+  describe "#download_artifacts_from_previous_run!" do
+    it "does not raise KeyError when accessing downloaded_artifacts for a new SHA" do
+      # Regression test: @downloaded_artifacts uses a hash with a default block, so we must use
+      # [] (not .fetch) when accessing by SHA. Using .fetch(sha) would raise KeyError for new SHAs.
+      new_sha = "8e624f21ac73d02a609cfec1ce620ccfee3aa97c"
+      allow(GitHub).to receive(:pull_request_labels).with("owner", "repo", 1).and_return([])
+      allow(GitHub::API).to receive_messages(credentials_type: :pat, open_graphql: { "repository" => {
+        "object" => {
+          "checkSuites" => {
+            "nodes" => [
+              {
+                "status"      => "COMPLETED",
+                "updatedAt"   => "2024-01-01T00:00:00Z",
+                "workflowRun" => { "databaseId" => 1, "event" => "pull_request", "workflow" => { "name" => "CI" } },
+                "checkRuns"   => { "nodes" => [{ "name" => "conclusion", "status" => "COMPLETED" }] },
+              },
+            ],
+          },
+        },
+      } })
+      allow(test_formulae).to receive_messages(
+        previous_github_sha:  new_sha,
+        github_event_payload: { "pull_request" => { "number" => 1 } },
+        artifact_metadata:    [
+          {
+            "name"                 => "bottles",
+            "archive_download_url" => "https://example.com/artifact",
+            "id"                   => 1,
+          },
+        ],
+      )
+      allow(GitHub).to receive(:download_artifact)
+
+      Dir.mktmpdir do |tmpdir|
+        Dir.chdir(tmpdir) do
+          with_env("GITHUB_REPOSITORY" => "owner/repo") do
+            test_formulae.send(:download_artifacts_from_previous_run!, "bottles*", dry_run: false)
+          end
+        end
+      end
+
+      # Proves we passed the @downloaded_artifacts[sha] access for a new SHA without KeyError.
+      downloaded = test_formulae.instance_variable_get(:@downloaded_artifacts)
+      expect(downloaded[new_sha]).to include("bottles")
+    end
+  end
+end

Library/Homebrew/test/test_bot/test_spec.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require "test_bot"
+
+RSpec.describe Homebrew::TestBot::Test do
+  describe "#test" do
+    it "converts Pathname arguments to strings" do
+      # Regression test: callers like TestCleanup pass Pathname objects (e.g. repository)
+      # as positional arguments. The `test` method must coerce them to String before
+      # forwarding to Step.new, which expects T::Array[String].
+      test_instance = described_class.new(dry_run: true)
+
+      step = test_instance.send(:test, "git", "-C", Pathname.new("/some/path"), "status")
+
+      expect(step.command).to eq(["git", "-C", "/some/path", "status"])
+      expect(step).to be_passed
+    end
+  end
+end

Library/Homebrew/test_bot.rb

@@ -1,4 +1,4 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 require "test_bot/step"
@@ -23,14 +23,17 @@ module TestBot
 
     HOMEBREW_TAP_REGEX = %r{^([\w-]+)/homebrew-([\w-]+)$}
 
+    sig { params(args: Homebrew::Cmd::TestBotCmd::Args).returns(T::Boolean) }
     def cleanup?(args)
       args.cleanup? || GitHub::Actions.env_set?
     end
 
+    sig { params(args: Homebrew::Cmd::TestBotCmd::Args).returns(T::Boolean) }
     def local?(args)
       args.local? || GitHub::Actions.env_set?
     end
 
+    sig { params(tap: T.nilable(String)).returns(T.nilable(Tap)) }
     def resolve_test_tap(tap = nil)
       return Tap.fetch(tap) if tap
 
@@ -52,6 +55,7 @@ def resolve_test_tap(tap = nil)
       end
     end
 
+    sig { params(args: Homebrew::Cmd::TestBotCmd::Args).void }
     def run!(args)
       $stdout.sync = true
       $stderr.sync = true

Library/Homebrew/test_bot/bottles_fetch.rb

@@ -1,11 +1,13 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 module Homebrew
   module TestBot
     class BottlesFetch < TestFormulae
+      sig { returns(T::Array[String]) }
       attr_accessor :testing_formulae
 
+      sig { params(args: Homebrew::Cmd::TestBotCmd::Args).void }
       def run!(args:)
         info_header "Testing formulae:"
         puts testing_formulae
@@ -19,6 +21,7 @@ def run!(args:)
 
       private
 
+      sig { returns(T::Hash[Utils::Bottles::Tag, T::Set[String]]) }
       def formulae_by_tag
         tags = Hash.new { |hash, key| hash[key] = Set.new }
 
@@ -38,6 +41,7 @@ def formulae_by_tag
         tags
       end
 
+      sig { params(tag: Utils::Bottles::Tag, formulae: T::Set[String], args: Homebrew::Cmd::TestBotCmd::Args).void }
       def fetch_bottles!(tag, formulae, args:)
         test_header(:BottlesFetch, method: "fetch_bottles!(#{tag})")
 

Library/Homebrew/test_bot/cleanup_after.rb

@@ -1,9 +1,10 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 module Homebrew
   module TestBot
     class CleanupAfter < TestCleanup
+      sig { params(args: Homebrew::Cmd::TestBotCmd::Args).void }
       def run!(args:)
         if ENV["HOMEBREW_GITHUB_ACTIONS"].present? && ENV["GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED"].blank? &&
            # don't need to do post-build cleanup unless testing test-bot itself.
@@ -27,6 +28,7 @@ def run!(args:)
 
       private
 
+      sig { void }
       def pkill_if_needed
         pgrep = ["pgrep", "-f", HOMEBREW_CELLAR.to_s]
 

Library/Homebrew/test_bot/cleanup_before.rb

@@ -1,9 +1,10 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 module Homebrew
   module TestBot
     class CleanupBefore < TestCleanup
+      sig { params(args: Homebrew::Cmd::TestBotCmd::Args).void }
       def run!(args:)
         test_header(:CleanupBefore)