Merge #6714

6714: Spec reset state and refactorings r=deivid-rodriguez a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that while working on #6713, I noticed several issues like order dependent failures, situations hard to debug, or specs doing too much and taking longer than they should.
 
### What was your diagnosis of the problem?

My diagnosis was that:

* Sometimes specs leak settings and env variable modifications.
* When a hang happens inside a subprocess, it's hard to debug because logging is not printed out anywhere.
* Some specs create unnecessary gemfiles and run `bundle install` multiple times unnecessarily.

### What is your fix for the problem, implemented in this PR?

My fix for state leaks is to reset state after each spec, for the hangs is to reorder some lines in the `sys_exec` helper (https://github.com/bundler/bundler/commit/b305a5b2524a6457b05a9d39e9526f75c98a0752), and for the unnecessary operations, to refactor the specs to avoid them.

### Why did you choose this fix out of the possible options?

I chose this fix because it seems like the best way to alleviate the issues found.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

7 files changed, 78 insertions, 46 deletions

diff --git a/spec/bundler/plugin/installer_spec.rb b/spec/bundler/plugin/installer_spec.rb
index 7ad4a9d37a..71fef76042 100644
--- a/spec/bundler/plugin/installer_spec.rb
+++ b/spec/bundler/plugin/installer_spec.rb
@@ -3,10 +3,6 @@
 RSpec.describe Bundler::Plugin::Installer do
   subject(:installer) { Bundler::Plugin::Installer.new }
 
-  before do
-    # allow(Bundler::SharedHelpers).to receive(:find_gemfile).and_return(Pathname.new("/Gemfile"))
-  end
-
   describe "cli install" do
     it "uses Gem.sources when non of the source is provided" do
       sources = double(:sources)
diff --git a/spec/bundler/settings_spec.rb b/spec/bundler/settings_spec.rb
index 1a31493e20..339428eb48 100644
--- a/spec/bundler/settings_spec.rb
+++ b/spec/bundler/settings_spec.rb
@@ -130,13 +130,15 @@ that would suck --ehhh=oh geez it looks like i might have broken bundler somehow
 
   describe "#temporary" do
     it "reset after used" do
-      Bundler.settings.set_local :no_install, true
+      Bundler.settings.set_command_option :no_install, true
 
       Bundler.settings.temporary(:no_install => false) do
         expect(Bundler.settings[:no_install]).to eq false
       end
 
       expect(Bundler.settings[:no_install]).to eq true
+
+      Bundler.settings.set_command_option :no_install, nil
     end
 
     it "returns the return value of the block" do
diff --git a/spec/bundler/source/git/git_proxy_spec.rb b/spec/bundler/source/git/git_proxy_spec.rb
index 3a29c97461..016105ccde 100644
--- a/spec/bundler/source/git/git_proxy_spec.rb
+++ b/spec/bundler/source/git/git_proxy_spec.rb
@@ -10,29 +10,33 @@ RSpec.describe Bundler::Source::Git::GitProxy do
 
   context "with configured credentials" do
     it "adds username and password to URI" do
-      Bundler.settings.temporary(uri => "u:p")
-      expect(subject).to receive(:git_retry).with(match("https://u:p@github.com/bundler/bundler.git"))
-      subject.checkout
+      Bundler.settings.temporary(uri => "u:p") do
+        expect(subject).to receive(:git_retry).with(match("https://u:p@github.com/bundler/bundler.git"))
+        subject.checkout
+      end
     end
 
     it "adds username and password to URI for host" do
-      Bundler.settings.temporary("github.com" => "u:p")
-      expect(subject).to receive(:git_retry).with(match("https://u:p@github.com/bundler/bundler.git"))
-      subject.checkout
+      Bundler.settings.temporary("github.com" => "u:p") do
+        expect(subject).to receive(:git_retry).with(match("https://u:p@github.com/bundler/bundler.git"))
+        subject.checkout
+      end
     end
 
     it "does not add username and password to mismatched URI" do
-      Bundler.settings.temporary("https://u:p@github.com/bundler/bundler-mismatch.git" => "u:p")
-      expect(subject).to receive(:git_retry).with(match(uri))
-      subject.checkout
+      Bundler.settings.temporary("https://u:p@github.com/bundler/bundler-mismatch.git" => "u:p") do
+        expect(subject).to receive(:git_retry).with(match(uri))
+        subject.checkout
+      end
     end
 
     it "keeps original userinfo" do
-      Bundler.settings.temporary("github.com" => "u:p")
-      original = "https://orig:info@github.com/bundler/bundler.git"
-      subject = described_class.new(Pathname("path"), original, "HEAD")
-      expect(subject).to receive(:git_retry).with(match(original))
-      subject.checkout
+      Bundler.settings.temporary("github.com" => "u:p") do
+        original = "https://orig:info@github.com/bundler/bundler.git"
+        subject = described_class.new(Pathname("path"), original, "HEAD")
+        expect(subject).to receive(:git_retry).with(match(original))
+        subject.checkout
+      end
     end
   end
 
diff --git a/spec/bundler/source/rubygems/remote_spec.rb b/spec/bundler/source/rubygems/remote_spec.rb
index 9a7ab42128..52fb4e7f1c 100644
--- a/spec/bundler/source/rubygems/remote_spec.rb
+++ b/spec/bundler/source/rubygems/remote_spec.rb
@@ -22,8 +22,9 @@ RSpec.describe Bundler::Source::Rubygems::Remote do
       end
 
       it "applies configured credentials" do
-        Bundler.settings.temporary(uri_no_auth.to_s => credentials)
-        expect(remote(uri_no_auth).uri).to eq(uri_with_auth)
+        Bundler.settings.temporary(uri_no_auth.to_s => credentials) do
+          expect(remote(uri_no_auth).uri).to eq(uri_with_auth)
+        end
       end
     end
 
@@ -33,8 +34,9 @@ RSpec.describe Bundler::Source::Rubygems::Remote do
       end
 
       it "does not apply given credentials" do
-        Bundler.settings.temporary(uri_no_auth.to_s => credentials)
-        expect(remote(uri_no_auth).anonymized_uri).to eq(uri_no_auth)
+        Bundler.settings.temporary(uri_no_auth.to_s => credentials) do
+          expect(remote(uri_no_auth).anonymized_uri).to eq(uri_no_auth)
+        end
       end
     end
 
@@ -44,8 +46,9 @@ RSpec.describe Bundler::Source::Rubygems::Remote do
       end
 
       it "only applies the given user" do
-        Bundler.settings.temporary(uri_no_auth.to_s => credentials)
-        expect(remote(uri_no_auth).cache_slug).to eq("gems.example.com.username.443.MD5HEX(gems.example.com.username.443./)")
+        Bundler.settings.temporary(uri_no_auth.to_s => credentials) do
+          expect(remote(uri_no_auth).cache_slug).to eq("gems.example.com.username.443.MD5HEX(gems.example.com.username.443./)")
+        end
       end
     end
   end
@@ -106,7 +109,9 @@ RSpec.describe Bundler::Source::Rubygems::Remote do
     let(:mirror_uri_with_auth) { URI("https://username:password@rubygems-mirror.org/") }
     let(:mirror_uri_no_auth) { URI("https://rubygems-mirror.org/") }
 
-    before { Bundler.settings.set_local("mirror.https://rubygems.org/", mirror_uri_with_auth.to_s) }
+    before { Bundler.settings.temporary("mirror.https://rubygems.org/" => mirror_uri_with_auth.to_s) }
+
+    after { Bundler.settings.temporary("mirror.https://rubygems.org/" => nil) }
 
     specify "#uri returns the mirror URI with credentials" do
       expect(remote(uri).uri).to eq(mirror_uri_with_auth)
@@ -135,6 +140,11 @@ RSpec.describe Bundler::Source::Rubygems::Remote do
       Bundler.settings.temporary(mirror_uri_no_auth.to_s => credentials)
     end
 
+    after do
+      Bundler.settings.temporary("mirror.https://rubygems.org/" => nil)
+      Bundler.settings.temporary(mirror_uri_no_auth.to_s => nil)
+    end
+
     specify "#uri returns the mirror URI with credentials" do
       expect(remote(uri).uri).to eq(mirror_uri_with_auth)
     end
diff --git a/spec/commands/config_spec.rb b/spec/commands/config_spec.rb
index c76135d72c..61734ef005 100644
--- a/spec/commands/config_spec.rb
+++ b/spec/commands/config_spec.rb
@@ -1,13 +1,6 @@
 # frozen_string_literal: true
 
 RSpec.describe ".bundle/config" do
-  before :each do
-    gemfile <<-G
-      source "file://#{gem_repo1}"
-      gem "rack", "1.0.0"
-    G
-  end
-
   describe "config" do
     before { bundle "config foo bar" }
 
@@ -42,7 +35,14 @@ RSpec.describe ".bundle/config" do
     end
   end
 
-  describe "BUNDLE_APP_CONFIG" do
+  describe "location" do
+    before :each do
+      gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
+
     it "can be moved with an environment variable" do
       ENV["BUNDLE_APP_CONFIG"] = tmp("foo/bar").to_s
       bundle "install", forgotten_command_line_options(:path => "vendor/bundle")
@@ -66,7 +66,12 @@ RSpec.describe ".bundle/config" do
   end
 
   describe "global" do
-    before(:each) { bundle :install }
+    before(:each) do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
 
     it "is the default" do
       bundle "config foo global"
@@ -155,7 +160,12 @@ RSpec.describe ".bundle/config" do
   end
 
   describe "local" do
-    before(:each) { bundle :install }
+    before(:each) do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
 
     it "can also be set explicitly" do
       bundle "config --local foo local"
@@ -208,7 +218,12 @@ RSpec.describe ".bundle/config" do
   end
 
   describe "env" do
-    before(:each) { bundle :install }
+    before(:each) do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
 
     it "can set boolean properties via the environment" do
       ENV["BUNDLE_FROZEN"] = "true"
@@ -276,7 +291,12 @@ RSpec.describe ".bundle/config" do
   end
 
   describe "gem mirrors" do
-    before(:each) { bundle :install }
+    before(:each) do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
 
     it "configures mirrors using keys with `mirror.`" do
       bundle "config --local mirror.http://gems.example.org http://gem-mirror.example.org"
@@ -338,7 +358,12 @@ E
   end
 
   describe "very long lines" do
-    before(:each) { bundle :install }
+    before(:each) do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "rack", "1.0.0"
+      G
+    end
 
     let(:long_string) do
       "--with-xml2-include=/usr/pkg/include/libxml2 --with-xml2-lib=/usr/pkg/lib " \
diff --git a/spec/install/gemfile/git_spec.rb b/spec/install/gemfile/git_spec.rb
index 651deb3265..beb5335fe4 100644
--- a/spec/install/gemfile/git_spec.rb
+++ b/spec/install/gemfile/git_spec.rb
@@ -922,12 +922,11 @@ RSpec.describe "bundle install with git sources" do
     build_git "foo", :path => lib_path("nested")
     build_git "bar", :path => lib_path("nested")
 
-    gemfile <<-G
+    install_gemfile <<-G
       gem "foo", :git => "#{lib_path("nested")}"
       gem "bar", :git => "#{lib_path("nested")}"
     G
 
-    bundle "install"
     expect(File.read(bundled_app("Gemfile.lock")).scan("GIT").size).to eq(1)
   end
 
@@ -1010,13 +1009,11 @@ RSpec.describe "bundle install with git sources" do
       install_gemfile <<-G
         gem "foo", :git => "file://#{lib_path("foo-1.0")}", :ref => "#{revision}"
       G
-      bundle "install"
       expect(out).to_not match(/Revision.*does not exist/)
 
       install_gemfile <<-G
         gem "foo", :git => "file://#{lib_path("foo-1.0")}", :ref => "deadbeef"
       G
-      bundle "install"
       expect(out).to include("Revision deadbeef does not exist in the repository")
     end
   end
@@ -1423,7 +1420,6 @@ In Gemfile:
           end
         G
 
-        bundle :install
         expect(last_command.stdboth).to_not include("password1")
         expect(last_command.stdout).to include("Fetching https://user1@github.com/company/private-repo")
       end
@@ -1439,7 +1435,6 @@ In Gemfile:
           end
         G
 
-        bundle :install
         expect(last_command.stdboth).to_not include("oauth_token")
         expect(last_command.stdout).to include("Fetching https://x-oauth-basic@github.com/company/private-repo")
       end
diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb
index cb73bfdc4e..a88578aea7 100644
--- a/spec/support/helpers.rb
+++ b/spec/support/helpers.rb
@@ -227,9 +227,9 @@ module Spec
         yield stdin, stdout, wait_thr if block_given?
         stdin.close
 
-        command_execution.exitstatus = wait_thr && wait_thr.value.exitstatus
         command_execution.stdout = Thread.new { stdout.read }.value.strip
         command_execution.stderr = Thread.new { stderr.read }.value.strip
+        command_execution.exitstatus = wait_thr && wait_thr.value.exitstatus
       end
 
       (@command_executions ||= []) << command_execution