Avoid use of ENV in rspec

6 files changed, 63 insertions, 39 deletions

diff --git a/ruby/lib/gitlab/config.rb b/ruby/lib/gitlab/config.rb
index 379d28f4c..6aa445aa8 100644
--- a/ruby/lib/gitlab/config.rb
+++ b/ruby/lib/gitlab/config.rb
@@ -1,13 +1,31 @@
 module Gitlab
-  # Config lets Gitlab::Git do mock config lookups.
+  #
+  # In production, gitaly-ruby configuration is derived from environment
+  # variables set by the Go gitaly parent process. During the rspec test
+  # suite this parent process is not there so we need to get configuration
+  # values from somewhere else. We used to work around this by setting
+  # variables in ENV during the rspec boot but that turned out to be
+  # fragile because Bundler.with_clean_env resets changes to ENV made
+  # after Bundler was loaded. Instead of changing ENV, the TestSetup
+  # module gives us a hacky way to set instance variables on the config
+  # objects, bypassing the ENV lookups.
+  #
+  module TestSetup
+    def test_global_ivar_override(name, value)
+      instance_variable_set("@#{name}".to_sym, value)
+    end
+  end
+
   class Config
     class Git
+      include TestSetup
+
       def bin_path
-        ENV['GITALY_RUBY_GIT_BIN_PATH']
+        @bin_path ||= ENV['GITALY_RUBY_GIT_BIN_PATH']
       end
 
       def hooks_directory
-        ENV['GITALY_GIT_HOOKS_DIR']
+        @hooks_directory ||= ENV['GITALY_GIT_HOOKS_DIR']
       end
 
       def write_buffer_size
@@ -20,35 +38,39 @@ module Gitlab
     end
 
     class GitlabShell
+      include TestSetup
+
       def path
-        ENV['GITALY_RUBY_GITLAB_SHELL_PATH']
+        @path ||= ENV['GITALY_RUBY_GITLAB_SHELL_PATH']
       end
 
       def git_timeout
-        10800 # TODO make this configurable or eliminate otherwise https://gitlab.com/gitlab-org/gitaly/issues/885
+        @git_timeout ||= 10800 # TODO make this configurable or eliminate otherwise https://gitlab.com/gitlab-org/gitaly/issues/885
       end
     end
 
     class Gitaly
+      include TestSetup
+
       def client_path
-        ENV['GITALY_RUBY_GITALY_BIN_DIR']
+        @client_path ||= ENV['GITALY_RUBY_GITALY_BIN_DIR']
       end
     end
 
     def git
-      Git.new
+      @git ||= Git.new
     end
 
     def gitlab_shell
-      GitlabShell.new
+      @gitlab_shell ||= GitlabShell.new
     end
 
     def gitaly
-      Gitaly.new
+      @gitaly ||= Gitaly.new
     end
   end
 
   def self.config
-    Config.new
+    @config ||= Config.new
   end
 end
diff --git a/ruby/spec/lib/gitlab/config_spec.rb b/ruby/spec/lib/gitlab/config_spec.rb
index 2a503d005..e419408a2 100644
--- a/ruby/spec/lib/gitlab/config_spec.rb
+++ b/ruby/spec/lib/gitlab/config_spec.rb
@@ -7,11 +7,7 @@ describe Gitlab::Config do
     let(:gitlab_shell_path) { '/foo/bar/gitlab-shell' }
 
     before do
-      ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] = gitlab_shell_path
-    end
-
-    after do
-      ENV.delete('GITALY_RUBY_GITLAB_SHELL_PATH')
+      allow(ENV).to receive(:[]).with('GITALY_RUBY_GITLAB_SHELL_PATH').and_return(gitlab_shell_path)
     end
 
     it { expect(subject.path).to eq(gitlab_shell_path) }
diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb
index ac2062975..2c2d45ab5 100644
--- a/ruby/spec/lib/gitlab/git/hook_spec.rb
+++ b/ruby/spec/lib/gitlab/git/hook_spec.rb
@@ -10,7 +10,6 @@ describe Gitlab::Git::Hook do
   end
 
   describe '#trigger' do
-    let!(:old_env) { ENV['GITALY_GIT_HOOKS_DIR'] }
     let(:tmp_dir) { Dir.mktmpdir }
     let(:hook_names) { %w[pre-receive post-receive update] }
     let(:repo) { gitlab_git_from_gitaly(test_repo_read_only) }
@@ -22,28 +21,44 @@ describe Gitlab::Git::Hook do
         FileUtils.chmod("u+x", path)
       end
 
