From b66097fa3d3a7927eb13771786c3832ae5621b17 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sat, 16 Mar 2024 13:14:12 -0700 Subject: [PATCH 1/3] spec_helper: add check for unexpected network calls Any test that is not tagged as :needs_network and that makes a call to an unapproved method in the `Utils::Curl` module will raise an error unless that method gets mocked somehow. tests: add exceptions for tests that use curl to download local files These are acceptable ways to use curl in local, non-network tests. For those edge cases we allow you to bypass the check with :needs_utils_curl. --- Library/Homebrew/test/cask/cask_spec.rb | 6 ++++-- Library/Homebrew/test/formulary_spec.rb | 3 ++- Library/Homebrew/test/spec_helper.rb | 9 +++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/test/cask/cask_spec.rb b/Library/Homebrew/test/cask/cask_spec.rb index fea5589a37025..6558078ee0379 100644 --- a/Library/Homebrew/test/cask/cask_spec.rb +++ b/Library/Homebrew/test/cask/cask_spec.rb @@ -46,13 +46,15 @@ expect(c.token).to eq("caffeine") end - it "returns an instance of the Cask from a URL" do + it "returns an instance of the Cask from a URL", :needs_utils_curl do + expect(Homebrew::API).not_to receive(:fetch_json_api_file) c = Cask::CaskLoader.load("file://#{tap_path}/Casks/local-caffeine.rb") expect(c).to be_a(described_class) expect(c.token).to eq("local-caffeine") end - it "raises an error when failing to download a Cask from a URL" do + it "raises an error when failing to download a Cask from a URL", :needs_utils_curl do + expect(Homebrew::API).not_to receive(:fetch_json_api_file) expect do Cask::CaskLoader.load("file://#{tap_path}/Casks/notacask.rb") end.to raise_error(Cask::CaskUnavailableError) diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 86631a3a667dd..3bc5c5ee05427 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -119,7 +119,8 @@ class Wrong#{described_class.class_s(formula_name)} < Formula expect(described_class.factory(formula_path)).to be_a(Formula) end - it "returns a Formula when given a URL" do + it "returns a Formula when given a URL", :needs_utils_curl do + expect(Homebrew::API).not_to receive(:fetch_json_api_file) formula = described_class.factory("file://#{formula_path}") expect(formula).to be_a(Formula) end diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index bb08dd6e5a661..47e5eff4016fc 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -165,6 +165,15 @@ skip "Requires network connection." unless ENV["HOMEBREW_TEST_ONLINE"] end + config.before do |example| + next if example.metadata.key?(:needs_network) + next if example.metadata.key?(:needs_utils_curl) + + allow(Utils::Curl).to receive(:curl_executable).and_raise(<<~ERROR) + Unexpected call to Utils::Curl.curl_executable without setting :needs_network or :needs_utils_curl. + ERROR + end + config.before(:each, :needs_svn) do svn_shim = HOMEBREW_SHIMS_PATH/"shared/svn" skip "Subversion is not installed." unless quiet_system svn_shim, "--version" From ad35db4b24997c7999e9c1c0a37ff66d2e985ccd Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sat, 16 Mar 2024 13:17:54 -0700 Subject: [PATCH 2/3] tests: fix tests that make unexpected network calls These were found with the Utils::Curl check and just turning off the network on my computer and running the entire test suite. --- Library/Homebrew/cli/named_args.rb | 8 ++++++-- Library/Homebrew/test/cask/info_spec.rb | 5 +++++ Library/Homebrew/test/formulary_spec.rb | 4 ++++ Library/Homebrew/test/tap_spec.rb | 2 +- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb index 1b30ef7d6b9f2..284c18aa9bbe4 100644 --- a/Library/Homebrew/cli/named_args.rb +++ b/Library/Homebrew/cli/named_args.rb @@ -256,11 +256,15 @@ def to_paths(only: parent&.only_formula_or_cask, recurse_tap: false) paths = [] if formula_path.exist? || - (!CoreTap.instance.installed? && Homebrew::API::Formula.all_formulae.key?(path.basename.to_s)) + (!Homebrew::EnvConfig.no_install_from_api? && + !CoreTap.instance.installed? && + Homebrew::API::Formula.all_formulae.key?(path.basename.to_s)) paths << formula_path end if cask_path.exist? || - (!CoreCaskTap.instance.installed? && Homebrew::API::Cask.all_casks.key?(path.basename.to_s)) + (!Homebrew::EnvConfig.no_install_from_api? && + !CoreCaskTap.instance.installed? && + Homebrew::API::Cask.all_casks.key?(path.basename.to_s)) paths << cask_path end diff --git a/Library/Homebrew/test/cask/info_spec.rb b/Library/Homebrew/test/cask/info_spec.rb index 2c5a5aefc0195..8710a8ba70e21 100644 --- a/Library/Homebrew/test/cask/info_spec.rb +++ b/Library/Homebrew/test/cask/info_spec.rb @@ -3,6 +3,11 @@ require "utils" RSpec.describe Cask::Info, :cask do + before do + # Prevent unnecessary network requests in `Utils::Analytics.cask_output` + ENV["HOMEBREW_NO_ANALYTICS"] = "1" + end + it "displays some nice info about the specified Cask" do expect do described_class.info(Cask::CaskLoader.load("local-transmission")) diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 3bc5c5ee05427..8e34f0fb42179 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -384,6 +384,10 @@ def formula_json_contents(extra_items = {}) before do ENV.delete("HOMEBREW_NO_INSTALL_FROM_API") + # avoid unnecessary network calls + allow(Homebrew::API::Formula).to receive_messages(all_aliases: {}, all_renames: {}) + allow(CoreTap.instance).to receive(:tap_migrations).and_return({}) + # don't try to load/fetch gcc/glibc allow(DevelopmentTools).to receive_messages(needs_libc_formula?: false, needs_compiler_formula?: false) end diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index 8ea9116037af9..44a771fc31a17 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -192,7 +192,7 @@ def setup_completion(link:) end describe "#remote" do - it "returns the remote URL" do + it "returns the remote URL", :needs_network do setup_git_repo expect(homebrew_foo_tap.remote).to eq("https://github.com/Homebrew/homebrew-foo") From 74aea8e92d78201db65b6eedfd9ec60954264fc1 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Tue, 19 Mar 2024 22:18:02 -0700 Subject: [PATCH 3/3] spec_helper: add :no_api test scope This sets the HOMEBREW_NO_INSTALL_FROM_API environment variable to prevent the selected tests from using the API. We will need this as we transition to having the API be enabled by default when running the tests but it's also nice as a sanity check with the :needs_utils_curl scope in a few places. --- .../test/cask/cask_loader/from_api_loader_spec.rb | 6 +----- Library/Homebrew/test/cask/cask_loader_spec.rb | 12 ++---------- Library/Homebrew/test/cask/cask_spec.rb | 6 ++---- Library/Homebrew/test/formulary_spec.rb | 3 +-- Library/Homebrew/test/spec_helper.rb | 4 ++++ Library/Homebrew/test/tap_spec.rb | 3 +-- 6 files changed, 11 insertions(+), 23 deletions(-) diff --git a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb index 7e567e95fe6d4..eb21387245271 100644 --- a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb +++ b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb @@ -22,11 +22,7 @@ describe ".can_load?" do include_context "with API setup", "test-opera" - context "when not using the API" do - before do - ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" - end - + context "when not using the API", :no_api do it "returns false" do expect(described_class.try_new(token)).to be_nil end diff --git a/Library/Homebrew/test/cask/cask_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader_spec.rb index e7fcc9696ec78..62be102cea7b3 100644 --- a/Library/Homebrew/test/cask/cask_loader_spec.rb +++ b/Library/Homebrew/test/cask/cask_loader_spec.rb @@ -30,11 +30,7 @@ .and_return(cask_renames) end - context "when not using the API" do - before do - ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" - end - + context "when not using the API", :no_api do it "warns when using the short token" do expect do expect(described_class.for("version-newest")).to be_a Cask::CaskLoader::FromPathLoader @@ -67,11 +63,7 @@ end end - context "when not using the API" do - before do - ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" - end - + context "when not using the API", :no_api do context "when a cask is migrated" do let(:token) { "local-caffeine" } diff --git a/Library/Homebrew/test/cask/cask_spec.rb b/Library/Homebrew/test/cask/cask_spec.rb index 6558078ee0379..cadba6516e3d1 100644 --- a/Library/Homebrew/test/cask/cask_spec.rb +++ b/Library/Homebrew/test/cask/cask_spec.rb @@ -46,15 +46,13 @@ expect(c.token).to eq("caffeine") end - it "returns an instance of the Cask from a URL", :needs_utils_curl do - expect(Homebrew::API).not_to receive(:fetch_json_api_file) + it "returns an instance of the Cask from a URL", :needs_utils_curl, :no_api do c = Cask::CaskLoader.load("file://#{tap_path}/Casks/local-caffeine.rb") expect(c).to be_a(described_class) expect(c.token).to eq("local-caffeine") end - it "raises an error when failing to download a Cask from a URL", :needs_utils_curl do - expect(Homebrew::API).not_to receive(:fetch_json_api_file) + it "raises an error when failing to download a Cask from a URL", :needs_utils_curl, :no_api do expect do Cask::CaskLoader.load("file://#{tap_path}/Casks/notacask.rb") end.to raise_error(Cask::CaskUnavailableError) diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 8e34f0fb42179..3bb7491bdf0c5 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -119,8 +119,7 @@ class Wrong#{described_class.class_s(formula_name)} < Formula expect(described_class.factory(formula_path)).to be_a(Formula) end - it "returns a Formula when given a URL", :needs_utils_curl do - expect(Homebrew::API).not_to receive(:fetch_json_api_file) + it "returns a Formula when given a URL", :needs_utils_curl, :no_api do formula = described_class.factory("file://#{formula_path}") expect(formula).to be_a(Formula) end diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 47e5eff4016fc..70fa46df38f5b 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -174,6 +174,10 @@ ERROR end + config.before(:each, :no_api) do + ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" + end + config.before(:each, :needs_svn) do svn_shim = HOMEBREW_SHIMS_PATH/"shared/svn" skip "Subversion is not installed." unless quiet_system svn_shim, "--version" diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index 44a771fc31a17..ecfa2b8996aa3 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -488,8 +488,7 @@ def setup_completion(link:) expect(described_class.to_a).to include(CoreTap.instance) end - it "omits the core tap without the api" do - ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" + it "omits the core tap without the api", :no_api do expect(described_class.to_a).not_to include(CoreTap.instance) end end