chore: adds quality gate for rerunning e2e spec files that are new or…

… have been modified (#24556)  ## **Description** This PR adds a quality gate for new or modified e2e spec files. Whenever there is a PR which modifies or changes a test, this will be run more times, in order to prevent introducing a flakiness accidentally. It is done as follows: - Identifies any new or modified e2e file from inside the test/ folder using `git diff` and using these 2 filters: - `file.filename.startsWith('test/e2e/') &&` - `file.filename.endsWith('.spec.js') || file.filename.endsWith('.spec.ts') ` - Copies the given specs x5 times in the list of testpaths to execute -> this number is arbitrary, we could modify it to any value we want. The reason for taking this approach instead of changing the retrial number is to benefit of the parallelization, as @HowardBraham pointed out in a comment. - Since we already had a flag which could support the re-running successful tests, `--retry-until-failure` I just leveraged this into the `for` loop for each test, and if that testcase was identified as new/modified, the flag is added so the new tests fail fast without retrials ### Incremental git fetch depth within shallow clone We use git fetch with incremental depth as @danjm suggested. The ci environment uses a shallow clone, meaning we won't be able to succeed just by using git diff as it won't find the merge base. For fixing that, we start with a git fetch depth of 1, and keep incrementing the depth (1, 10, 100) it the error is `no merge base` up until 100. If the git diff still fails, we then do a full git fetch with the `unshallow` flag. - [Update] This is the working result with the last commit https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/89269/workflows/103b78a8-8f0d-4323-96b0-9e235c4cbc81/jobs/3296802 ![Screenshot from 2024-06-26 11-39-19](https://github.com/MetaMask/metamask-extension/assets/54408225/a2a89d6a-3a73-48ba-91a3-20aeadc38573) ### New ci Job The git diff is done in a new ci job which runs at the beginning in parallel of prep-deps. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24556?quickstart=1) ## **Related issues** Fixes: #24009 ## **Manual testing steps** 1. Check ci runs (notice previous runs had failing and changed tests on purpose, in order to try the different scenarios described below) ## **Screenshots/Recordings** =============================================== [UPDATE with the new code changes] - 🟢 Case 1: A test has changed -> it's rerun 1+5 times and it's successful (it will be run in different buckets) https://github.com/MetaMask/metamask-extension/assets/54408225/c1456104-1f5f-4ef3-9364-4e435f8797f4 https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/89277/workflows/7fce0a2e-773f-46da-8ab9-1dbec7992b58/jobs/3297267/parallel-runs/10?filterBy=ALL - 🟢 Case 2: A test has changed, but it has a mistake in the code (intentionally to simulate a flaky test) -> it fails immediately and there are no more retries. The rest of the tests, are retried if they failed as usual - Case for main test build test: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/89277/workflows/7fce0a2e-773f-46da-8ab9-1dbec7992b58/jobs/3297267/artifacts - Case for flask specific test: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/89277/workflows/7fce0a2e-773f-46da-8ab9-1dbec7992b58/jobs/3297277/artifacts - 🟢 Case 3: A PR has no test spec files changed -> nothing different happens - ci run: check current ci ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Howard Braham <[email protected]>
MetaMask · Jul 1, 2024 · 3ea381d · 3ea381d
1 parent f353f0c
commit 3ea381d
Show file tree

Hide file tree

Showing 6 changed files with 244 additions and 18 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -105,6 +105,9 @@ workflows:
             - prep-deps
       - check-pr-tag
       - prep-deps
+      - get-changed-files-with-git-diff:
+          requires:
+            - prep-deps
       - test-deps-audit:
           requires:
             - prep-deps
@@ -187,41 +190,51 @@ workflows:
       - test-e2e-chrome:
           requires:
             - prep-build-test
+            - get-changed-files-with-git-diff
       - test-e2e-chrome-confirmation-redesign:
           requires:
             - prep-build-confirmation-redesign-test
+            - get-changed-files-with-git-diff
       - test-e2e-firefox:
           requires:
             - prep-build-test-mv2
+            - get-changed-files-with-git-diff
       - test-e2e-firefox-confirmation-redesign:
           <<: *develop_master_rc_only
           requires:
             - prep-build-confirmation-redesign-test-mv2
+            - get-changed-files-with-git-diff
       - test-e2e-chrome-rpc:
           requires:
             - prep-build-test
+            - get-changed-files-with-git-diff
       - test-api-specs:
           requires:
             - prep-build-test
       - test-e2e-chrome-multiple-providers:
           requires:
             - prep-build-test
+            - get-changed-files-with-git-diff
       - test-e2e-chrome-flask:
           requires:
             - prep-build-test-flask
+            - get-changed-files-with-git-diff
       - test-e2e-firefox-flask:
           <<: *develop_master_rc_only
           requires:
             - prep-build-test-flask-mv2
+            - get-changed-files-with-git-diff
       - test-e2e-chrome-mmi:
           requires:
             - prep-build-test-mmi
+            - get-changed-files-with-git-diff
       - test-e2e-mmi-playwright - OPTIONAL:
           requires:
             - prep-build-test-mmi-playwright
       - test-e2e-chrome-rpc-mmi:
           requires:
             - prep-build-test-mmi
+            - get-changed-files-with-git-diff
       - test-e2e-chrome-vault-decryption:
           filters:
             branches:
@@ -230,6 +243,7 @@ workflows:
                 - /^Version-v(\d+)[.](\d+)[.](\d+)/
           requires:
             - prep-build
+            - get-changed-files-with-git-diff
       - test-unit-jest-main:
           requires:
             - prep-deps
@@ -472,6 +486,23 @@ jobs:
             - node_modules
             - build-artifacts
 
+  # This job is used for the e2e quality gate.
+  # It must be run before any job which uses the run-all.js script.
+  get-changed-files-with-git-diff:
+    executor: node-browsers-small
+    steps:
+      - run: *shallow-git-clone
+      - run: sudo corepack enable
+      - attach_workspace:
+          at: .
+      - run:
+          name: Get changed files with git diff
+          command: npx tsx .circleci/scripts/git-diff-develop.ts
+      - persist_to_workspace:
+          root: .
+          paths:
+            - changed-files
+
   validate-lavamoat-allow-scripts:
     executor: node-browsers-small
     steps:

diff --git a/.circleci/scripts/git-diff-develop.ts b/.circleci/scripts/git-diff-develop.ts
@@ -0,0 +1,99 @@
+import { hasProperty } from '@metamask/utils';
+import { exec as execCallback } from 'child_process';
+import fs from 'fs';
+import path from 'path';
+import { promisify } from 'util';
+
+const exec = promisify(execCallback);
+
+/**
+ * Fetches the git repository with a specified depth.
+ *
+ * @param depth - The depth to use for the fetch command.
+ * @returns True if the fetch is successful, otherwise false.
+ */
+async function fetchWithDepth(depth: number): Promise<boolean> {
+  try {
+    await exec(`git fetch --depth ${depth} origin develop`);
+    await exec(`git fetch --depth ${depth} origin ${process.env.CIRCLE_BRANCH}`);
+    return true;
+  } catch (error: unknown) {
+    console.error(`Failed to fetch with depth ${depth}:`, error);
+    return false;
+  }
+}
+
+/**
+ * Attempts to fetch the necessary commits until the merge base is found.
+ * It tries different fetch depths and performs a full fetch if needed.
+ *
+ * @throws If an unexpected error occurs during the execution of git commands.
+ */
+async function fetchUntilMergeBaseFound() {
+  const depths = [1, 10, 100];
+  for (const depth of depths) {
+    console.log(`Attempting git diff with depth ${depth}...`);
+    await fetchWithDepth(depth);
+
+    try {
+      await exec(`git merge-base origin/HEAD HEAD`);
+      return;
+    } catch (error: unknown) {
+      if (
+        error instanceof Error &&
+        hasProperty(error, 'code') &&
+        error.code === 1
+      ) {
+        console.error(`Error 'no merge base' encountered with depth ${depth}. Incrementing depth...`);
+      } else {
+        throw error;
+      }
+    }
+  }
+  await exec(`git fetch --unshallow origin develop`);
+}
+
+/**
+ * Performs a git diff command to get the list of files changed between the current branch and the origin.
+ * It first ensures that the necessary commits are fetched until the merge base is found.
+ *
+ * @returns The output of the git diff command, listing the changed files.
+ * @throws If unable to get the diff after fetching the merge base or if an unexpected error occurs.
+ */
+async function gitDiff(): Promise<string> {
+  await fetchUntilMergeBaseFound();
+  const { stdout: diffResult } = await exec(`git diff --name-only origin/HEAD...${process.env.CIRCLE_BRANCH}`);
+  if (!diffResult) {
+      throw new Error('Unable to get diff after full checkout.');
+  }
+  return diffResult;
+}
+
+/**
+ * Stores the output of git diff to a file.
+ *
+ * @returns Returns a promise that resolves when the git diff output is successfully stored.
+ */
+async function storeGitDiffOutput() {
+  try {
+    console.log("Attempting to get git diff...");
+    const diffOutput = await gitDiff();
+    console.log(diffOutput);
+
+    // Create the directory
+    const outputDir = 'changed-files';
+    fs.mkdirSync(outputDir, { recursive: true });
+
+    // Store the output of git diff
+    const outputPath = path.resolve(outputDir, 'changed-files.txt');
+    fs.writeFileSync(outputPath, diffOutput);
+
+    console.log(`Git diff results saved to ${outputPath}`);
+    process.exit(0);
+  } catch (error: any) {
+    console.error('An error occurred:', error.message);
+    process.exit(1);
+  }
+}
+
+storeGitDiffOutput();
diff --git a/development/lib/retry.js b/development/lib/retry.js
@@ -11,20 +11,20 @@
  * @param {string} [args.rejectionMessage] - The message for the rejected promise
  * this function will return in the event of failure. (Default: "Retry limit
  * reached")
- * @param {boolean} [args.retryUntilFailure] - Retries until the function fails.
+ * @param {boolean} [args.stopAfterOneFailure] - Retries until the function fails.
  * @param {Function} functionToRetry - The function that is run and tested for
  * failure.
  * @returns {Promise<* | null | Error>} a promise that either resolves with one of the following:
  * - If successful, resolves with the return value of functionToRetry.
- * - If functionToRetry fails while retryUntilFailure is true, resolves with null.
+ * - If functionToRetry fails while stopAfterOneFailure is true, resolves with null.
  * - Otherwise it is rejected with rejectionMessage.
  */
 async function retry(
   {
     retries,
     delay = 0,
     rejectionMessage = 'Retry limit reached',
-    retryUntilFailure = false,
+    stopAfterOneFailure = false,
   },
   functionToRetry,
 ) {
@@ -36,7 +36,7 @@ async function retry(
 
     try {
       const result = await functionToRetry();
-      if (!retryUntilFailure) {
+      if (!stopAfterOneFailure) {
         return result;
       }
     } catch (error) {
@@ -46,18 +46,22 @@ async function retry(
         console.error('error caught in retry():', error);
       }
 
-      if (attempts < retries) {
-        console.log('Ready to retry() again');
+      if (stopAfterOneFailure) {
+        throw new Error('Test failed. No more retries will be performed');
       }
 
-      if (retryUntilFailure) {
-        return null;
+      if (attempts < retries) {
+        console.log('Ready to retry() again');
       }
     } finally {
       attempts += 1;
     }
   }
 
+  if (stopAfterOneFailure) {
+    return null;
+  }
+
   throw new Error(rejectionMessage);
 }
 

diff --git a/test/e2e/changedFilesUtil.js b/test/e2e/changedFilesUtil.js
@@ -0,0 +1,44 @@
+const fs = require('fs').promises;
+const path = require('path');
+
+const BASE_PATH = path.resolve(__dirname, '..', '..');
+const CHANGED_FILES_PATH = path.join(
+  BASE_PATH,
+  'changed-files',
+  'changed-files.txt',
+);
+
+/**
+ * Reads the list of changed files from the git diff file.
+ *
+ * @returns {Promise<string[]>} An array of changed file paths.
+ */
+async function readChangedFiles() {
+  try {
+    const data = await fs.readFile(CHANGED_FILES_PATH, 'utf8');
+    const changedFiles = data.split('\n');
+    return changedFiles;
+  } catch (error) {
+    console.error('Error reading from file:', error);
+    return '';
+  }
+}
+
+/**
+ * Filters the list of changed files to include only E2E test files within the 'test/e2e/' directory.
+ *
+ * @returns {Promise<string[]>} An array of filtered E2E test file paths.
+ */
+async function filterE2eChangedFiles() {
+  const changedFiles = await readChangedFiles();
+  const e2eChangedFiles = changedFiles
+    .filter(
+      (file) =>
+        file.startsWith('test/e2e/') &&
+        (file.endsWith('.spec.js') || file.endsWith('.spec.ts')),
+    )
+    .map((file) => `${BASE_PATH}/${file}`);
+  return e2eChangedFiles;
+}
+
+module.exports = { filterE2eChangedFiles };