diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md index 5786287d00c..d25d856c3a3 100644 --- a/doc/development/gotchas.md +++ b/doc/development/gotchas.md @@ -92,6 +92,54 @@ describe API::Labels do end ``` +## Avoid using `allow_any_instance_of` in RSpec + +### Why + +* Because it is not isolated therefore it might be broken at times. +* Because it doesn't work whenever the method we want to stub was defined + in a prepended module, which is very likely the case in EE. We could see + error like this: + + 1.1) Failure/Error: allow_any_instance_of(ApplicationSetting).to receive_messages(messages) + Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported. + +### Alternative: `expect_next_instance_of` + +Instead of writing: + +```ruby +# Don't do this: +allow_any_instance_of(Project).to receive(:add_import_job) +``` + +We could write: + +```ruby +# Do this: +expect_next_instance_of(Project) do |project| + expect(project).to receive(:add_import_job) +end +``` + +If we also want to expect the instance was initialized with some particular +arguments, we could also pass it to `expect_next_instance_of` like: + +```ruby +# Do this: +expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service| + expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref) +end +``` + +This would expect the following: + +```ruby +# Above expects: +refresh_service = MergeRequests::RefreshService.new(project, user) +refresh_service.execute(oldrev, newrev, ref) +``` + ## Do not `rescue Exception` See ["Why is it bad style to `rescue Exception => e` in Ruby?"][Exception]. diff --git a/spec/lib/gitlab/bitbucket_import/project_creator_spec.rb b/spec/lib/gitlab/bitbucket_import/project_creator_spec.rb index c3f528dd6fc..ed6fa3d229f 100644 --- a/spec/lib/gitlab/bitbucket_import/project_creator_spec.rb +++ b/spec/lib/gitlab/bitbucket_import/project_creator_spec.rb @@ -25,7 +25,9 @@ describe Gitlab::BitbucketImport::ProjectCreator do end it 'creates project' do - allow_any_instance_of(Project).to receive(:add_import_job) + expect_next_instance_of(Project) do |project| + expect(project).to receive(:add_import_job) + end project_creator = described_class.new(repo, 'vim', namespace, user, access_params) project = project_creator.execute diff --git a/spec/lib/gitlab/gitlab_import/project_creator_spec.rb b/spec/lib/gitlab/gitlab_import/project_creator_spec.rb index 5ea086e4abd..b814f5fc76c 100644 --- a/spec/lib/gitlab/gitlab_import/project_creator_spec.rb +++ b/spec/lib/gitlab/gitlab_import/project_creator_spec.rb @@ -21,7 +21,9 @@ describe Gitlab::GitlabImport::ProjectCreator do end it 'creates project' do - allow_any_instance_of(Project).to receive(:add_import_job) + expect_next_instance_of(Project) do |project| + expect(project).to receive(:add_import_job) + end project_creator = described_class.new(repo, namespace, user, access_params) project = project_creator.execute diff --git a/spec/lib/gitlab/google_code_import/project_creator_spec.rb b/spec/lib/gitlab/google_code_import/project_creator_spec.rb index 24cd518c77b..b959e006292 100644 --- a/spec/lib/gitlab/google_code_import/project_creator_spec.rb +++ b/spec/lib/gitlab/google_code_import/project_creator_spec.rb @@ -16,7 +16,9 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do end it 'creates project' do - allow_any_instance_of(Project).to receive(:add_import_job) + expect_next_instance_of(Project) do |project| + expect(project).to receive(:add_import_job) + end project_creator = described_class.new(repo, namespace, user) project = project_creator.execute diff --git a/spec/lib/gitlab/legacy_github_import/project_creator_spec.rb b/spec/lib/gitlab/legacy_github_import/project_creator_spec.rb index 972b17d5b12..3d4240fa4ba 100644 --- a/spec/lib/gitlab/legacy_github_import/project_creator_spec.rb +++ b/spec/lib/gitlab/legacy_github_import/project_creator_spec.rb @@ -17,7 +17,10 @@ describe Gitlab::LegacyGithubImport::ProjectCreator do before do namespace.add_owner(user) - allow_any_instance_of(Project).to receive(:add_import_job) + + expect_next_instance_of(Project) do |project| + expect(project).to receive(:add_import_job) + end end describe '#execute' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index dac609e2545..fdce8e84620 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -69,6 +69,7 @@ RSpec.configure do |config| config.include StubFeatureFlags config.include StubGitlabCalls config.include StubGitlabData + config.include ExpectNextInstanceOf config.include TestEnv config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::IntegrationHelpers, type: :feature diff --git a/spec/support/helpers/expect_next_instance_of.rb b/spec/support/helpers/expect_next_instance_of.rb new file mode 100644 index 00000000000..b95046b2b42 --- /dev/null +++ b/spec/support/helpers/expect_next_instance_of.rb @@ -0,0 +1,13 @@ +module ExpectNextInstanceOf + def expect_next_instance_of(klass, *new_args) + receive_new = receive(:new) + receive_new.with(*new_args) if new_args.any? + + expect(klass).to receive_new + .and_wrap_original do |method, *original_args| + method.call(*original_args).tap do |instance| + yield(instance) + end + end + end +end diff --git a/spec/workers/update_merge_requests_worker_spec.rb b/spec/workers/update_merge_requests_worker_spec.rb index 80137815d2b..0b553db0ca4 100644 --- a/spec/workers/update_merge_requests_worker_spec.rb +++ b/spec/workers/update_merge_requests_worker_spec.rb @@ -18,13 +18,9 @@ describe UpdateMergeRequestsWorker do end it 'executes MergeRequests::RefreshService with expected values' do - expect(MergeRequests::RefreshService).to receive(:new) - .with(project, user).and_wrap_original do |method, *args| - method.call(*args).tap do |refresh_service| - expect(refresh_service) - .to receive(:execute).with(oldrev, newrev, ref) - end - end + expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service| + expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref) + end perform end