Merge branch 'alberts-run-less-test-before-approval' into 'master'

Run minimal rspec jobs before merge request is approved

See merge request gitlab-org/gitlab!64903

.gitlab/ci/rails.gitlab-ci.yml

@@ -641,36 +641,33 @@ rspec-ee unit pg12 geo:
     - .rails:rules:ee-only-unit
     - .rspec-ee-unit-geo-parallel
 
-# FIXME: Temporarily disable geo minimal rspec jobs https://gitlab.com/gitlab-org/gitlab/-/issues/294212
-#rspec-ee unit pg12 geo minimal:
-#  extends:
-#    - rspec-ee unit pg12 geo
-#    - .minimal-rspec-tests
-#    - .rails:rules:ee-only-unit:minimal
+rspec-ee unit pg12 geo minimal:
+  extends:
+    - rspec-ee unit pg12 geo
+    - .minimal-rspec-tests
+    - .rails:rules:ee-only-unit:minimal
 
 rspec-ee integration pg12 geo:
   extends:
     - .rspec-ee-base-geo-pg12
     - .rails:rules:ee-only-integration
 
-# FIXME: Temporarily disable geo minimal rspec jobs https://gitlab.com/gitlab-org/gitlab/-/issues/294212
-#rspec-ee integration pg12 geo minimal:
-#  extends:
-#    - rspec-ee integration pg12 geo
-#    - .minimal-rspec-tests
-#    - .rails:rules:ee-only-integration:minimal
+rspec-ee integration pg12 geo minimal:
+  extends:
+    - rspec-ee integration pg12 geo
+    - .minimal-rspec-tests
+    - .rails:rules:ee-only-integration:minimal
 
 rspec-ee system pg12 geo:
   extends:
     - .rspec-ee-base-geo-pg12
     - .rails:rules:ee-only-system
 
-# FIXME: Temporarily disable geo minimal rspec jobs https://gitlab.com/gitlab-org/gitlab/-/issues/294212
-#rspec-ee system pg12 geo minimal:
-#  extends:
-#    - rspec-ee system pg12 geo
-#    - .minimal-rspec-tests
-#    - .rails:rules:ee-only-system:minimal
+rspec-ee system pg12 geo minimal:
+  extends:
+    - rspec-ee system pg12 geo
+    - .minimal-rspec-tests
+    - .rails:rules:ee-only-system:minimal
 
 db:rollback geo:
   extends:

.gitlab/ci/rules.gitlab-ci.yml

@@ -34,6 +34,15 @@
 .if-merge-request: &if-merge-request
   if: '$CI_MERGE_REQUEST_IID'
 
+.if-merge-request-approved: &if-merge-request-approved
+  if: '$CI_MERGE_REQUEST_IID && $CI_MERGE_REQUEST_APPROVED'
+
+.if-merge-request-not-approved: &if-merge-request-not-approved
+  if: '$CI_MERGE_REQUEST_IID && $CI_MERGE_REQUEST_APPROVED != "true"'
+
+.if-automated-merge-request: &if-automated-merge-request
+  if: '$CI_MERGE_REQUEST_SOURCE_BRANCH_NAME == "release-tools/update-gitaly" || $CI_MERGE_REQUEST_TARGET_BRANCH_NAME =~ /stable-ee$/'
+
 .if-merge-request-title-as-if-foss: &if-merge-request-title-as-if-foss
   if: '$CI_MERGE_REQUEST_TITLE =~ /RUN AS-IF-FOSS/'
 
@@ -70,9 +79,6 @@
 .if-cache-credentials-schedule: &if-cache-credentials-schedule
   if: '$CI_REPO_CACHE_CREDENTIALS && $CI_PIPELINE_SOURCE == "schedule"'
 
-.if-merge-request-rspec-minimal-disabled: &if-merge-request-rspec-minimal-disabled
-  if: '$CI_MERGE_REQUEST_IID && $RSPEC_MINIMAL_ENABLED != "true"'
-
 .if-rspec-fail-fast-disabled: &if-rspec-fail-fast-disabled
   if: '$RSPEC_FAIL_FAST_ENABLED != "true"'
 
@@ -612,12 +618,18 @@
 ###############
 .rails:rules:ee-and-foss-migration:
   rules:
