Merge #7179

7179: Random test order r=deivid-rodriguez a=deivid-rodriguez

This is a follow-up to the work started by @colby-swandale in #7103.

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

The problem was that our specs do not run in random order, so the tests might be depending on state set by other tests to pass.

### What was your diagnosis of the problem?

My diagnosis was that we should enable random test ordering.

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

My fix is to randomize the specs and fix the resulting failures.

Co-authored-by: Colby Swandale <me@colby.fyi>
Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

7 files changed, 81 insertions, 60 deletions

diff --git a/.rspec b/.rspec
index 146d615488..a4aa3ddbf0 100644
--- a/.rspec
+++ b/.rspec
@@ -2,3 +2,4 @@
 --color
 --warnings
 --require spec_helper
+--order random
diff --git a/Rakefile b/Rakefile
index fbbd3365b6..c9835b1333 100644
--- a/Rakefile
+++ b/Rakefile
@@ -97,17 +97,12 @@ namespace :spec do
   end
 
   desc "Run the spec suite with the sudo tests"
-  task :sudo => %w[set_sudo spec clean_sudo]
+  task :sudo => %w[set_sudo spec]
 
   task :set_sudo do
     ENV["BUNDLER_SUDO_TESTS"] = "1"
   end
 
-  task :clean_sudo do
-    puts "Cleaning up sudo test files..."
-    system "sudo rm -rf #{File.expand_path("../tmp/sudo_gem_home", __FILE__)}"
-  end
-
   # RubyGems specs by version
   namespace :rubygems do
     # When editing this list, also edit .travis.yml!
@@ -121,7 +116,7 @@ namespace :spec do
 
       # Create tasks like spec:rubygems:v1.8.3:sudo to run the sudo specs
       namespace rg do
-        task :sudo => ["set_sudo", rg, "clean_sudo"]
+        task :sudo => ["set_sudo", rg]
         task :realworld => ["set_realworld", rg]
       end
 
@@ -139,7 +134,7 @@ namespace :spec do
     end
 
     namespace "co" do
-      task :sudo => ["set_sudo", "co", "clean_sudo"]
+      task :sudo => ["set_sudo", "co"]
       task :realworld => ["set_realworld", "co"]
     end
 
diff --git a/spec/bundler/bundler_spec.rb b/spec/bundler/bundler_spec.rb
index 9b772bffef..f74c1780bd 100644
--- a/spec/bundler/bundler_spec.rb
+++ b/spec/bundler/bundler_spec.rb
@@ -370,54 +370,51 @@ EOF
         it { should be true }
       end
     end
-  end
 
-  describe "#requires_sudo?" do
-    before do
-      allow(Bundler).to receive(:which).with("sudo").and_return("/usr/bin/sudo")
-      FileUtils.mkdir_p("tmp/vendor/bundle")
-      FileUtils.mkdir_p("tmp/vendor/bin_dir")
-    end
-    after do
-      FileUtils.rm_rf("tmp/vendor/bundle")
-      FileUtils.rm_rf("tmp/vendor/bin_dir")
-      Bundler.remove_instance_variable(:@requires_sudo_ran)
-      Bundler.remove_instance_variable(:@requires_sudo)
-    end
-    context "writable paths" do
-      it "should return false and display nothing" do
-        allow(Bundler).to receive(:bundle_path).and_return(Pathname("tmp/vendor/bundle"))
-        expect(Bundler.ui).to_not receive(:warn)
-        expect(Bundler.requires_sudo?).to eq(false)
-      end
-    end
-    context "unwritable paths" do
+    context "path writability" do
       before do
-        FileUtils.touch("tmp/vendor/bundle/unwritable1.txt")
-        FileUtils.touch("tmp/vendor/bundle/unwritable2.txt")
-        FileUtils.touch("tmp/vendor/bin_dir/unwritable3.txt")
-        FileUtils.chmod(0o400, "tmp/vendor/bundle/unwritable1.txt")
-        FileUtils.chmod(0o400, "tmp/vendor/bundle/unwritable2.txt")
-        FileUtils.chmod(0o400, "tmp/vendor/bin_dir/unwritable3.txt")
+        FileUtils.mkdir_p("tmp/vendor/bundle")
+        FileUtils.mkdir_p("tmp/vendor/bin_dir")
+      end
+      after do
+        FileUtils.rm_rf("tmp/vendor/bundle")
+        FileUtils.rm_rf("tmp/vendor/bin_dir")
       end
