Merge pull request #22390 from Homebrew/test-speedup-slow-examples

Speed up slow specs

Author: MikeMcQuaid

AGENTS.md

@@ -27,7 +27,7 @@ When running Ruby directly (e.g. `ruby -e ...`, `gem`, profiling tools), never u
 ### Development Flow
 
 - Write new code (using Sorbet `sig` type signatures and `typed: strict` for new files).
-- Write new tests (avoid more than one `:integration_test` per command for speed; add another only for essential core functionality in essential non-developer commands). Try `typed: true` as a baseline but revert to `typed: false` if there are not easily fixable errors.
+- Write new tests (use at most one `:integration_test` per command, make it a happy-path test and keep it as fast as possible; add another only for essential core functionality in essential non-developer commands). Try `typed: true` as a baseline but revert to `typed: false` if there are not easily fixable errors.
   Write fast tests by preferring a single `expect` per unit test and combine expectations in a single test when it is an integration test or has non-trivial `before` for test setup.
 - When adding or tightening tests, verify them with a red/green cycle using the exact `--only=file:line` target for the example you changed.
 - Formula classes created in specs may be frozen; avoid stubbing class methods on them with RSpec mocks and prefer instance-level stubs or test setup that does not require class-method stubbing.

Library/Homebrew/dev-cmd/extract.rb

@@ -174,67 +174,86 @@ def formula_at_revision(repo, name, file, rev)
 
       sig { params(_block: T.proc.void).returns(T.untyped) }
       def with_monkey_patch(&_block)
-        # Since `method_defined?` is not a supported type guard, the use of `alias_method` below is not typesafe:
+        DependencyCollector.clear_cache
+
         BottleSpecification.class_eval do
-          T.unsafe(self).alias_method :old_method_missing, :method_missing if method_defined?(:method_missing)
+          if method_defined?(:method_missing) || private_method_defined?(:method_missing)
+            send(:alias_method, :old_method_missing, :method_missing)
+            send(:private, :old_method_missing)
+          end
           define_method(:method_missing) do |*_|
             # do nothing
           end
+          send(:private, :method_missing)
         end
 
         Module.class_eval do
-          T.unsafe(self).alias_method :old_method_missing, :method_missing if method_defined?(:method_missing)
+          if method_defined?(:method_missing) || private_method_defined?(:method_missing)
+            send(:alias_method, :old_method_missing, :method_missing)
+            send(:private, :old_method_missing)
+          end
           define_method(:method_missing) do |*_|
             # do nothing
           end
+          send(:private, :method_missing)
         end
 
         Resource.class_eval do
-          T.unsafe(self).alias_method :old_method_missing, :method_missing if method_defined?(:method_missing)
+          if method_defined?(:method_missing) || private_method_defined?(:method_missing)
+            send(:alias_method, :old_method_missing, :method_missing)
+            send(:private, :old_method_missing)
+          end
           define_method(:method_missing) do |*_|
             # do nothing
           end
+          send(:private, :method_missing)
         end
 
         DependencyCollector.class_eval do
-          if method_defined?(:parse_symbol_spec)
-            T.unsafe(self).alias_method :old_parse_symbol_spec,
-                                        :parse_symbol_spec
+          if method_defined?(:parse_symbol_spec) || private_method_defined?(:parse_symbol_spec)
+            send(:alias_method, :old_parse_symbol_spec, :parse_symbol_spec)
+            send(:private, :old_parse_symbol_spec)
           end
           define_method(:parse_symbol_spec) do |*_|
             # do nothing
           end
+          send(:private, :parse_symbol_spec)
         end
 
         yield
       ensure
         BottleSpecification.class_eval do
-          if method_defined?(:old_method_missing)
-            T.unsafe(self).alias_method :method_missing, :old_method_missing
-            T.unsafe(self).undef :old_method_missing
+          if method_defined?(:old_method_missing) || private_method_defined?(:old_method_missing)
+            send(:alias_method, :method_missing, :old_method_missing)
+            send(:private, :method_missing)
+            send(:undef_method, :old_method_missing)
           end
         end
 
         Module.class_eval do
-          if method_defined?(:old_method_missing)
-            T.unsafe(self).alias_method :method_missing, :old_method_missing
-            T.unsafe(self).undef :old_method_missing
+          if method_defined?(:old_method_missing) || private_method_defined?(:old_method_missing)
+            send(:alias_method, :method_missing, :old_method_missing)
+            send(:private, :method_missing)
+            send(:undef_method, :old_method_missing)
           end
         end
 
         Resource.class_eval do
