Merge branch '18765-stub_env_in_specs' into 'master'

Resolve "Add a 'custom cop' to RuboCop that bans ENV assignment"

Closes #18765

See merge request gitlab-org/gitlab-ce!14810

lib/backup/manager.rb

@@ -101,8 +101,7 @@ module Backup
     end
 
     def unpack
-      Dir.chdir(backup_path)
-
+      Dir.chdir(backup_path) do
         # check for existing backups in the backup dir
         if backup_file_list.empty?
           $progress.puts "No backups found in #{backup_path}"
@@ -147,6 +146,7 @@ module Backup
           exit 1
         end
       end
+    end
 
     def tar_version
       tar_version, _ = Gitlab::Popen.popen(%w(tar --version))

rubocop/cop/rspec/env_assignment.rb

+require 'rubocop-rspec'
+require_relative '../../spec_helpers'
+
+module RuboCop
+  module Cop
+    module RSpec
+      # This cop checks for ENV assignment in specs
+      #
+      # @example
+      #
+      #   # bad
+      #   before do
+      #     ENV['FOO'] = 'bar'
+      #   end
+      #
+      #   # good
+      #   before do
+      #     stub_env('FOO', 'bar')
+      #   end
+      class EnvAssignment < Cop
+        include SpecHelpers
+
+        MESSAGE = "Don't assign to ENV, use `stub_env` instead.".freeze
+
+        def_node_search :env_assignment?, <<~PATTERN
+          (send (const nil? :ENV) :[]= ...)
+        PATTERN
+
+        # Following is what node.children looks like on a match
+        # [s(:const, nil, :ENV), :[]=, s(:str, "key"), s(:str, "value")]
+        def on_send(node)
+          return unless in_spec?(node)
+          return unless env_assignment?(node)
+
+          add_offense(node, :expression, MESSAGE)
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node.loc.expression, stub_env(env_key(node), env_value(node)))
+          end
+        end
+
+        def env_key(node)
+          node.children[2].source
+        end
+
+        def env_value(node)
+          node.children[3].source
+        end
+
+        def stub_env(key, value)
+          "stub_env(#{key}, #{value})"
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

+require_relative 'cop/active_record_dependent'
+require_relative 'cop/active_record_serialize'
 require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
-require_relative 'cop/active_record_serialize'
-require_relative 'cop/redirect_with_status'
+require_relative 'cop/in_batches'
 require_relative 'cop/polymorphic_associations'
 require_relative 'cop/project_path_helper'
-require_relative 'cop/active_record_dependent'
-require_relative 'cop/in_batches'
+require_relative 'cop/redirect_with_status'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default_to_large_table'
 require_relative 'cop/migration/add_concurrent_foreign_key'
@@ -13,12 +13,13 @@ require_relative 'cop/migration/add_concurrent_index'
 require_relative 'cop/migration/add_index'
 require_relative 'cop/migration/add_timestamps'
 require_relative 'cop/migration/datetime'
-require_relative 'cop/migration/safer_boolean_column'
 require_relative 'cop/migration/hash_index'
 require_relative 'cop/migration/remove_concurrent_index'
 require_relative 'cop/migration/remove_index'
 require_relative 'cop/migration/reversible_add_column_with_default'
+require_relative 'cop/migration/safer_boolean_column'
 require_relative 'cop/migration/timestamps'
 require_relative 'cop/migration/update_column_in_batches'
+require_relative 'cop/rspec/env_assignment'
 require_relative 'cop/rspec/single_line_hook'
 require_relative 'cop/rspec/verbose_include_metadata'

rubocop/spec_helpers.rb

+module RuboCop
+  module SpecHelpers
+    SPEC_HELPERS = %w[spec_helper.rb rails_helper.rb].freeze
+
+    # Returns true if the given node originated from the spec directory.
+    def in_spec?(node)
+      path = node.location.expression.source_buffer.name
+
+      !SPEC_HELPERS.include?(File.basename(path)) && path.start_with?(File.join(Dir.pwd, 'spec'))
+    end
+  end
+end

spec/lib/gitlab/backup/manager_spec.rb

@@ -172,10 +172,6 @@ describe Backup::Manager do
   end
 
   describe '#unpack' do
-    before do
-      allow(Dir).to receive(:chdir)
-    end
-
     context 'when there are no backup files in the directory' do
       before do
         allow(Dir).to receive(:glob).and_return([])

