From 13fb14a95f6d0c909451e94e96b14447ccabdfcb Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Thu, 27 Jul 2017 01:52:21 +0530 Subject: [PATCH 1/6] Added tests fir utils/svn --- Library/Homebrew/test/utils/svn_spec.rb | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Library/Homebrew/test/utils/svn_spec.rb diff --git a/Library/Homebrew/test/utils/svn_spec.rb b/Library/Homebrew/test/utils/svn_spec.rb new file mode 100644 index 0000000000..35f8917aaf --- /dev/null +++ b/Library/Homebrew/test/utils/svn_spec.rb @@ -0,0 +1,33 @@ +require "utils/svn" + +describe Utils do + describe "#self.svn_available?" do + it "processes value when @svn is not defined" do + expect(described_class.svn_available?).to be_truthy + end + + it "returns value of @svn when @svn is defined" do + described_class.instance_variable_set(:@svn, true) + expect(described_class.svn_available?).to be_truthy + end + end + + describe "#self.svn_remote_exists" do + let(:url) { "https://dl.bintray.com/homebrew/mirror/" } + + it "gives true when @svn is false" do + allow_any_instance_of(Process::Status).to receive(:success?).and_return(false) + described_class.instance_variable_set(:@svn, false) + expect(described_class.svn_remote_exists(url)).to be_truthy + end + + it "gives false when url is obscure" do + expect(described_class.svn_remote_exists(url)).to be_falsy + end + + it "gives true when quiet_system succeeds with given url" do + allow_any_instance_of(Process::Status).to receive(:success?).and_return(true) + expect(described_class.svn_remote_exists(url)).to be_truthy + end + end +end From cf96d8d9700d0869d52add879f25179509b3773c Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Thu, 27 Jul 2017 04:18:43 +0530 Subject: [PATCH 2/6] Improved tests for svn_available? --- Library/Homebrew/test/utils/svn_spec.rb | 19 ++++++++++++------- Library/Homebrew/utils/svn.rb | 3 +-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/test/utils/svn_spec.rb b/Library/Homebrew/test/utils/svn_spec.rb index 35f8917aaf..3ff9ac6555 100644 --- a/Library/Homebrew/test/utils/svn_spec.rb +++ b/Library/Homebrew/test/utils/svn_spec.rb @@ -2,11 +2,17 @@ require "utils/svn" describe Utils do describe "#self.svn_available?" do - it "processes value when @svn is not defined" do + it "returns true if svn --version command succeeds" do + allow_any_instance_of(Process::Status).to receive(:success?).and_return(true) expect(described_class.svn_available?).to be_truthy end - it "returns value of @svn when @svn is defined" do + it "returns false if svn --version command does not succeed" do + allow_any_instance_of(Process::Status).to receive(:success?).and_return(false) + expect(described_class.svn_available?).to be_falsey + end + + it "returns svn version if already set" do described_class.instance_variable_set(:@svn, true) expect(described_class.svn_available?).to be_truthy end @@ -15,17 +21,16 @@ describe Utils do describe "#self.svn_remote_exists" do let(:url) { "https://dl.bintray.com/homebrew/mirror/" } - it "gives true when @svn is false" do - allow_any_instance_of(Process::Status).to receive(:success?).and_return(false) + it "returns true when svn is not available" do described_class.instance_variable_set(:@svn, false) expect(described_class.svn_remote_exists(url)).to be_truthy end - it "gives false when url is obscure" do - expect(described_class.svn_remote_exists(url)).to be_falsy + it "returns false when remote does not exist" do + expect(described_class.svn_remote_exists(url)).to be_falsey end - it "gives true when quiet_system succeeds with given url" do + it "returns true when remote exists" do allow_any_instance_of(Process::Status).to receive(:success?).and_return(true) expect(described_class.svn_remote_exists(url)).to be_truthy end diff --git a/Library/Homebrew/utils/svn.rb b/Library/Homebrew/utils/svn.rb index fb49ac2e99..7a99551b33 100644 --- a/Library/Homebrew/utils/svn.rb +++ b/Library/Homebrew/utils/svn.rb @@ -1,7 +1,6 @@ module Utils def self.svn_available? - return @svn if instance_variable_defined?(:@svn) - @svn = quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version" + @svn ||= quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version" end def self.svn_remote_exists(url) From 53be6bb4bd69eaf8cc8e5c3c1ac9ecd1ddb1582c Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Thu, 27 Jul 2017 04:40:27 +0530 Subject: [PATCH 3/6] Added check for svn availability --- Library/Homebrew/dev-cmd/audit.rb | 4 ++- Library/Homebrew/test/utils/svn_spec.rb | 43 +++++++++++++------------ Library/Homebrew/utils/svn.rb | 3 +- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 2c93364812..0e4500a124 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -1228,7 +1228,9 @@ class ResourceAuditor end elsif strategy <= SubversionDownloadStrategy next unless DevelopmentTools.subversion_handles_most_https_certificates? - unless Utils.svn_remote_exists url + if !Utils.svn_available? + problem "No valid version of svn found" + elsif Utils.svn_remote_exists url problem "The URL #{url} is not a valid svn URL" end end diff --git a/Library/Homebrew/test/utils/svn_spec.rb b/Library/Homebrew/test/utils/svn_spec.rb index 3ff9ac6555..6f841cc92c 100644 --- a/Library/Homebrew/test/utils/svn_spec.rb +++ b/Library/Homebrew/test/utils/svn_spec.rb @@ -2,37 +2,40 @@ require "utils/svn" describe Utils do describe "#self.svn_available?" do - it "returns true if svn --version command succeeds" do - allow_any_instance_of(Process::Status).to receive(:success?).and_return(true) - expect(described_class.svn_available?).to be_truthy + before(:each) do + if described_class.instance_variable_defined?(:@svn) + described_class.send(:remove_instance_variable, :@svn) + end end - it "returns false if svn --version command does not succeed" do - allow_any_instance_of(Process::Status).to receive(:success?).and_return(false) - expect(described_class.svn_available?).to be_falsey - end - - it "returns svn version if already set" do - described_class.instance_variable_set(:@svn, true) + it "returns svn version if svn available" do expect(described_class.svn_available?).to be_truthy end end describe "#self.svn_remote_exists" do - let(:url) { "https://dl.bintray.com/homebrew/mirror/" } - it "returns true when svn is not available" do - described_class.instance_variable_set(:@svn, false) - expect(described_class.svn_remote_exists(url)).to be_truthy + allow(Utils).to receive(:svn_available?).and_return(false) + expect(described_class.svn_remote_exists("blah")).to be_truthy end - it "returns false when remote does not exist" do - expect(described_class.svn_remote_exists(url)).to be_falsey - end + context "when svn is available" do + before do + allow(Utils).to receive(:svn_available?).and_return(true) + end - it "returns true when remote exists" do - allow_any_instance_of(Process::Status).to receive(:success?).and_return(true) - expect(described_class.svn_remote_exists(url)).to be_truthy + it "returns false when remote does not exist" do + expect(described_class.svn_remote_exists(HOMEBREW_CACHE/"install")).to be_falsey + end + + it "returns true when remote exists", :needs_network do + remote = "http://github.com/Homebrew/install" + svn = HOMEBREW_SHIMS_PATH/"scm/svn" + + HOMEBREW_CACHE.cd { system svn, "checkout", remote } + + expect(described_class.svn_remote_exists(HOMEBREW_CACHE/"install")).to be_truthy + end end end end diff --git a/Library/Homebrew/utils/svn.rb b/Library/Homebrew/utils/svn.rb index 7a99551b33..fb49ac2e99 100644 --- a/Library/Homebrew/utils/svn.rb +++ b/Library/Homebrew/utils/svn.rb @@ -1,6 +1,7 @@ module Utils def self.svn_available? - @svn ||= quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version" + return @svn if instance_variable_defined?(:@svn) + @svn = quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version" end def self.svn_remote_exists(url) From d9c0b8ce9fb65c288d2192f9db0efb93a2bd4d52 Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Thu, 24 Aug 2017 01:30:29 +0530 Subject: [PATCH 4/6] Added clear_svn_version_cache --- Library/Homebrew/utils/svn.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Library/Homebrew/utils/svn.rb b/Library/Homebrew/utils/svn.rb index fb49ac2e99..150b7eee74 100644 --- a/Library/Homebrew/utils/svn.rb +++ b/Library/Homebrew/utils/svn.rb @@ -1,4 +1,8 @@ module Utils + def self.clear_svn_version_cache + remove_instance_variable(:@svn) if instance_variable_defined?(:@svn) + end + def self.svn_available? return @svn if instance_variable_defined?(:@svn) @svn = quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version" From d533973395e549c558c2282a46acc12df4e7d6da Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Thu, 24 Aug 2017 01:31:12 +0530 Subject: [PATCH 5/6] Used clear_svn_version_cache to remove @svn --- Library/Homebrew/test/utils/svn_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Library/Homebrew/test/utils/svn_spec.rb b/Library/Homebrew/test/utils/svn_spec.rb index 6f841cc92c..4edb365a03 100644 --- a/Library/Homebrew/test/utils/svn_spec.rb +++ b/Library/Homebrew/test/utils/svn_spec.rb @@ -3,9 +3,7 @@ require "utils/svn" describe Utils do describe "#self.svn_available?" do before(:each) do - if described_class.instance_variable_defined?(:@svn) - described_class.send(:remove_instance_variable, :@svn) - end + described_class.clear_svn_version_cache end it "returns svn version if svn available" do From 3d8873ca5b6270351ac8e4260fba8a41f8c0fe04 Mon Sep 17 00:00:00 2001 From: mansimarkaur Date: Tue, 29 Aug 2017 16:14:00 +0530 Subject: [PATCH 6/6] url skipped if svn not available when auditing urls --- Library/Homebrew/dev-cmd/audit.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 0e4500a124..be332481ca 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -1228,9 +1228,8 @@ class ResourceAuditor end elsif strategy <= SubversionDownloadStrategy next unless DevelopmentTools.subversion_handles_most_https_certificates? - if !Utils.svn_available? - problem "No valid version of svn found" - elsif Utils.svn_remote_exists url + next unless Utils.svn_available? + if Utils.svn_remote_exists url problem "The URL #{url} is not a valid svn URL" end end