-    - changes: *db-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *db-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *db-patterns
 
 .rails:rules:ee-and-foss-migration:minimal:
   rules:
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -643,12 +655,18 @@
 
 .rails:rules:ee-and-foss-unit:
   rules:
-    - changes: *backend-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *backend-patterns
 
 .rails:rules:ee-and-foss-unit:minimal:
   rules:
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -660,12 +678,18 @@
 
 .rails:rules:ee-and-foss-integration:
   rules:
-    - changes: *backend-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *backend-patterns
 
 .rails:rules:ee-and-foss-integration:minimal:
   rules:
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -677,12 +701,18 @@
 
 .rails:rules:ee-and-foss-system:
   rules:
-    - changes: *code-backstage-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *code-backstage-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *code-backstage-patterns
 
 .rails:rules:ee-and-foss-system:minimal:
   rules:
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -694,12 +724,18 @@
 
 .rails:rules:ee-and-foss-fast_spec_helper:
   rules:
-    - changes: ["config/**/*"]
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: ["config/**/*"]
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: ["config/**/*"]
 
 .rails:rules:ee-and-foss-fast_spec_helper:minimal:
   rules:
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -718,14 +754,20 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - changes: *db-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *db-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *db-patterns
 
 .rails:rules:ee-only-migration:minimal:
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -739,14 +781,20 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - changes: *backend-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *backend-patterns
 
 .rails:rules:ee-only-unit:minimal:
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -760,14 +808,20 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - changes: *backend-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *backend-patterns
 
 .rails:rules:ee-only-integration:minimal:
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -781,14 +835,20 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - changes: *code-backstage-patterns
     - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *code-backstage-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
+    - changes: *code-backstage-patterns
 
 .rails:rules:ee-only-system:minimal:
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request-title-run-all-rspec
       when: never
@@ -802,11 +862,15 @@
   rules:
     - <<: *if-not-ee
       when: never
+    - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *db-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
     - <<: *if-security-merge-request
       changes: *db-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *db-patterns
-    - <<: *if-merge-request-title-run-all-rspec
     - <<: *if-merge-request
       changes: *ci-patterns
 
@@ -814,7 +878,9 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request
       changes: *ci-patterns
@@ -823,17 +889,20 @@
       changes: *db-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *db-patterns
-    - <<: *if-merge-request-title-run-all-rspec
 
 .rails:rules:as-if-foss-unit:
   rules:
     - <<: *if-not-ee
       when: never
+    - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
     - <<: *if-security-merge-request
       changes: *backend-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *backend-patterns
-    - <<: *if-merge-request-title-run-all-rspec
     - <<: *if-merge-request
       changes: *ci-patterns
 
@@ -841,7 +910,9 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request
       changes: *ci-patterns
@@ -850,17 +921,20 @@
       changes: *backend-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *backend-patterns
-    - <<: *if-merge-request-title-run-all-rspec
 
 .rails:rules:as-if-foss-integration:
   rules:
     - <<: *if-not-ee
       when: never
+    - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *backend-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
     - <<: *if-security-merge-request
       changes: *backend-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *backend-patterns
-    - <<: *if-merge-request-title-run-all-rspec
     - <<: *if-merge-request
       changes: *ci-patterns
 
@@ -868,7 +942,9 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request
       changes: *ci-patterns
@@ -877,17 +953,20 @@
       changes: *backend-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *backend-patterns
-    - <<: *if-merge-request-title-run-all-rspec
 
 .rails:rules:as-if-foss-system:
   rules:
     - <<: *if-not-ee
       when: never
+    - <<: *if-merge-request-title-run-all-rspec
+    - <<: *if-automated-merge-request
+      changes: *code-backstage-patterns
+    - <<: *if-merge-request-not-approved
+      when: never
     - <<: *if-security-merge-request
       changes: *code-backstage-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *code-backstage-patterns
-    - <<: *if-merge-request-title-run-all-rspec
     - <<: *if-merge-request
       changes: *ci-patterns
 
@@ -895,7 +974,9 @@
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-merge-request-rspec-minimal-disabled
+    - <<: *if-merge-request-approved
+      when: never
+    - <<: *if-automated-merge-request
       when: never
     - <<: *if-merge-request
       changes: *ci-patterns