spec/rubocop/cop/rspec/env_assignment_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/rspec/env_assignment'
+
+describe RuboCop::Cop::RSpec::EnvAssignment do
+  include CopHelper
+
+  OFFENSE_CALL_SINGLE_QUOTES_KEY = %(ENV['FOO'] = 'bar').freeze
+  OFFENSE_CALL_DOUBLE_QUOTES_KEY = %(ENV["FOO"] = 'bar').freeze
+
+  let(:source_file) { 'spec/foo_spec.rb' }
+
+  subject(:cop) { described_class.new }
+
+  shared_examples 'an offensive ENV#[]= call' do |content|
+    it "registers an offense for `#{content}`" do
+      inspect_source(cop, content, source_file)
+
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.map(&:line)).to eq([1])
+      expect(cop.highlights).to eq([content])
+    end
+  end
+
+  shared_examples 'an autocorrected ENV#[]= call' do |content, autocorrected_content|
+    it "registers an offense for `#{content}` and autocorrects it to `#{autocorrected_content}`" do
+      autocorrected = autocorrect_source(cop, content, source_file)
+
+      expect(autocorrected).to eql(autocorrected_content)
+    end
+  end
+
+  context 'in a spec file' do
+    before do
+      allow(cop).to receive(:in_spec?).and_return(true)
+    end
+
+    context 'with a key using single quotes' do
+      it_behaves_like 'an offensive ENV#[]= call', OFFENSE_CALL_SINGLE_QUOTES_KEY
+      it_behaves_like 'an autocorrected ENV#[]= call', OFFENSE_CALL_SINGLE_QUOTES_KEY, %(stub_env('FOO', 'bar'))
+    end
+
+    context 'with a key using double quotes' do
+      it_behaves_like 'an offensive ENV#[]= call', OFFENSE_CALL_DOUBLE_QUOTES_KEY
+      it_behaves_like 'an autocorrected ENV#[]= call', OFFENSE_CALL_DOUBLE_QUOTES_KEY, %(stub_env("FOO", 'bar'))
+    end
+  end
+
+  context 'outside of a spec file' do
+    it "does not register an offense for `#{OFFENSE_CALL_SINGLE_QUOTES_KEY}` in a non-spec file" do
+      inspect_source(cop, OFFENSE_CALL_SINGLE_QUOTES_KEY)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end

spec/tasks/gitlab/backup_rake_spec.rb

@@ -4,7 +4,15 @@ require 'rake'
 describe 'gitlab:app namespace rake task' do
   let(:enable_registry) { true }
 
-  before :all do
+  def tars_glob
+    Dir.glob(File.join(Gitlab.config.backup.path, '*_gitlab_backup.tar'))
+  end
+
+  def backup_tar
+    tars_glob.first
+  end
+
+  before(:all) do
     Rake.application.rake_require 'tasks/gitlab/helpers'
     Rake.application.rake_require 'tasks/gitlab/backup'
     Rake.application.rake_require 'tasks/gitlab/shell'
@@ -19,9 +27,16 @@ describe 'gitlab:app namespace rake task' do
   end
 
   before do
+    stub_env('force', 'yes')
+    FileUtils.rm(tars_glob, force: true)
+    reenable_backup_sub_tasks
     stub_container_registry_config(enabled: enable_registry)
   end
 
+  after do
+    FileUtils.rm(tars_glob, force: true)
+  end
+
   def run_rake_task(task_name)
     Rake::Task[task_name].reenable
     Rake.application.invoke_task task_name
@@ -34,22 +49,15 @@ describe 'gitlab:app namespace rake task' do
   end
 
   describe 'backup_restore' do
-    before do
-      # avoid writing task output to spec progress
-      allow($stdout).to receive :write
-    end
-
     context 'gitlab version' do
       before do
         allow(Dir).to receive(:glob).and_return(['1_gitlab_backup.tar'])
-        allow(Dir).to receive(:chdir)
         allow(File).to receive(:exist?).and_return(true)
         allow(Kernel).to receive(:system).and_return(true)
         allow(FileUtils).to receive(:cp_r).and_return(true)
         allow(FileUtils).to receive(:mv).and_return(true)
         allow(Rake::Task["gitlab:shell:setup"])
           .to receive(:invoke).and_return(true)
-        ENV['force'] = 'yes'
       end
 
       let(:gitlab_version) { Gitlab::VERSION }
