Commit e446e871 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'ce-6436-6149-unify-project_creator_spec' into 'master'

CE: Resolve "Extract EE specific files/lines for project_creator_spec"

See merge request gitlab-org/gitlab-ce!19850
parents 8f99d303 fda4d554
...@@ -92,6 +92,54 @@ describe API::Labels do ...@@ -92,6 +92,54 @@ describe API::Labels do
end 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` ## Do not `rescue Exception`
See ["Why is it bad style to `rescue Exception => e` in Ruby?"][Exception]. See ["Why is it bad style to `rescue Exception => e` in Ruby?"][Exception].
......
...@@ -25,7 +25,9 @@ describe Gitlab::BitbucketImport::ProjectCreator do ...@@ -25,7 +25,9 @@ describe Gitlab::BitbucketImport::ProjectCreator do
end end
it 'creates project' do 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_creator = described_class.new(repo, 'vim', namespace, user, access_params)
project = project_creator.execute project = project_creator.execute
......
...@@ -21,7 +21,9 @@ describe Gitlab::GitlabImport::ProjectCreator do ...@@ -21,7 +21,9 @@ describe Gitlab::GitlabImport::ProjectCreator do
end end
it 'creates project' do 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_creator = described_class.new(repo, namespace, user, access_params)
project = project_creator.execute project = project_creator.execute
......
...@@ -16,7 +16,9 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do ...@@ -16,7 +16,9 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do
end end
it 'creates project' do 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_creator = described_class.new(repo, namespace, user)
project = project_creator.execute project = project_creator.execute
......
...@@ -17,7 +17,10 @@ describe Gitlab::LegacyGithubImport::ProjectCreator do ...@@ -17,7 +17,10 @@ describe Gitlab::LegacyGithubImport::ProjectCreator do
before do before do
namespace.add_owner(user) 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 end
describe '#execute' do describe '#execute' do
......
...@@ -69,6 +69,7 @@ RSpec.configure do |config| ...@@ -69,6 +69,7 @@ RSpec.configure do |config|
config.include StubFeatureFlags config.include StubFeatureFlags
config.include StubGitlabCalls config.include StubGitlabCalls
config.include StubGitlabData config.include StubGitlabData
config.include ExpectNextInstanceOf
config.include TestEnv config.include TestEnv
config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::ControllerHelpers, type: :controller
config.include Devise::Test::IntegrationHelpers, type: :feature config.include Devise::Test::IntegrationHelpers, type: :feature
......
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
...@@ -18,13 +18,9 @@ describe UpdateMergeRequestsWorker do ...@@ -18,13 +18,9 @@ describe UpdateMergeRequestsWorker do
end end
it 'executes MergeRequests::RefreshService with expected values' do it 'executes MergeRequests::RefreshService with expected values' do
expect(MergeRequests::RefreshService).to receive(:new) expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service|
.with(project, user).and_wrap_original do |method, *args| expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref)
method.call(*args).tap do |refresh_service| end
expect(refresh_service)
.to receive(:execute).with(oldrev, newrev, ref)
end
end
perform perform
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment