Merge branch...

Merge branch '9858-follow-up-on-a-bug-that-happens-when-using-a-factory-built-object-in-the-where-block-of-rspec-parameterized-tablesyntax' into 'master'

Fix and document an RSpec::Parameterized::TableSyntax edge-case

Closes #9858

See merge request gitlab-org/gitlab-ee!9621

doc/development/testing_guide/best_practices.md

@@ -358,16 +358,11 @@ range of inputs, might look like this:
 describe "#==" do
   using RSpec::Parameterized::TableSyntax
 
-  let(:project1) { create(:project) }
-  let(:project2) { create(:project) }
   where(:a, :b, :result) do
     1         | 1        | true
     1         | 2        | false
     true      | true     | true
     true      | false    | false
-    project1  | project1 | true
-    project2  | project2 | true
-    project 1 | project2 | false
   end
 
   with_them do
@@ -380,6 +375,11 @@ describe "#==" do
 end
 ```
 
+CAUTION: **Caution:**
+Only use simple values as input in the `where` block. Using procs, stateful
+objects, FactoryBot-created objects etc. can lead to
+[unexpected results](https://github.com/tomykaira/rspec-parameterized/issues/8).
+
 ### Prometheus tests
 
 Prometheus metrics may be preserved from one test run to another. To ensure that metrics are

ee/spec/factories/projects.rb

+# frozen_string_literal: true
+
+FactoryBot.modify do
+  factory :project do
+    transient do
+      last_update_at nil
+      last_successful_update_at nil
+      retry_count 0
+    end
+
+    after(:create) do |project, evaluator|
+      import_state = project.import_state
+      if import_state
+        import_state.last_successful_update_at = evaluator.last_successful_update_at
+        import_state.retry_count = evaluator.retry_count
+
+        case import_state.status.to_sym
+        when :scheduled
+          import_state.last_update_scheduled_at = Time.now
+        when :started
+          import_state.last_update_started_at = Time.now
+        when :finished
+          timestamp = evaluator.last_update_at || Time.now
+          import_state.last_update_at = timestamp
+          import_state.last_successful_update_at = timestamp
+        when :failed
+          import_state.last_update_at = evaluator.last_update_at || Time.now
+        end
+        import_state.save!
+      end
+    end
+
+    trait :import_none do
+      import_status :none
+    end
+
+    trait :import_hard_failed do
+      import_status :failed
+      last_update_at { Time.now - 1.minute }
+      retry_count { Gitlab::Mirror::MAX_RETRY + 1 }
+    end
+
+    trait :disabled_mirror do
+      mirror false
+      import_url { generate(:url) }
+      mirror_user_id { creator_id }
+    end
+
+    trait :mirror do
+      mirror true
+      import_url { generate(:url) }
+      mirror_user_id { creator_id }
+    end
+
+    trait :random_last_repository_updated_at do
+      last_repository_updated_at { rand(1.year).seconds.ago }
+    end
+  end
+end

ee/spec/models/vulnerabilities/occurrence_spec.rb

@@ -46,15 +46,15 @@ describe Vulnerabilities::Occurrence do
       using RSpec::Parameterized::TableSyntax
 
       # we use block to delay object creations
-      where(:key, :value_block) do
-        :primary_identifier | -> { create(:vulnerabilities_identifier) }
-        :scanner | -> { create(:vulnerabilities_scanner) }
-        :project | -> { create(:project) }
+      where(:key, :factory_name) do
+        :primary_identifier | :vulnerabilities_identifier
+        :scanner | :vulnerabilities_scanner
+        :project | :project
       end
 
       with_them do
         it "is valid" do
-          expect { new_occurrence.update!({ key => value_block.call }) }.not_to raise_error
+          expect { new_occurrence.update!({ key => create(factory_name) }) }.not_to raise_error
         end
       end
     end

spec/factories/projects.rb

@@ -32,9 +32,6 @@ FactoryBot.define do
       group_runners_enabled nil
       import_status nil
       import_jid nil
-      last_update_at nil
-      last_successful_update_at nil
-      retry_count 0
     end
 
     after(:create) do |project, evaluator|
@@ -73,9 +70,6 @@ FactoryBot.define do
       if evaluator.import_status
         import_state = project.import_state || project.build_import_state
         import_state.status = evaluator.import_status
-        import_state.last_update_at = evaluator.last_update_at
-        import_state.last_successful_update_at = evaluator.last_successful_update_at
-        import_state.retry_count = evaluator.retry_count
         import_state.jid = evaluator.import_jid
         import_state.save
       end
@@ -93,55 +87,20 @@ FactoryBot.define do
       visibility_level Gitlab::VisibilityLevel::PRIVATE
     end
 
-    trait :import_none do
-      import_status :none
-    end
-
     trait :import_scheduled do
       import_status :scheduled
-
-      after(:create) do |project, _|
-        project.import_state&.update_attributes(last_update_scheduled_at: Time.now)
-      end
     end
 
     trait :import_started do
       import_status :started
-
-      after(:create) do |project, _|
-        project.import_state&.update_attributes(last_update_started_at: Time.now)
-      end
     end
 
     trait :import_finished do
-      timestamp = Time.now
-
       import_status :finished
-      last_update_at timestamp
-      last_successful_update_at timestamp
     end
 
     trait :import_failed do
       import_status :failed
-      last_update_at { Time.now }
-    end
-
-    trait :import_hard_failed do
-      import_status :failed
-      last_update_at { Time.now - 1.minute }
-      retry_count { Gitlab::Mirror::MAX_RETRY + 1 }
-    end
-
-    trait :disabled_mirror do
-      mirror false
-      import_url { generate(:url) }
-      mirror_user_id { creator_id }
-    end
-
-    trait :mirror do
-      mirror true
-      import_url { generate(:url) }
-      mirror_user_id { creator_id }
     end
 
     trait :archived do
@@ -247,7 +206,6 @@ FactoryBot.define do
         url "http://foo.com"
         enabled true
       end
-
       after(:create) do |project, evaluator|
         project.remote_mirrors.create!(url: evaluator.url, enabled: evaluator.enabled)
       end
@@ -287,10 +245,6 @@ FactoryBot.define do
       end
     end
 
-    trait :random_last_repository_updated_at do
-      last_repository_updated_at { rand(1.year).seconds.ago }
-    end
-
     trait(:wiki_enabled)            { wiki_access_level ProjectFeature::ENABLED }
     trait(:wiki_disabled)           { wiki_access_level ProjectFeature::DISABLED }
     trait(:wiki_private)            { wiki_access_level ProjectFeature::PRIVATE }

spec/models/prometheus_metric_spec.rb

@@ -4,7 +4,6 @@ require 'spec_helper'
 
 describe PrometheusMetric do
   subject { build(:prometheus_metric) }
-  let(:other_project) { build(:project) }
 
   it_behaves_like 'having unique enum values'
 
@@ -16,17 +15,17 @@ describe PrometheusMetric do
   describe 'common metrics' do
     using RSpec::Parameterized::TableSyntax
 
-    where(:common, :project, :result) do
-      false | other_project | true
-      false | nil           | false
-      true  | other_project | false
-      true  | nil           | true
+    where(:common, :with_project, :result) do
+      false | true | true
+      false | false | false
+      true  | true | false
+      true  | false | true
     end
 
     with_them do
       before do
         subject.common = common
-        subject.project = project
+        subject.project = with_project ? build(:project) : nil
       end
 
       it { expect(subject.valid?).to eq(result) }