Skip to content

Commit

Permalink
ci: Prevent E2E timeouts on release changes (#26846)
Browse files Browse the repository at this point in the history
## **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=#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.
  • Loading branch information
Gudahtt authored Sep 3, 2024
1 parent 74dc3e7 commit 594d580
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 12 deletions.
9 changes: 1 addition & 8 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
45 changes: 41 additions & 4 deletions .circleci/scripts/git-diff-develop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string | null> {
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.
*
Expand Down Expand Up @@ -76,14 +96,31 @@ async function gitDiff(): Promise<string> {
*/
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());
Expand Down

0 comments on commit 594d580

Please sign in to comment.