From 594d580016f2cddd3510255a4e8e0197cb249fee Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 3 Sep 2024 10:00:20 -0230 Subject: [PATCH] ci: Prevent E2E timeouts on release changes (#26846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Our CI was setup to compare each branch with `develop`, and run any tests that have changed since `develop` extra times to catch new flaky test regressions. Unfortunately this was happening even for PRs that do not target develop, resulting in massive lists of "changed" tests that ended up causing persistent test timeouts. The quality gate should only impact PRs that target `develop`. PRs that target other branches no longer repeat "changed" tests. This should fix release PR e2e test timeouts caused by excessive e2e test runs. You can see an example of this problem occurring here: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/98357/workflows/b61a16b5-acfd-411f-b82e-7a31706ca658/jobs/3660843 Notice that `/home/circleci/project/test/e2e/tests/request-queuing/ui.spec.js` appears 6 times in the full test list, as to every other "changed" test. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26846?quickstart=1) ## **Related issues** This quality gate was first introduced in #24556 ## **Manual testing steps** The script can be run locally if you setup "fake" CircleCI environment variables to allow it to run properly. For example: ``` CIRCLE_PULL_REQUEST=https://github.com/MetaMask/metamask-extension/pull/26822 yarn tsx ./.circleci/scripts/git-diff-develop.ts ``` The CI run for this PR can be viewed as well: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/98369/workflows/e9de2170-a19e-433e-a83c-846eeb239842/jobs/3661034 ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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. --- .circleci/config.yml | 9 +----- .circleci/scripts/git-diff-develop.ts | 45 ++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 25e99cee682e..1a1acc7c09fd 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -53,13 +53,6 @@ develop_master_rc_only: &develop_master_rc_only - master - /^Version-v(\d+)[.](\d+)[.](\d+)/ -exclude_develop_master_rc: &exclude_develop_master_rc - filters: - branches: - ignore: - - develop - - master - - /^Version-v(\d+)[.](\d+)[.](\d+)/ aliases: # Shallow Git Clone - &shallow-git-clone-and-enable-vnc @@ -124,7 +117,6 @@ workflows: - check-pr-tag - prep-deps - get-changed-files-with-git-diff: - <<: *exclude_develop_master_rc requires: - prep-deps - test-deps-audit: @@ -505,6 +497,7 @@ jobs: - run: sudo corepack enable - attach_workspace: at: . + - gh/install - run: name: Get changed files with git diff command: npx tsx .circleci/scripts/git-diff-develop.ts diff --git a/.circleci/scripts/git-diff-develop.ts b/.circleci/scripts/git-diff-develop.ts index bca1bdeec143..3cf5022d4e12 100644 --- a/.circleci/scripts/git-diff-develop.ts +++ b/.circleci/scripts/git-diff-develop.ts @@ -6,6 +6,26 @@ import { promisify } from 'util'; const exec = promisify(execCallback); +const MAIN_BRANCH = 'develop'; + +/** + * Get the target branch for the given pull request. + * + * @returns The name of the branch targeted by the PR. + */ +async function getBaseRef(): Promise { + if (!process.env.CIRCLE_PULL_REQUEST) { + return null; + } + + // We're referencing the CIRCLE_PULL_REQUEST environment variable within the script rather than + // passing it in because this makes it easier to use Bash parameter expansion to extract the + // PR number from the URL. + const result = await exec(`gh pr view --json baseRefName "\${CIRCLE_PULL_REQUEST##*/}" --jq '.baseRefName'`); + const baseRef = result.stdout.trim(); + return baseRef; +} + /** * Fetches the git repository with a specified depth. * @@ -76,14 +96,31 @@ async function gitDiff(): Promise { */ async function storeGitDiffOutput() { try { - console.log("Attempting to get git diff..."); - const diffOutput = await gitDiff(); - console.log(diffOutput); - // Create the directory + // This is done first because our CirleCI config requires that this directory is present, + // even if we want to skip this step. const outputDir = 'changed-files'; fs.mkdirSync(outputDir, { recursive: true }); + console.log(`Determining whether this run is for a PR targetting ${MAIN_BRANCH}`) + if (!process.env.CIRCLE_PULL_REQUEST) { + console.log("Not a PR, skipping git diff"); + return; + } + + const baseRef = await getBaseRef(); + if (baseRef === null) { + console.log("Not a PR, skipping git diff"); + return; + } else if (baseRef !== MAIN_BRANCH) { + console.log(`This is for a PR targeting '${baseRef}', skipping git diff`); + return; + } + + console.log("Attempting to get git diff..."); + const diffOutput = await gitDiff(); + console.log(diffOutput); + // Store the output of git diff const outputPath = path.resolve(outputDir, 'changed-files.txt'); fs.writeFileSync(outputPath, diffOutput.trim());