-          if method_defined?(:old_method_missing)
-            T.unsafe(self).alias_method :method_missing, :old_method_missing
-            T.unsafe(self).undef :old_method_missing
+          if method_defined?(:old_method_missing) || private_method_defined?(:old_method_missing)
+            send(:alias_method, :method_missing, :old_method_missing)
+            send(:private, :method_missing)
+            send(:undef_method, :old_method_missing)
           end
         end
 
         DependencyCollector.class_eval do
-          if method_defined?(:old_parse_symbol_spec)
-            T.unsafe(self).alias_method :parse_symbol_spec, :old_parse_symbol_spec
-            T.unsafe(self).undef :old_parse_symbol_spec
+          if method_defined?(:old_parse_symbol_spec) || private_method_defined?(:old_parse_symbol_spec)
+            send(:alias_method, :parse_symbol_spec, :old_parse_symbol_spec)
+            send(:private, :parse_symbol_spec)
+            send(:undef_method, :old_parse_symbol_spec)
           end
         end
+        DependencyCollector.clear_cache
       end
     end
   end

Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb

@@ -290,6 +290,7 @@
     it "is supported" do
       allow(subject).to receive(:running_processes).with(bundle_id)
                                                    .and_return(unix_pids.map { |pid| [pid, 0, bundle_id] })
+      allow(subject).to receive(:sleep).with(3)
 
       signals.each do |signal|
         expect(Process).to receive(:kill).with(signal, *unix_pids).and_return(1)

Library/Homebrew/test/cask/installer_spec.rb

@@ -4,6 +4,14 @@
 RSpec.describe Cask::Installer, :cask do
   let(:klass) { Cask::Installer }
 
+  def stub_dmg_extraction
+    allow(UnpackStrategy::Dmg).to receive(:can_extract?).and_return(true)
+    allow_any_instance_of(UnpackStrategy::Dmg).to receive(:extract_nestedly) do |_strategy, to:, **|
+      to.mkpath
+      yield to
+    end
+  end
+
   describe "install" do
     it "downloads and installs a nice fresh Cask" do
       caffeine = Cask::CaskLoader.load(cask_path("local-caffeine"))
@@ -16,6 +24,7 @@
 
     it "works with HFS+ dmg-based Casks" do
       asset = Cask::CaskLoader.load(cask_path("container-dmg"))
+      stub_dmg_extraction { |path| FileUtils.touch path/"container" }
 
       klass.new(asset).install
 
@@ -165,6 +174,7 @@
 
     it "installs a cask from a dmg file" do
       transmission = Cask::CaskLoader.load(cask_path("local-transmission"))
+      stub_dmg_extraction { |path| (path/"Transmission.app").mkpath }
 
       expect(transmission).not_to be_installed
 

Library/Homebrew/test/cask/upgrade_spec.rb

@@ -615,19 +615,23 @@ def write_info_plist(path, short_version:, bundle_version:)
   end
 
   context "when there were multiple failures" do
-    # These tests perform actual upgrades and test error handling,
-    # so they need full real installations.
+    # This test exercises upgrade error handling, so it needs installed Casks.
     before do
       [
         "outdated/bad-checksum",
         "outdated/local-transmission-zip",
         "outdated/bad-checksum2",
       ].each do |cask|
-        Cask::Installer.new(Cask::CaskLoader.load(cask_path(cask))).install
+        InstallHelper.stub_cask_installation(Cask::CaskLoader.load(cask_path(cask)))
       end
+
+      bad_checksum_2_path = Cask::CaskLoader.load("bad-checksum2").config.appdir.join("container")
+      FileUtils.rm_rf(bad_checksum_2_path)
+      FileUtils.touch(bad_checksum_2_path)
     end
 
     it "does not end the upgrade process" do
+      upgraded_tokens = []
       bad_checksum = Cask::CaskLoader.load("bad-checksum")
       bad_checksum_path = bad_checksum.config.appdir.join("Caffeine.app")
 
@@ -646,10 +650,19 @@ def write_info_plist(path, short_version:, bundle_version:)
       expect(bad_checksum_2_path).to be_a_file
       expect(bad_checksum_2.installed_version).to eq "1.2.2"
 
+      allow(klass).to receive(:upgrade_cask) do |_, new_cask, **|
+        upgraded_tokens << new_cask.token
+        raise Cask::CaskError, "failed" if new_cask.token.start_with?("bad-checksum")
+
+        InstallHelper.stub_cask_installation(new_cask)
+      end
+
       expect do
-        klass.upgrade_casks!(args:)
+        klass.upgrade_casks!(args:, skip_prefetch: true)
       end.to raise_error(Cask::MultipleCaskErrors)
 
+      expect(upgraded_tokens).to contain_exactly("bad-checksum", "bad-checksum2", "local-transmission-zip")
+
       expect(bad_checksum).to be_installed
       expect(bad_checksum_path).to be_a_directory
       expect(bad_checksum.installed_version).to eq "1.2.2"

