From 3ea381d26963849a33d76d5b644c5f0044c9c778 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Mon, 1 Jul 2024 10:17:11 +0200 Subject: [PATCH] chore: adds quality gate for rerunning e2e spec files that are new or have been modified (#24556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **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: https://github.com/MetaMask/metamask-extension/issues/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] - :green_circle: 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 - :green_circle: 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 - :green_circle: 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 Co-authored-by: Howard Braham --- .circleci/config.yml | 31 +++++++++ .circleci/scripts/git-diff-develop.ts | 99 +++++++++++++++++++++++++++ development/lib/retry.js | 20 +++--- test/e2e/changedFilesUtil.js | 44 ++++++++++++ test/e2e/run-all.js | 62 +++++++++++++++-- test/e2e/run-e2e-test.js | 6 +- 6 files changed, 244 insertions(+), 18 deletions(-) create mode 100644 .circleci/scripts/git-diff-develop.ts create mode 100644 test/e2e/changedFilesUtil.js diff --git a/.circleci/config.yml b/.circleci/config.yml index 5ab0af8fb0cc..d9f0754335e7 100644 --- 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 new file mode 100644 index 000000000000..8b5680b17d3f --- /dev/null +++ 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 { + 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 { + 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 index e6e5dfc040af..813a63aa44e4 100644 --- a/development/lib/retry.js +++ b/development/lib/retry.js @@ -11,12 +11,12 @@ * @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( @@ -24,7 +24,7 @@ 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 new file mode 100644 index 000000000000..5ead76203db0 --- /dev/null +++ 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} 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} 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 }; diff --git a/test/e2e/run-all.js b/test/e2e/run-all.js index 0ff043261a7b..d52a37e9afe6 100644 --- a/test/e2e/run-all.js +++ b/test/e2e/run-all.js @@ -6,6 +6,7 @@ const { hideBin } = require('yargs/helpers'); const { runInShell } = require('../../development/lib/run-command'); const { exitWithError } = require('../../development/lib/exit-with-error'); const { loadBuildTypesConfig } = require('../../development/lib/build-type'); +const { filterE2eChangedFiles } = require('./changedFilesUtil'); // These tests should only be run on Flask for now. const FLASK_ONLY_TESTS = ['test-snap-namelookup.spec.js']; @@ -30,9 +31,47 @@ const getTestPathsForTestDir = async (testDir) => { return testPaths; }; +// Quality Gate Retries +const RETRIES_FOR_NEW_OR_CHANGED_TESTS = 5; + +/** + * Runs the quality gate logic to filter and append changed or new tests if present. + * + * @param {string} fullTestList - List of test paths to be considered. + * @param {string[]} changedOrNewTests - List of changed or new test paths. + * @returns {string} The updated full test list. + */ +async function applyQualityGate(fullTestList, changedOrNewTests) { + let qualityGatedList = fullTestList; + + if (changedOrNewTests.length > 0) { + // Filter to include only the paths present in fullTestList + const filteredTests = changedOrNewTests.filter((test) => + fullTestList.includes(test), + ); + + // If there are any filtered tests, append them to fullTestList + if (filteredTests.length > 0) { + const filteredTestsString = filteredTests.join('\n'); + for (let i = 0; i < RETRIES_FOR_NEW_OR_CHANGED_TESTS; i++) { + qualityGatedList += `\n${filteredTestsString}`; + } + } + } + + return qualityGatedList; +} + // For running E2Es in parallel in CI -function runningOnCircleCI(testPaths) { - const fullTestList = testPaths.join('\n'); +async function runningOnCircleCI(testPaths) { + const changedOrNewTests = await filterE2eChangedFiles(); + console.log('Changed or new test list:', changedOrNewTests); + + const fullTestList = await applyQualityGate( + testPaths.join('\n'), + changedOrNewTests, + ); + console.log('Full test list:', fullTestList); fs.writeFileSync('test/test-results/fullTestList.txt', fullTestList); @@ -46,7 +85,7 @@ function runningOnCircleCI(testPaths) { // Report if no tests found, exit gracefully if (result.indexOf('There were no tests found') !== -1) { console.log(`run-all.js info: Skipping this node because "${result}"`); - return []; + return { fullTestList: [] }; } // If there's no text file, it means this node has no tests, so exit gracefully @@ -54,13 +93,15 @@ function runningOnCircleCI(testPaths) { console.log( 'run-all.js info: Skipping this node because there is no myTestList.txt', ); - return []; + return { fullTestList: [] }; } // take the space-delimited result and split into an array - return fs + const myTestList = fs .readFileSync('test/test-results/myTestList.txt', { encoding: 'utf8' }) .split(' '); + + return { fullTestList: myTestList, changedOrNewTests }; } async function main() { @@ -204,8 +245,10 @@ async function main() { await fs.promises.mkdir('test/test-results/e2e', { recursive: true }); let myTestList; + let changedOrNewTests; if (process.env.CIRCLECI) { - myTestList = runningOnCircleCI(testPaths); + ({ fullTestList: myTestList, changedOrNewTests = [] } = + await runningOnCircleCI(testPaths)); } else { myTestList = testPaths; } @@ -217,7 +260,12 @@ async function main() { if (testPath !== '') { testPath = testPath.replace('\n', ''); // sometimes there's a newline at the end of the testPath console.log(`\nExecuting testPath: ${testPath}\n`); - await runInShell('node', [...args, testPath]); + + const isTestChangedOrNew = changedOrNewTests?.includes(testPath); + const qualityGateArg = isTestChangedOrNew + ? ['--stop-after-one-failure'] + : []; + await runInShell('node', [...args, ...qualityGateArg, testPath]); } } } diff --git a/test/e2e/run-e2e-test.js b/test/e2e/run-e2e-test.js index a4c0496dbda6..0acf0e571cdb 100644 --- a/test/e2e/run-e2e-test.js +++ b/test/e2e/run-e2e-test.js @@ -35,7 +35,7 @@ async function main() { 'Set how many times the test should be retried upon failure.', type: 'number', }) - .option('retry-until-failure', { + .option('stop-after-one-failure', { default: false, description: 'Retries until the test fails', type: 'boolean', @@ -73,7 +73,7 @@ async function main() { mmi, e2eTestPath, retries, - retryUntilFailure, + stopAfterOneFailure, leaveRunning, updateSnapshot, updatePrivacySnapshot, @@ -141,7 +141,7 @@ async function main() { const dir = 'test/test-results/e2e'; fs.mkdir(dir, { recursive: true }); - await retry({ retries, retryUntilFailure }, async () => { + await retry({ retries, stopAfterOneFailure }, async () => { await runInShell('yarn', [ 'mocha', `--config=${configFile}`,