@@ -58,8 +66,9 @@ describe 'gitlab:app namespace rake task' do
         allow(YAML).to receive(:load_file)
           .and_return({ gitlab_version: "not #{gitlab_version}" })
 
-        expect { run_rake_task('gitlab:backup:restore') }
-          .to raise_error(SystemExit)
+        expect do
+          expect { run_rake_task('gitlab:backup:restore') }.to output.to_stdout
+        end.to raise_error(SystemExit)
       end
 
       it 'invokes restoration on match' do
@@ -75,44 +84,15 @@ describe 'gitlab:app namespace rake task' do
         expect(Rake::Task['gitlab:backup:lfs:restore']).to receive(:invoke)
         expect(Rake::Task['gitlab:backup:registry:restore']).to receive(:invoke)
         expect(Rake::Task['gitlab:shell:setup']).to receive(:invoke)
-        expect { run_rake_task('gitlab:backup:restore') }.not_to raise_error
+        expect { run_rake_task('gitlab:backup:restore') }.to output.to_stdout
       end
     end
   end # backup_restore task
 
   describe 'backup' do
-    before(:all) do
-      ENV['force'] = 'yes'
-    end
-
-    def tars_glob
-      Dir.glob(File.join(Gitlab.config.backup.path, '*_gitlab_backup.tar'))
-    end
-
-    def create_backup
-      FileUtils.rm tars_glob
-
+    before do
       # This reconnect makes our project fixture disappear, breaking the restore. Stub it out.
       allow(ActiveRecord::Base.connection).to receive(:reconnect!)
-
-      # Redirect STDOUT and run the rake task
-      orig_stdout = $stdout
-      $stdout = StringIO.new
-      reenable_backup_sub_tasks
-      run_rake_task('gitlab:backup:create')
-      reenable_backup_sub_tasks
-      $stdout = orig_stdout
-
-      @backup_tar = tars_glob.first
-    end
-
-    def restore_backup
-      orig_stdout = $stdout
-      $stdout = StringIO.new
-      reenable_backup_sub_tasks
-      run_rake_task('gitlab:backup:restore')
-      reenable_backup_sub_tasks
-      $stdout = orig_stdout
     end
 
     describe 'backup creation and deletion using custom_hooks' do
@@ -120,27 +100,17 @@ describe 'gitlab:app namespace rake task' do
       let(:user_backup_path) { "repositories/#{project.disk_path}" }
 
       before do
-        @origin_cd = Dir.pwd
-
-        path = File.join(project.repository.path_to_repo, filename)
+        stub_env('SKIP', 'db')
+        path = File.join(project.repository.path_to_repo, 'custom_hooks')
         FileUtils.mkdir_p(path)
         FileUtils.touch(File.join(path, "dummy.txt"))
-
-        ENV["SKIP"] = "db"
-        create_backup
-      end
-
-      after do
-        ENV["SKIP"] = ""
-        FileUtils.rm(@backup_tar)
-        Dir.chdir(@origin_cd)
       end
 
       context 'project uses custom_hooks and successfully creates backup' do
-        let(:filename) { "custom_hooks" }
-
         it 'creates custom_hooks.tar and project bundle' do