-      it "should return true and display warn message" do
-        allow(Bundler).to receive(:bundle_path).and_return(Pathname("tmp/vendor/bundle"))
-        bin_dir = Pathname("tmp/vendor/bin_dir/")
+      context "writable paths" do
+        it "should return false and display nothing" do
+          allow(Bundler).to receive(:bundle_path).and_return(Pathname("tmp/vendor/bundle"))
+          expect(Bundler.ui).to_not receive(:warn)
+          expect(Bundler.requires_sudo?).to eq(false)
+        end
+      end
+      context "unwritable paths" do
+        before do
+          FileUtils.touch("tmp/vendor/bundle/unwritable1.txt")
+          FileUtils.touch("tmp/vendor/bundle/unwritable2.txt")
+          FileUtils.touch("tmp/vendor/bin_dir/unwritable3.txt")
+          FileUtils.chmod(0o400, "tmp/vendor/bundle/unwritable1.txt")
+          FileUtils.chmod(0o400, "tmp/vendor/bundle/unwritable2.txt")
+          FileUtils.chmod(0o400, "tmp/vendor/bin_dir/unwritable3.txt")
+        end
+        it "should return true and display warn message" do
+          allow(Bundler).to receive(:bundle_path).and_return(Pathname("tmp/vendor/bundle"))
+          bin_dir = Pathname("tmp/vendor/bin_dir/")
 
-        # allow File#writable? to be called with args other than the stubbed on below
-        allow(File).to receive(:writable?).and_call_original
+          # allow File#writable? to be called with args other than the stubbed on below
+          allow(File).to receive(:writable?).and_call_original
 
-        # fake make the directory unwritable
-        allow(File).to receive(:writable?).with(bin_dir).and_return(false)
-        allow(Bundler).to receive(:system_bindir).and_return(Pathname("tmp/vendor/bin_dir/"))
-        message = <<-MESSAGE.chomp
+          # fake make the directory unwritable
+          allow(File).to receive(:writable?).with(bin_dir).and_return(false)
+          allow(Bundler).to receive(:system_bindir).and_return(Pathname("tmp/vendor/bin_dir/"))
+          message = <<-MESSAGE.chomp
 Following files may not be writable, so sudo is needed:
   tmp/vendor/bin_dir/
   tmp/vendor/bundle/unwritable1.txt
   tmp/vendor/bundle/unwritable2.txt
 MESSAGE
-        expect(Bundler.ui).to receive(:warn).with(message)
-        expect(Bundler.requires_sudo?).to eq(true)
+          expect(Bundler.ui).to receive(:warn).with(message)
+          expect(Bundler.requires_sudo?).to eq(true)
+        end
       end
     end
   end
diff --git a/spec/bundler/plugin/events_spec.rb b/spec/bundler/plugin/events_spec.rb
index b09e915682..28d70c6fdd 100644
--- a/spec/bundler/plugin/events_spec.rb
+++ b/spec/bundler/plugin/events_spec.rb
@@ -2,10 +2,14 @@
 
 RSpec.describe Bundler::Plugin::Events do
   context "plugin events" do
+    before { Bundler::Plugin::Events.send :reset }
+
     describe "#define" do
       it "raises when redefining a constant" do
+        Bundler::Plugin::Events.send(:define, :TEST_EVENT, "foo")
+
         expect do
-          Bundler::Plugin::Events.send(:define, :GEM_BEFORE_INSTALL_ALL, "another-value")
+          Bundler::Plugin::Events.send(:define, :TEST_EVENT, "bar")
         end.to raise_error(ArgumentError)
       end
 
diff --git a/spec/bundler/ruby_version_spec.rb b/spec/bundler/ruby_version_spec.rb
index 46a1b2918b..3ac7d9ef3a 100644
--- a/spec/bundler/ruby_version_spec.rb
+++ b/spec/bundler/ruby_version_spec.rb
@@ -399,8 +399,14 @@ RSpec.describe "Bundler::RubyVersion and its subclasses" do
 
       let(:bundler_system_ruby_version) { subject }
 
-      before do
-        Bundler::RubyVersion.instance_variable_set("@ruby_version", nil)
+      around do |example|
+        begin
+          old_ruby_version = Bundler::RubyVersion.instance_variable_get("@ruby_version")
+          Bundler::RubyVersion.instance_variable_set("@ruby_version", nil)
+          example.run
+        ensure
+          Bundler::RubyVersion.instance_variable_set("@ruby_version", old_ruby_version)
+        end
       end
 
       it "should return an instance of Bundler::RubyVersion" do