Library/Homebrew/test/cmd/--cache_spec.rb

@@ -5,27 +5,20 @@
 require "cmd/shared_examples/args_parse"
 
 RSpec.describe Homebrew::Cmd::Cache do
+  let(:klass) { Homebrew::Cmd::Cache }
+
   it_behaves_like "parseable arguments"
 
-  it "prints all cache files for a given Formula", :integration_test do
-    expect { brew "--cache", testball }
-      .to output(%r{#{HOMEBREW_CACHE}/downloads/[\da-f]{64}--testball-}o).to_stdout
-      .and be_a_success
-    expect { brew "--cache", "--formula", testball }
+  it "prints all cache files for a given Formula" do
+    expect { klass.new(["--formula", (TEST_FIXTURE_DIR/"testball.rb").to_s]).run }
       .to output(%r{#{HOMEBREW_CACHE}/downloads/[\da-f]{64}--testball-}o).to_stdout
       .and not_to_output.to_stderr
-      .and be_a_success
   end
 
-  it "prints the cache files for a given Cask", :integration_test, :needs_macos do
-    expect { brew "--cache", cask_path("local-caffeine") }
-      .to output(%r{#{HOMEBREW_CACHE}/downloads/[\da-f]{64}--caffeine\.zip}o).to_stdout
-      .and output(/Treating #{Regexp.escape(cask_path("local-caffeine"))} as a cask/).to_stderr
-      .and be_a_success
-    expect { brew "--cache", "--cask", cask_path("local-caffeine") }
+  it "prints the cache files for a given Cask", :cask do
+    expect { klass.new(["--cask", cask_path("local-caffeine").to_s]).run }
       .to output(%r{#{HOMEBREW_CACHE}/downloads/[\da-f]{64}--caffeine\.zip}o).to_stdout
       .and not_to_output.to_stderr
-      .and be_a_success
   end
 
   it "prints the cache files for a given Formula and Cask", :integration_test, :needs_macos do

Library/Homebrew/test/cmd/--caskroom_spec.rb

@@ -5,6 +5,8 @@
 require "cmd/shared_examples/args_parse"
 
 RSpec.describe Homebrew::Cmd::Caskroom do
+  let(:klass) { Homebrew::Cmd::Caskroom }
+
   it_behaves_like "parseable arguments"
 
   it "prints Homebrew's Caskroom", :integration_test do
@@ -14,11 +16,16 @@
       .and be_a_success
   end
 
-  it "prints the Caskroom for Casks", :integration_test do
-    expect { brew "--caskroom", cask_path("local-transmission"), cask_path("local-caffeine") }
+  it "prints the Caskroom for Casks" do
+    cmd = klass.new(%w[local-transmission local-caffeine])
+    allow(cmd.args.named).to receive(:to_casks).and_return([
+      instance_double(Cask::Cask, token: "local-transmission"),
+      instance_double(Cask::Cask, token: "local-caffeine"),
+    ])
+
+    expect { cmd.run }
       .to output("#{HOMEBREW_PREFIX/"Caskroom"/"local-transmission"}\n" \
-                 "#{HOMEBREW_PREFIX/"Caskroom"/"local-caffeine\n"}").to_stdout
+                 "#{HOMEBREW_PREFIX/"Caskroom"/"local-caffeine"}\n").to_stdout
       .and not_to_output.to_stderr
-      .and be_a_success
   end
 end

Library/Homebrew/test/cmd/--cellar_spec.rb

@@ -5,6 +5,8 @@
 require "cmd/shared_examples/args_parse"
 
 RSpec.describe Homebrew::Cmd::Cellar do
+  let(:klass) { Homebrew::Cmd::Cellar }
+
   it_behaves_like "parseable arguments"
 
   it "prints Homebrew's Cellar", :integration_test do
@@ -14,10 +16,13 @@
       .and be_a_success
   end
 
-  it "prints the Cellar for a Formula", :integration_test do
-    expect { brew "--cellar", testball }
+  it "prints the Cellar for a Formula" do
+    cmd = klass.new(["testball"])
+    allow(cmd.args.named).to receive(:to_resolved_formulae)
+      .and_return([instance_double(Formula, rack: HOMEBREW_CELLAR/"testball")])
+
+    expect { cmd.run }
       .to output(%r{#{HOMEBREW_CELLAR}/testball}o).to_stdout
       .and not_to_output.to_stderr
-      .and be_a_success
   end
 end

Library/Homebrew/test/cmd/--prefix_spec.rb

@@ -5,6 +5,8 @@
 require "cmd/shared_examples/args_parse"
 
 RSpec.describe Homebrew::Cmd::Prefix do
+  let(:klass) { Homebrew::Cmd::Prefix }
+
   it_behaves_like "parseable arguments"
 
   it "prints Homebrew's prefix", :integration_test do
@@ -14,24 +16,31 @@
       .and be_a_success
   end
 
-  it "prints the prefix for a Formula", :integration_test, :needs_homebrew_core do
-    expect { brew_sh "--prefix", "wget" }
-      .to output("#{ENV.fetch("HOMEBREW_PREFIX")}/opt/wget\n").to_stdout
+  it "prints the prefix for a Formula" do
+    cmd = klass.new(["testball"])
+    allow(cmd.args.named).to receive(:to_resolved_formulae)
+      .and_return([instance_double(Formula, opt_prefix: HOMEBREW_PREFIX/"opt/testball")])
+
+    expect { cmd.run }
+      .to output("#{HOMEBREW_PREFIX}/opt/testball\n").to_stdout
       .and not_to_output.to_stderr
-      .and be_a_success
   end
 
-  it "errors if the given Formula doesn't exist", :integration_test do
-    expect { brew "--prefix", "nonexistent" }
-      .to output(/No available formula/).to_stderr
-      .and not_to_output.to_stdout
-      .and be_a_failure
+  it "errors if the given Formula doesn't exist" do
+    cmd = klass.new(["nonexistent"])
+    allow(cmd.args.named).to receive(:to_resolved_formulae)
+      .and_raise(FormulaUnavailableError.new("nonexistent"))
+
+    expect { cmd.run }.to raise_error(FormulaUnavailableError, /nonexistent/)
   end
 
-  it "prints a warning when `--installed` is used and the given Formula is not installed", :integration_test do
-    expect { brew "--prefix", "--installed", testball }
-      .to not_to_output.to_stdout
-      .and output(/testball/).to_stderr
-      .and be_a_failure
+  it "prints a warning when `--installed` is used and the given Formula is not installed" do
+    cmd = klass.new(["--installed", "testball"])
+    allow(cmd.args.named).to receive(:to_resolved_formulae).and_return([
+      instance_double(Formula, name: "testball", opt_prefix: HOMEBREW_PREFIX/"opt/testball", optlinked?: false),
+    ])
+
+    expect { cmd.run }
+      .to raise_error(NotAKegError, /testball/)
   end
 end

Library/Homebrew/test/cmd/--repository_spec.rb

@@ -1,25 +1,29 @@
 # typed: false
 # frozen_string_literal: true
 
-RSpec.describe "brew --repository", type: :system do
-  it "prints Homebrew's repository", :integration_test do
-    expect { brew_sh "--repository" }
-      .to output("#{ENV.fetch("HOMEBREW_REPOSITORY")}\n").to_stdout
-      .and not_to_output.to_stderr
-      .and be_a_success
-  end
+require "open3"
 
-  it "prints a Tap's repository", :integration_test do
-    expect { brew_sh "--repository", "foo/bar" }
-      .to output("#{ENV.fetch("HOMEBREW_LIBRARY")}/Taps/foo/homebrew-bar\n").to_stdout
-      .and not_to_output.to_stderr
-      .and be_a_success
-  end
+RSpec.describe "brew --repository", type: :system do
+  it "prints Homebrew and Tap repositories" do
+    stdout, stderr, status = Open3.capture3(
+      {
+        "HOMEBREW_LIBRARY"    => ENV.fetch("HOMEBREW_LIBRARY"),
+        "HOMEBREW_REPOSITORY" => ENV.fetch("HOMEBREW_REPOSITORY"),
+      },
+      "/bin/bash", "-c", <<~SH,
+        source "$1"
+        homebrew---repository
+        homebrew---repository foo/bar foo/homebrew-bar
+      SH
+      "bash", (HOMEBREW_LIBRARY_PATH/"cmd/--repository.sh").to_s
+    )
 
-  it "prints a Tap's repository correctly when homebrew- prefix is supplied", :integration_test do
-    expect { brew_sh "--repository", "foo/homebrew-bar" }
-      .to output("#{ENV.fetch("HOMEBREW_LIBRARY")}/Taps/foo/homebrew-bar\n").to_stdout
-      .and not_to_output.to_stderr
-      .and be_a_success
+    expect(status).to be_success
+    expect(stdout).to eq(<<~EOS)
+      #{ENV.fetch("HOMEBREW_REPOSITORY")}
+      #{ENV.fetch("HOMEBREW_LIBRARY")}/Taps/foo/homebrew-bar
+      #{ENV.fetch("HOMEBREW_LIBRARY")}/Taps/foo/homebrew-bar
+    EOS
+    expect(stderr).to be_empty
   end
 end