-          tar_contents, exit_status = Gitlab::Popen.popen(%W{tar -tvf #{@backup_tar}})
+          expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
+          tar_contents, exit_status = Gitlab::Popen.popen(%W{tar -tvf #{backup_tar}})
 
           expect(exit_status).to eq(0)
           expect(tar_contents).to match(user_backup_path)
@@ -149,47 +119,43 @@ describe 'gitlab:app namespace rake task' do
         end
 
         it 'restores files correctly' do
-          restore_backup
+          expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+          expect { run_rake_task('gitlab:backup:restore') }.to output.to_stdout
 
-          expect(Dir.entries(File.join(project.repository.path, "custom_hooks"))).to include("dummy.txt")
+          expect(Dir.entries(File.join(project.repository.path, 'custom_hooks'))).to include("dummy.txt")
         end
       end
     end
 
     context 'tar creation' do
-      before do
-        create_backup
-      end
-
-      after do
-        FileUtils.rm(@backup_tar)
-      end
-
       context 'archive file permissions' do
         it 'sets correct permissions on the tar file' do
-          expect(File.exist?(@backup_tar)).to be_truthy
-          expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100600')
+          expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
+          expect(File.exist?(backup_tar)).to be_truthy
+          expect(File::Stat.new(backup_tar).mode.to_s(8)).to eq('100600')
         end
 
         context 'with custom archive_permissions' do
           before do
             allow(Gitlab.config.backup).to receive(:archive_permissions).and_return(0651)
-            # We created a backup in a before(:all) so it got the default permissions.
-            # We now need to do some work to create a _new_ backup file using our stub.
-            FileUtils.rm(@backup_tar)
-            create_backup
           end
 
           it 'uses the custom permissions' do
-            expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100651')
+            expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
+            expect(File::Stat.new(backup_tar).mode.to_s(8)).to eq('100651')
           end
         end
       end
 
       it 'sets correct permissions on the tar contents' do
+        expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
         tar_contents, exit_status = Gitlab::Popen.popen(
-          %W{tar -tvf #{@backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz registry.tar.gz}
+          %W{tar -tvf #{backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz registry.tar.gz}
         )
+
         expect(exit_status).to eq(0)
         expect(tar_contents).to match('db/')
         expect(tar_contents).to match('uploads.tar.gz')
@@ -203,6 +169,8 @@ describe 'gitlab:app namespace rake task' do
       end
 
       it 'deletes temp directories' do
+        expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
         temp_dirs = Dir.glob(
           File.join(Gitlab.config.backup.path, '{db,repositories,uploads,builds,artifacts,pages,lfs,registry}')
         )
@@ -214,9 +182,12 @@ describe 'gitlab:app namespace rake task' do
         let(:enable_registry) { false }
 
         it 'does not create registry.tar.gz' do
+          expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
           tar_contents, exit_status = Gitlab::Popen.popen(
-            %W{tar -tvf #{@backup_tar}}
+            %W{tar -tvf #{backup_tar}}
           )
+
           expect(exit_status).to eq(0)
           expect(tar_contents).not_to match('registry.tar.gz')
         end
@@ -232,37 +203,33 @@ describe 'gitlab:app namespace rake task' do
         }
       end
 
-      let(:project_a) { create(:project, :repository, repository_storage: 'default') }
-      let(:project_b) { create(:project, :repository, repository_storage: 'test_second_storage') }
-
       before do
-        FileUtils.mkdir('tmp/tests/default_storage')
-        FileUtils.mkdir('tmp/tests/custom_storage')
+        # We only need a backup of the repositories for this test
+        stub_env('SKIP', 'db,uploads,builds,artifacts,lfs,registry')
+        FileUtils.mkdir(Settings.absolute('tmp/tests/default_storage'))
+        FileUtils.mkdir(Settings.absolute('tmp/tests/custom_storage'))
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
 
-        # Create the projects now, after mocking the settings but before doing the backup
-        project_a
-        project_b
-
         # Avoid asking gitaly about the root ref (which will fail beacuse of the
         # mocked storages)
         allow_any_instance_of(Repository).to receive(:empty_repo?).and_return(false)
-
-        # We only need a backup of the repositories for this test
-        ENV["SKIP"] = "db,uploads,builds,artifacts,lfs,registry"
-        create_backup
       end
 
       after do
-        FileUtils.rm_rf('tmp/tests/default_storage')
-        FileUtils.rm_rf('tmp/tests/custom_storage')
-        FileUtils.rm(@backup_tar) if @backup_tar
+        FileUtils.rm_rf(Settings.absolute('tmp/tests/default_storage'))
+        FileUtils.rm_rf(Settings.absolute('tmp/tests/custom_storage'))
       end
 
       it 'includes repositories in all repository storages' do
+        project_a = create(:project, :repository, repository_storage: 'default')
+        project_b = create(:project, :repository, repository_storage: 'test_second_storage')
+
+        expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
         tar_contents, exit_status = Gitlab::Popen.popen(
-          %W{tar -tvf #{@backup_tar} repositories}
+          %W{tar -tvf #{backup_tar} repositories}
         )
+
         expect(exit_status).to eq(0)
         expect(tar_contents).to match("repositories/#{project_a.disk_path}.bundle")
         expect(tar_contents).to match("repositories/#{project_b.disk_path}.bundle")
@@ -271,35 +238,15 @@ describe 'gitlab:app namespace rake task' do
   end # backup_create task
 
   describe "Skipping items" do
-    def tars_glob
-      Dir.glob(File.join(Gitlab.config.backup.path, '*_gitlab_backup.tar'))
-    end
-
-    before :all do
-      @origin_cd = Dir.pwd
-
-      reenable_backup_sub_tasks
-
-      FileUtils.rm tars_glob
-
-      # Redirect STDOUT and run the rake task
-      orig_stdout = $stdout
-      $stdout = StringIO.new
-      ENV["SKIP"] = "repositories,uploads"
-      run_rake_task('gitlab:backup:create')
-      $stdout = orig_stdout
-
-      @backup_tar = tars_glob.first
-    end
-
-    after :all do
-      FileUtils.rm(@backup_tar)
-      Dir.chdir @origin_cd
+    before do
+      stub_env('SKIP', 'repositories,uploads')
     end
 
     it "does not contain skipped item" do
+      expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
       tar_contents, _exit_status = Gitlab::Popen.popen(
-        %W{tar -tvf #{@backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz registry.tar.gz}
+        %W{tar -tvf #{backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz registry.tar.gz}
       )
 
       expect(tar_contents).to match('db/')
@@ -313,9 +260,10 @@ describe 'gitlab:app namespace rake task' do
     end
 
     it 'does not invoke repositories restore' do
+      expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
       allow(Rake::Task['gitlab:shell:setup'])
         .to receive(:invoke).and_return(true)
-      allow($stdout).to receive :write
 
       expect(Rake::Task['gitlab:db:drop_tables']).to receive :invoke
       expect(Rake::Task['gitlab:backup:db:restore']).to receive :invoke
@@ -327,38 +275,15 @@ describe 'gitlab:app namespace rake task' do
       expect(Rake::Task['gitlab:backup:lfs:restore']).to receive :invoke
       expect(Rake::Task['gitlab:backup:registry:restore']).to receive :invoke
       expect(Rake::Task['gitlab:shell:setup']).to receive :invoke
-      expect { run_rake_task('gitlab:backup:restore') }.not_to raise_error
+      expect { run_rake_task('gitlab:backup:restore') }.to output.to_stdout
     end
   end
 
   describe "Human Readable Backup Name" do
-    def tars_glob
-      Dir.glob(File.join(Gitlab.config.backup.path, '*_gitlab_backup.tar'))
-    end
-
-    before :all do
-      @origin_cd = Dir.pwd
-
-      reenable_backup_sub_tasks
-
-      FileUtils.rm tars_glob
-
-      # Redirect STDOUT and run the rake task
-      orig_stdout = $stdout
-      $stdout = StringIO.new
-      run_rake_task('gitlab:backup:create')
-      $stdout = orig_stdout
-
-      @backup_tar = tars_glob.first
-    end
-
-    after :all do
-      FileUtils.rm(@backup_tar)
-      Dir.chdir @origin_cd
-    end
-
     it 'name has human readable time' do
-      expect(@backup_tar).to match(/\d+_\d{4}_\d{2}_\d{2}_\d+\.\d+\.\d+.*_gitlab_backup.tar$/)
+      expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout
+
+      expect(backup_tar).to match(/\d+_\d{4}_\d{2}_\d{2}_\d+\.\d+\.\d+.*_gitlab_backup.tar$/)
     end
   end
 end # gitlab:app namespace

spec/tasks/gitlab/gitaly_rake_spec.rb

@@ -43,15 +43,8 @@ describe 'gitlab:gitaly namespace rake task' do
     describe 'gmake/make' do
       let(:command_preamble) { %w[/usr/bin/env -u RUBYOPT -u BUNDLE_GEMFILE] }
 
-      before(:all) do
-        @old_env_ci = ENV.delete('CI')
-      end
-
-      after(:all) do
-        ENV['CI'] = @old_env_ci if @old_env_ci
-      end
-
       before do
+        stub_env('CI', false)
         FileUtils.mkdir_p(clone_path)
         expect(Dir).to receive(:chdir).with(clone_path).and_call_original
         allow(Bundler).to receive(:bundle_path).and_return('/fake/bundle_path')

spec/tasks/gitlab/ldap_rake_spec.rb

@@ -4,7 +4,7 @@ describe 'gitlab:ldap:rename_provider rake task' do
   it 'completes without error' do
     Rake.application.rake_require 'tasks/gitlab/ldap'
     stub_warn_user_is_not_gitlab
-    ENV['force'] = 'yes'
+    stub_env('force', 'yes')
 
     create(:identity) # Necessary to prevent `exit 1` from the task.