diff --git a/spec/install/gems/sudo_spec.rb b/spec/install/gems/sudo_spec.rb
index a7372b1fbe..0d2d6b7eb4 100644
--- a/spec/install/gems/sudo_spec.rb
+++ b/spec/install/gems/sudo_spec.rb
@@ -3,13 +3,20 @@
 RSpec.describe "when using sudo", :sudo => true do
   describe "and BUNDLE_PATH is writable" do
     context "but BUNDLE_PATH/build_info is not writable" do
+      let(:subdir) do
+        system_gem_path("cache")
+      end
+
       before do
         bundle! "config set path.system true"
-        subdir = system_gem_path("cache")
         subdir.mkpath
         sudo "chmod u-w #{subdir}"
       end
 
+      after do
+        sudo "chmod u+w #{subdir}"
+      end
+
       it "installs" do
         install_gemfile <<-G
           source "file://#{gem_repo1}"
@@ -101,6 +108,10 @@ RSpec.describe "when using sudo", :sudo => true do
       sudo "chmod ugo-w #{default_bundle_path}"
     end
 
+    after do
+      sudo "chmod ugo+w #{default_bundle_path}"
+    end
+
     it "installs" do
       install_gemfile <<-G
         source "file://#{gem_repo1}"
@@ -142,6 +153,8 @@ RSpec.describe "when using sudo", :sudo => true do
       bundle :install, :env => { "GEM_HOME" => gem_home.to_s, "GEM_PATH" => nil }
       expect(gem_home.join("bin/rackup")).to exist
       expect(the_bundle).to include_gems "rack 1.0", :env => { "GEM_HOME" => gem_home.to_s, "GEM_PATH" => nil }
+
+      sudo "rm -rf #{tmp("sudo_gem_home")}"
     end
   end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 067c8dc070..d3de1ff784 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -23,12 +23,6 @@ end
 $debug = false
 
 Spec::Manpages.setup unless Gem.win_platform?
-Spec::Rubygems.setup
-ENV["RUBYOPT"] = "#{ENV["RUBYOPT"]} -r#{Spec::Path.spec_dir}/support/hax.rb"
-ENV["BUNDLE_SPEC_RUN"] = "true"
-
-# Don't wrap output in tests
-ENV["THOR_COLUMNS"] = "10000"
 
 module Gem
   def self.ruby=(ruby)
@@ -58,6 +52,8 @@ RSpec.configure do |config|
   # forever due to memory constraints
   config.fail_fast ||= 25 if ENV["CI"]
 
+  config.bisect_runner = :shell
+
   if ENV["BUNDLER_SUDO_TESTS"] && Spec::Sudo.present?
     config.filter_run :sudo => true
   else
@@ -102,6 +98,15 @@ RSpec.configure do |config|
   end
 
   config.before :suite do
+    Spec::Rubygems.setup
+    ENV["RUBYOPT"] = original_env["RUBYOPT"] = "#{ENV["RUBYOPT"]} -r#{Spec::Path.spec_dir}/support/hax.rb"
+    ENV["BUNDLE_SPEC_RUN"] = original_env["BUNDLE_SPEC_RUN"] = "true"
+
+    # Don't wrap output in tests
+    ENV["THOR_COLUMNS"] = "10000"
+
+    original_env = ENV.to_hash.delete_if {|k, _v| k.start_with?(Bundler::EnvironmentPreserver::BUNDLER_PREFIX) }
+
     if ENV["BUNDLE_RUBY"]
       FileUtils.cp_r Spec::Path.bindir, File.join(Spec::Path.root, "lib", "exe")
     end
@@ -111,14 +116,15 @@ RSpec.configure do |config|
     build_repo1
   end
 
-  config.before :each do
+  config.around :each do |example|
+    ENV.replace(original_env)
     reset!
     system_gems []
     in_app_root
     @command_executions = []
-  end
 
-  config.after :each do |example|
+    example.run
+
     all_output = @command_executions.map(&:to_s_verbose).join("\n\n")
     if example.exception && !all_output.empty?
       warn all_output unless config.formatters.grep(RSpec::Core::Formatters::DocumentationFormatter).empty?
@@ -129,7 +135,6 @@ RSpec.configure do |config|
     end
 
     Dir.chdir(original_wd)
-    ENV.replace(original_env)
   end
 
   config.after :suite do