-      ENV['GITALY_GIT_HOOKS_DIR'] = tmp_dir
+      allow(Gitlab.config.git).to receive(:hooks_directory).and_return(tmp_dir)
     end
 
     after do
       FileUtils.remove_entry(tmp_dir)
-      ENV['GITALY_GIT_HOOKS_DIR'] = old_env
     end
 
     context 'when the hooks require environment variables' do
-      let(:vars) { %w[GL_ID GL_USERNAME PWD GIT_DIR] }
+      let(:vars) do
+        {
+          'GL_ID' => 'user-123',
+          'GL_USERNAME' => 'janedoe',
+          'PWD' => repo.path,
+          'GIT_DIR' => repo.path,
+        }
+      end
+
       let(:script) do
-        "#!/bin/sh\n" +
-          vars.map { |var| "if [ -z \"$#{var}\"]; then exit 1; fi\n" }.join("\n") +
-          "exit 0\n"
+        [
+          "#!/bin/sh",
+          vars.map do |key, value|
+            <<-SCRIPT
+              if [ x$#{key} != x#{value} ]; then
+                echo "unexpected value: #{key}=$#{key}"
+                exit 1
+               fi
+            SCRIPT
+          end.join,
+          "exit 0"
+        ].join("\n")
       end
 
       it 'returns true' do
         hook_names.each do |hook|
           trigger_result = described_class.new(hook, repo)
-                                          .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master')
+                                          .trigger(vars['GL_ID'], vars['GL_USERNAME'], '0' * 40, 'a' * 40, 'master')
 
-          expect(trigger_result.first).to be(true)
+          expect(trigger_result.first).to be(true), "#{hook} failed:  #{trigger_result.last}"
         end
       end
     end
@@ -54,7 +69,7 @@ describe Gitlab::Git::Hook do
       it 'returns true' do
         hook_names.each do |hook|
           trigger_result = described_class.new(hook, repo)
-                                          .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master')
+                                          .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
 
           expect(trigger_result.first).to be(true)
         end
@@ -67,7 +82,7 @@ describe Gitlab::Git::Hook do
       it 'returns false' do
         hook_names.each do |hook|
           trigger_result = described_class.new(hook, repo)
-                                          .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master')
+                                          .trigger('user-1', 'admin', '0' * 40, 'a' * 40, 'master')
 
           expect(trigger_result.first).to be(false)
         end
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index e5cfb7d7b..82e69f935 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -23,15 +23,6 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
       }
     end
     let(:call) { double(metadata: call_metadata) }
-    let(:gitlab_shell_path) { '/foo/bar/gitlab-shell' }
-
-    before do
-      ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] = gitlab_shell_path
-    end
-
-    after do
-      ENV.delete('GITALY_RUBY_GITLAB_SHELL_PATH')
-    end
 
     it 'cleans up the repository' do
       described_class.from_gitaly_with_block(test_repo_read_only, call) do |repository|
diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb
index 183cac7ba..8b861254d 100644
--- a/ruby/spec/spec_helper.rb
+++ b/ruby/spec/spec_helper.rb
@@ -11,9 +11,9 @@ require File.join(__dir__, 'support/helpers/gitlab_shell_helper.rb')
 
 Dir[File.join(__dir__, 'support/helpers/*.rb')].each { |f| require f }
 
-ENV['GITALY_RUBY_GIT_BIN_PATH'] ||= 'git'
-ENV['GITALY_GIT_HOOKS_DIR'] ||= File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks")
-ENV['GITALY_RUBY_GITALY_BIN_DIR'] = __dir__
+Gitlab.config.git.test_global_ivar_override(:bin_path, 'git')
+Gitlab.config.git.test_global_ivar_override(:hooks_directory, File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks"))
+Gitlab.config.gitaly.test_global_ivar_override(:client_path, __dir__)
 
 RSpec.configure do |config|
   config.include FactoryBot::Syntax::Methods
diff --git a/ruby/spec/support/helpers/gitlab_shell_helper.rb b/ruby/spec/support/helpers/gitlab_shell_helper.rb
index ad127bf2e..b38b6ae62 100644
--- a/ruby/spec/support/helpers/gitlab_shell_helper.rb
+++ b/ruby/spec/support/helpers/gitlab_shell_helper.rb
@@ -7,7 +7,7 @@ GITLAB_SHELL_DIR = File.join(TMP_DIR, 'gitlab-shell').freeze
 
 module GitlabShellHelper
   def self.setup_gitlab_shell
-    ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] = GITLAB_SHELL_DIR
+    Gitlab.config.gitlab_shell.test_global_ivar_override(:path, GITLAB_SHELL_DIR)
 
     FileUtils.mkdir_p([TMP_DIR, File.join(GITLAB_SHELL_DIR, 'hooks')])
   end