@@ -904,7 +985,6 @@
       changes: *code-backstage-patterns
     - <<: *if-merge-request-title-as-if-foss
       changes: *code-backstage-patterns
-    - <<: *if-merge-request-title-run-all-rspec
 
 .rails:rules:ee-and-foss-db-library-code:
   rules:

doc/development/code_review.md

@@ -423,6 +423,7 @@ WARNING:
     do not merge the merge request** except for
     [very specific cases](https://about.gitlab.com/handbook/engineering/workflow/#criteria-for-merging-during-broken-master).
     For other cases, follow these [handbook instructions](https://about.gitlab.com/handbook/engineering/workflow/#merging-during-broken-master).
+  - If the latest pipeline was created before the merge request was approved, start a new pipeline to ensure that full RSpec suite has been run. You may skip this step only if the merge request does not contain any backend change.
   - If the **latest [Pipeline for Merged Results](../ci/pipelines/pipelines_for_merged_results.md)** finished less than 2 hours ago, you
     might merge without starting a new pipeline as the merge request is close
     enough to `main`.

doc/development/pipelines.md

@@ -421,58 +421,18 @@ This experiment is only enabled when the CI/CD variable `RSPEC_FAIL_FAST_ENABLED
 The test files related to the Merge Request are determined using the [`test_file_finder`](https://gitlab.com/gitlab-org/ci-cd/test_file_finder) gem.
 We are using a custom mapping between source file to test files, maintained in the `tests.yml` file.
 
-### RSpec minimal job experiment
+### RSpec minimal jobs
 
-As part of the objective to improve overall pipeline duration, we are experimenting with a minimal set of RSpec tests.
-The purpose of this experiment is to verify if we are able to run a minimal set of RSpec tests in a Merge Request pipeline,
-without resulting in increased number of broken main branch.
+Before a merge request is approved, the pipeline will run a minimal set of RSpec tests that are related to the merge request changes.
+This is to reduce the pipeline cost and shorten the job duration.
 
 To identify the minimal set of tests needed, we use [Crystalball gem](https://github.com/toptal/crystalball) to create a test mapping.
 The test mapping contains a map of each source files to a list of test files which is dependent of the source file.
 This mapping is currently generated using a combination of test coverage tracing and a static mapping.
 In the `detect-tests` job, we use this mapping to identify the minimal tests needed for the current Merge Request.
 
-In this experiment, each `rspec` job is accompanied with a `minimal` version.
-For example, `rspec unit` job has a corresponding `rspec unit minimal` job.
-During the experiment, each Merge Request pipeline will contain both versions of the job, running in parallel.
-
-To illustrate this:
-
-```mermaid
-graph LR
-    A --"artifact: list of test files"--> C1 & D1 & E1 & F1
-
-    subgraph "prepare stage";
-        A["detect-tests"];
-    end
-
-    subgraph "test stage";
-        C["rspec migration"];
-        C1["rspec migration minimal"];
-        D["rspec unit"];
-        D1["rspec unit minimal"];
-        E["rspec integration"];
-        E1["rspec integration minimal"];
-        F["rspec system"];
-        F1["rspec system minimal"];
-    end
-```
-
-The result of both set of jobs in the pipeline is then compared to identify any false positive.
-A list of such pipeline can be found in [Sisense](https://app.periscopedata.com/app/gitlab/496118/Engineering-Productivity-Sandbox?widget=10492739&udv=833427).
-
-A false positive is defined as a pipeline where the `minimal` jobs passed, but the non-`minimal` jobs failed.
-This indicates that the changeset resulted in a test failure, which was not detected by the `minimal` jobs.
-Consequently, this signifies a gap in the test mapping used, which would need to be rectified.
-
-#### Findings
-
-After a round of the experiment done in December 2020,
-we discovered that it was challenging to achieve a mapping that gives high confidence at the moment,
-because of 2 reasons:
-
-- Each identified gap in the test mapping is unique, each needing its own investigation and improvement to the creation of the test mapping.
-- There is a large number of flaky tests which added a lot of noise in the data, slowing down the investigation process.
+After a merge request has been approved, the pipeline would contain the full RSpec tests. This will ensure that all tests
+have been run before a merge request is merged.
 
 ### PostgreSQL versions testing