Merge branch 'fix_file_read_stub' into 'master'

Stub file reads only for the files we are interested in

See merge request gitlab-org/gitlab!48014

doc/development/testing_guide/best_practices.md

@@ -603,6 +603,32 @@ it "really connects to Prometheus", :permit_dns do
 And if you need more specific control, the DNS blocking is implemented in
 `spec/support/helpers/dns_helpers.rb` and these methods can be called elsewhere.
 
+#### Stubbing File methods
+
+In the situations where you need to
+[stub](https://relishapp.com/rspec/rspec-mocks/v/3-9/docs/basics/allowing-messages)
+methods such as `File.read`, make sure to:
+
+1. Stub `File.read` for only the filepath you are interested in.
+1. Call the original implementation for other filepaths.
+
+Otherwise `File.read` calls from other parts of the codebase get
+stubbed incorrectly. You should use the `stub_file_read`, and
+`expect_file_read` helper methods which does the stubbing for
+`File.read` correctly.
+
+```ruby
+# bad, all Files will read and return nothing
+allow(File).to receive(:read)
+
+# good
+stub_file_read(my_filepath)
+
+# also OK
+allow(File).to receive(:read).and_call_original
+allow(File).to receive(:read).with(my_filepath)
+```
+
 #### Filesystem
 
 Filesystem data can be roughly split into "repositories", and "everything else".

ee/spec/lib/gitlab/sitemaps/sitemap_file_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Gitlab::Sitemaps::SitemapFile do
 
   describe '#render' do
     it 'returns if no elements has been provided' do
-      expect(File).not_to receive(:read)
+      expect_file_not_to_read(described_class::SITEMAP_FILE_PATH)
 
       described_class.new.save # rubocop: disable Rails/SaveBang
     end

ee/spec/tasks/gitlab/license_rake_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe 'gitlab:license namespace rake tasks' do
           let(:license_file_contents) { 'valid contents' }
 
           it 'succeeds in adding the license' do
-            expect(File).to receive(:read).with(license_path).and_return(license_file_contents)
+            expect_file_read(license_path, content: license_file_contents)
             expect(License).to receive(:create).with(data: license_file_contents).and_return(true)
 
             expect { subject }.not_to raise_error
@@ -47,7 +47,7 @@ RSpec.describe 'gitlab:license namespace rake tasks' do
           let(:license_file_contents) { 'invalid contents' }
 
           it 'fails to add the license' do
-            expect(File).to receive(:read).with(license_path).and_return(license_file_contents)
+            expect_file_read(license_path, content: license_file_contents)
             expect(License).to receive(:create).with(data: license_file_contents).and_return(false)
 
             expect { subject }.to raise_error(RuntimeError, "License Invalid")

spec/controllers/help_controller_spec.rb

@@ -101,7 +101,8 @@ RSpec.describe HelpController do
     context 'for Markdown formats' do
       context 'when requested file exists' do
         before do
-          expect(File).to receive(:read).and_return(fixture_file('blockquote_fence_after.md'))
+          expect_file_read(File.join(Rails.root, 'doc/ssh/README.md'), content: fixture_file('blockquote_fence_after.md'))
+
           get :show, params: { path: 'ssh/README' }, format: :md
         end
 
@@ -213,6 +214,6 @@ RSpec.describe HelpController do
   end
 
   def stub_readme(content)
-    expect(File).to receive(:read).and_return(content)
+    expect_file_read(Rails.root.join('doc', 'README.md'), content: content)
   end
 end

spec/helpers/icons_helper_spec.rb

@@ -86,7 +86,7 @@ RSpec.describe IconsHelper do
       it 'does not raise in production mode' do
         stub_rails_env('production')
 
-        expect(File).not_to receive(:read)
+        expect_file_not_to_read(Rails.root.join('node_modules/@gitlab/svgs/dist/icons.json'))
 
         expect { sprite_icon(non_existing) }.not_to raise_error
       end

spec/initializers/secret_token_spec.rb

@@ -11,10 +11,13 @@ RSpec.describe 'create_tokens' do
   let(:rsa_key) { /\A-----BEGIN RSA PRIVATE KEY-----\n.+\n-----END RSA PRIVATE KEY-----\n\Z/m.freeze }
 
   before do
-    allow(File).to receive(:write)
-    allow(File).to receive(:delete)
     allow(Rails).to receive_message_chain(:application, :secrets).and_return(secrets)
     allow(Rails).to receive_message_chain(:root, :join) { |string| string }
+
+    allow(File).to receive(:write).and_call_original
+    allow(File).to receive(:write).with(Rails.root.join('config/secrets.yml'))
+    allow(File).to receive(:delete).and_call_original
+    allow(File).to receive(:delete).with(Rails.root.join('.secret'))
     allow(self).to receive(:warn)
     allow(self).to receive(:exit)
   end
@@ -105,7 +108,7 @@ RSpec.describe 'create_tokens' do
         secrets.openid_connect_signing_key = 'openid_connect_signing_key'
 
         allow(File).to receive(:exist?).with('.secret').and_return(true)
-        allow(File).to receive(:read).with('.secret').and_return('file_key')
+        stub_file_read('.secret', content: 'file_key')
       end
 
       context 'when secret_key_base exists in the environment and secrets.yml' do

spec/lib/feature/definition_spec.rb

@@ -75,7 +75,7 @@ RSpec.describe Feature::Definition do
 
   describe '.load_from_file' do
     it 'properly loads a definition from file' do
-      expect(File).to receive(:read).with(path) { yaml_content }
+      expect_file_read(path, content: yaml_content)
 
       expect(described_class.send(:load_from_file, path).attributes)
         .to eq(definition.attributes)
@@ -93,7 +93,7 @@ RSpec.describe Feature::Definition do
 
     context 'for invalid definition' do
       it 'raises exception' do
-        expect(File).to receive(:read).with(path) { '{}' }
+        expect_file_read(path, content: '{}')
 
         expect do
           described_class.send(:load_from_file, path)

spec/lib/gitlab/email/smime/certificate_spec.rb

@@ -69,8 +69,8 @@ RSpec.describe Gitlab::Email::Smime::Certificate do
 
   describe '.from_files' do
     it 'parses correctly a certificate and key' do
-      allow(File).to receive(:read).with('a_key').and_return(@cert[:key].to_s)
-      allow(File).to receive(:read).with('a_cert').and_return(@cert[:cert].to_pem)
+      stub_file_read('a_key', content: @cert[:key].to_s)
+      stub_file_read('a_cert', content: @cert[:cert].to_pem)
 
       parsed_cert = described_class.from_files('a_key', 'a_cert')
 
@@ -79,9 +79,9 @@ RSpec.describe Gitlab::Email::Smime::Certificate do
 
     context 'with optional ca_certs' do
       it 'parses correctly certificate, key and ca_certs' do
-        allow(File).to receive(:read).with('a_key').and_return(@cert[:key].to_s)
-        allow(File).to receive(:read).with('a_cert').and_return(@cert[:cert].to_pem)
-        allow(File).to receive(:read).with('a_ca_cert').and_return(@intermediate_ca[:cert].to_pem)
+        stub_file_read('a_key', content: @cert[:key].to_s)
+        stub_file_read('a_cert', content: @cert[:cert].to_pem)
+        stub_file_read('a_ca_cert', content: @intermediate_ca[:cert].to_pem)
 
         parsed_cert = described_class.from_files('a_key', 'a_cert', 'a_ca_cert')
 
@@ -94,8 +94,8 @@ RSpec.describe Gitlab::Email::Smime::Certificate do
     it 'parses correctly a certificate and key' do
       cert = generate_cert(signer_ca: @root_ca)
 
-      allow(File).to receive(:read).with('a_key').and_return(cert[:key].to_s)
-      allow(File).to receive(:read).with('a_cert').and_return(cert[:cert].to_pem)
+      stub_file_read('a_key', content: cert[:key].to_s)
+      stub_file_read('a_cert', content: cert[:cert].to_pem)
 
       parsed_cert = described_class.from_files('a_key', 'a_cert')
 

spec/lib/gitlab/gitaly_client_spec.rb

@@ -53,7 +53,7 @@ RSpec.describe Gitlab::GitalyClient do
   describe '.filesystem_id_from_disk' do
     it 'catches errors' do
       [Errno::ENOENT, Errno::EACCES, JSON::ParserError].each do |error|
-        allow(File).to receive(:read).with(described_class.storage_metadata_file_path('default')).and_raise(error)
+        stub_file_read(described_class.storage_metadata_file_path('default'), error: error)
 
         expect(described_class.filesystem_id_from_disk('default')).to be_nil
       end

spec/lib/gitlab/webpack/manifest_spec.rb

@@ -97,7 +97,7 @@ RSpec.describe Gitlab::Webpack::Manifest do
   context "with dev server disabled" do
     before do
       allow(Gitlab.config.webpack.dev_server).to receive(:enabled).and_return(false)
-      allow(File).to receive(:read).with(::Rails.root.join("manifest_output/my_manifest.json")).and_return(manifest)
+      stub_file_read(::Rails.root.join("manifest_output/my_manifest.json"), content: manifest)
     end
 
     describe ".asset_paths" do
@@ -105,7 +105,7 @@ RSpec.describe Gitlab::Webpack::Manifest do
 
       it "errors if we can't find the manifest" do
         allow(Gitlab.config.webpack).to receive(:manifest_filename).and_return('broken.json')
-        allow(File).to receive(:read).with(::Rails.root.join("manifest_output/broken.json")).and_raise(Errno::ENOENT)
+        stub_file_read(::Rails.root.join("manifest_output/broken.json"), error: Errno::ENOENT)
         expect { Gitlab::Webpack::Manifest.asset_paths("entry1") }.to raise_error(Gitlab::Webpack::Manifest::ManifestLoadError)
       end
     end

spec/lib/gitlab_spec.rb

@@ -26,18 +26,17 @@ RSpec.describe Gitlab do
       end
 
       it 'returns the actual Git revision' do
-        expect(File).to receive(:read)
-                          .with(described_class.root.join('REVISION'))
-                          .and_return("abc123\n")
+        expect_file_read(described_class.root.join('REVISION'), content: "abc123\n")
 
         expect(described_class.revision).to eq('abc123')
       end
 
       it 'memoizes the revision' do
+        stub_file_read(described_class.root.join('REVISION'), content: "abc123\n")
+
         expect(File).to receive(:read)
-                          .once
-                          .with(described_class.root.join('REVISION'))
-                          .and_return("abc123\n")
+          .once
+          .with(described_class.root.join('REVISION'))
 
         2.times { described_class.revision }
       end

spec/models/instance_configuration_spec.rb

@@ -10,31 +10,35 @@ RSpec.describe InstanceConfiguration do
         let(:sha256) { 'SHA256:2KJDT7xf2i68mBgJ3TVsjISntg4droLbXYLfQj0VvSY' }
 
         it 'does not return anything if file does not exist' do
-          stub_pub_file(exist: false)
+          stub_pub_file(pub_file(exist: false))
 
           expect(subject.settings[:ssh_algorithms_hashes]).to be_empty
         end
 
         it 'does not return anything if file is empty' do
-          stub_pub_file
+          stub_pub_file(pub_file)
 
-          allow(File).to receive(:read).and_return('')
+          stub_file_read(pub_file, content: '')
 
           expect(subject.settings[:ssh_algorithms_hashes]).to be_empty
         end
 
         it 'returns the md5 and sha256 if file valid and exists' do
-          stub_pub_file
+          stub_pub_file(pub_file)
 
           result = subject.settings[:ssh_algorithms_hashes].select { |o| o[:md5] == md5 && o[:sha256] == sha256 }
 
           expect(result.size).to eq(InstanceConfiguration::SSH_ALGORITHMS.size)
         end
 
-        def stub_pub_file(exist: true)
+        def pub_file(exist: true)
           path = exist ? 'spec/fixtures/ssh_host_example_key.pub' : 'spec/fixtures/ssh_host_example_key.pub.random'
 
-          allow(subject).to receive(:ssh_algorithm_file).and_return(Rails.root.join(path))
+          Rails.root.join(path)
+        end
+
+        def stub_pub_file(path)
+          allow(subject).to receive(:ssh_algorithm_file).and_return(path)
         end
       end
 

spec/services/clusters/aws/fetch_credentials_service_spec.rb

@@ -60,9 +60,7 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do
         subject { described_class.new(provision_role, provider: provider).execute }
 
         before do
-          allow(File).to receive(:read)
-            .with(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json'))
-            .and_return(session_policy)
+          stub_file_read(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json'), content: session_policy)
         end
 
         it { is_expected.to eq assumed_role_credentials }

spec/services/clusters/aws/provision_service_spec.rb

@@ -42,9 +42,7 @@ RSpec.describe Clusters::Aws::ProvisionService do
       allow(provider).to receive(:api_client)
         .and_return(client)
 
-      allow(File).to receive(:read)
-        .with(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml'))
-        .and_return(cloudformation_template)
+      stub_file_read(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml'), content: cloudformation_template)
     end
 
     it 'updates the provider status to :creating and configures the provider with credentials' do

spec/services/metrics/dashboard/clone_dashboard_service_spec.rb

@@ -146,7 +146,7 @@ RSpec.describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memor
         it 'extends dashboard template path to absolute url' do
           allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success }))
 
-          expect(File).to receive(:read).with(Rails.root.join('config/prometheus/common_metrics.yml')).and_return('')
+          expect_file_read(Rails.root.join('config/prometheus/common_metrics.yml'), content: '')
 
           service_call
         end

spec/spec_helper.rb

@@ -134,6 +134,7 @@ RSpec.configure do |config|
   config.include NextFoundInstanceOf
   config.include NextInstanceOf
   config.include TestEnv
+  config.include FileReadHelpers
   config.include Devise::Test::ControllerHelpers, type: :controller
   config.include Devise::Test::IntegrationHelpers, type: :feature
   config.include LoginHelpers, type: :feature

spec/support/helpers/file_read_helpers.rb

+# frozen_string_literal: true
+
+module FileReadHelpers
+  def stub_file_read(file, content: nil, error: nil)
+    allow_original_file_read
+
+    expectation = allow(File).to receive(:read).with(file)
+
+    if error
+      expectation.and_raise(error)
+    elsif content
+      expectation.and_return(content)
+    else
+      expectation
+    end
+  end
+
+  def expect_file_read(file, content: nil, error: nil)
+    allow_original_file_read
+
+    expectation = expect(File).to receive(:read).with(file)
+
+    if error
+      expectation.and_raise(error)
+    elsif content
+      expectation.and_return(content)
+    else
+      expectation
+    end
+  end
+
+  def expect_file_not_to_read(file)
+    allow_original_file_read
+
+    expect(File).not_to receive(:read).with(file)
+  end
+
+  private
+
+  def allow_original_file_read
+    # Don't set this mock twice, otherwise subsequent calls will clobber
+    # previous mocks
+    return if @allow_original_file_read
+
+    @allow_original_file_read = true
+    allow(File).to receive(:read).and_call_original
+  end
+end

spec/support/shared_examples/services/metrics/dashboard_shared_examples.rb

@@ -46,7 +46,7 @@ RSpec.shared_examples 'refreshes cache when dashboard_version is changed' do
       allow(service).to receive(:dashboard_version).and_return('1', '2')
     end
 
-    expect(File).to receive(:read).twice.and_call_original
+    expect_file_read(Rails.root.join(described_class::DASHBOARD_PATH)).twice.and_call_original
 
     service = described_class.new(*service_params)
 

spec/tasks/gitlab/db_rake_spec.rb

@@ -129,7 +129,7 @@ RSpec.describe 'gitlab:db namespace rake task' do
     let(:output) { StringIO.new }
 
     before do
-      allow(File).to receive(:read).with(structure_file).and_return(input)
+      stub_file_read(structure_file, content: input)
       allow(File).to receive(:open).with(structure_file, any_args).and_yield(output)
     end
 

spec/tooling/lib/tooling/test_map_generator_spec.rb

 # frozen_string_literal: true
 
 require_relative '../../../../tooling/lib/tooling/test_map_generator'
+require_relative '../../../support/helpers/file_read_helpers'
 
 RSpec.describe Tooling::TestMapGenerator do
+  include FileReadHelpers
+
   subject { described_class.new }
 
   describe '#parse' do
@@ -39,8 +42,8 @@ RSpec.describe Tooling::TestMapGenerator do
     let(:pathname) { instance_double(Pathname) }
 
     before do
-      allow(File).to receive(:read).with('yaml1.yml').and_return(yaml1)
-      allow(File).to receive(:read).with('yaml2.yml').and_return(yaml2)
+      stub_file_read('yaml1.yml', content: yaml1)
+      stub_file_read('yaml2.yml', content: yaml2)
     end
 
     context 'with single yaml' do