Use FactoryDefault and let_it_be to speed up merge_request_spec

changed milestone to %13.3

	1 Warning
	Please add a throughput label to this merge request.

1 Message

CHANGELOG missing:

If you want to create a changelog entry for GitLab FOSS, run the following:

bin/changelog -m 39996 "Use FactoryDefault and let_it_be to speed up merge_request_spec"

If you want to create a changelog entry for GitLab EE, run the following instead:

bin/changelog --ee -m 39996 "Use FactoryDefault and let_it_be to speed up merge_request_spec"

If this merge request doesn’t need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.

Category	Reviewer	Maintainer
backend	Nicolas Dular (`@nicolasdular`) (UTC+2, 1 hour behind `@igor.drozdov`)	Ash McKenzie (`@ashmckenzie`) (UTC+10, 7 hours ahead of `@igor.drozdov`)

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

Hi @igor.drozdov,

Please add labels to your merge request, this helps triage community merge requests.

Thanks for your help!

You are welcome to help improve this comment.

added auto updated label

Setting label groupsource code based on @igor.drozdov's group.

added groupsource code label

added sectiondev label

changed milestone to %13.4

added missed:13.3 label

removed missed:13.3 label

added devopscreate rspec profiling labels and removed auto updated label

added 771 commits

aa7fea7e...54a24fb6 - 771 commits from branch id-use-factory-default-for-pipelines

Compare with previous version

changed target branch from id-use-factory-default-for-pipelines to master

changed the description

added 167 commits

54a24fb6...b9c42a28 - 166 commits from branch master
3b30daca - Use FactoryDefault to speed up merge_request_spec

Compare with previous version

changed title from Use FactoryDefault to speed up merge_request_spec to Use FactoryDefault and let_it_be to speed up merge_request_spec

@allison.browne could you please review this merge request?

assigned to @allison.browne

can this one use the default project?

we using a separate project for this one because 3002th line changes the repository of the project and we don't want it to be done for the default project

Yeah, this is very interesting. If I use the default project on it's own (diff below), the spec passes when run by itself (bin/rspec spec/models/merge_request_spec.rb:2293) but the when running the whole file bin/rspec spec/models/merge_request_spec.rb, I see failures.

This is potentially a source for confusion. Perhaps we can clearly document this failure mode in the guidelines for FactoryDefault ?

Also, I wonder if we should restrict FactoryDefault to a few well-known factories like Project, Users, and Groups.

$ git diff
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f7caf13f729..ff7f67d5a9d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2988,8 +2988,6 @@ def set_compare(merge_request)
   end
 
   describe '#branch_merge_base_commit' do
-    let(:project) { create(:project, :repository) }
-
     subject { create(:merge_request, :with_diffs, source_project: project) }
 
     context 'source and target branch exist' do

This is potentially a source for confusion. Perhaps we can clearly document this failure mode in the guidelines for FactoryDefault ?

Actually, this problem is not specific to FactoryDefault, I'd say it's specific to let_it_be because it creates a shared instance. We have a note regarding this (about reload and refind): https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#common-test-setup, but I guess refind/reload won't help if we deal with repository changes. It's probably worth noting in the docs though

Also, I wonder if we should restrict FactoryDefault to a few well-known factories like Project, Users, and Groups.

I think it's fine to get away without restrictions as long as we have it scoped to a single test suite

I think it's fine to get away without restrictions as long as we have it scoped to a single test suite

Ok, let's give it a try and see what feedback we get, if any!

@tkuah @igor.drozdov

I just ran into some confusion due to this. It seems like changes to the subject.project in earlier tests persist for later tests.

Is this due to factory_default: :keep and create_default(:project)?

Just like @tkuah noticed, running the test in isolation will pass, but running as part of the whole spec will fail.

All of this seems pretty bad to me .

An example is:

diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 1cf197322f5..f88244a3bc3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -14,6 +14,17 @@

   subject { create(:merge_request) }

+  it 'will always pass' do
+    subject.project.description = 'test'
+    subject.project.save
+
+    expect(subject.project.description).to eq('test')
+  end
+
+  it 'fails when run in the full spec, but passes when run in isolation, ' \
+     'or if `factory_default: :keep` is removed' do 
+    expect(subject.project.description).not_to eq('test')
+  end
+
   describe 'associations' do
     subject { build_stubbed(:merge_request) }

There are 3 tests that fail if we remove factory_default: :keep and I wonder whether those tests are poorly written and passing due to the accident of changes persisting in previous tests.

@.luke @igor.drozdov

Well, that sounds bad if there are specs that now depend on previous specs. Can we fix them ?

https://github.com/test-prof/test-prof/blob/master/docs/recipes/factory_default.md has more about factory defaults

approved this merge request

unassigned @allison.browne

@tkuah could you please review this merge request?

assigned to @tkuah

For my reference: !39809 (merged) introduced FactoryDefault

Thanks @igor.drozdov ! I see no issues with the MR itself - just have a question overall, which probably can be done later. See !39996 (comment 403552620)

unassigned @tkuah

assigned to @tkuah

resolved all threads

approved this merge request

enabled an automatic merge when the pipeline for fa7b3938 succeeds

merged

mentioned in commit 5ec10aa4

added workflowstaging label

added workflowcanary label and removed workflowstaging label

added workflowproduction label and removed workflowcanary label

This merge request has been deployed to the pre.gitlab.com environment, and will be included in the upcoming self-managed GitLab 13.5.0 release.

This comment is generated automatically using the Release Tools project.

added published label

This merge request has been deployed to the release.gitlab.net environment, and will be included in the upcoming self-managed GitLab 13.5.0 release.

This comment is generated automatically using the Release Tools project.

mentioned in merge request !51852 (merged)

mentioned in commit 957d598c

mentioned in merge request !53196 (merged)

mentioned in commit 628370a8

Use FactoryDefault and let_it_be to speed up merge_request_spec

What does this MR do?

Activity

Reviewer roulette

Use FactoryDefault and let_it_be to speed up merge_request_spec

What does this MR do?

Merge request reports

Activity

Reviewer roulette