ci: Prevent E2E timeouts on release changes by Gudahtt · Pull Request #26846 · MetaMask/metamask-extension

Gudahtt · 2024-09-02T19:57:05Z

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.

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

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

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.

sonarqubecloud · 2024-09-02T20:19:04Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov · 2024-09-02T20:37:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.12%. Comparing base (e239d85) to head (2a83a56).
Report is 7 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #26846   +/-   ##
========================================
  Coverage    70.12%   70.12%           
========================================
  Files         1422     1422           
  Lines        49526    49526           
  Branches     13863    13863           
========================================
  Hits         34730    34730           
  Misses       14796    14796

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-09-02T20:48:26Z

Builds ready [2a83a56]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1811 ± 101 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	266	2369	1746	400	192
		domContentLoaded	1538	2361	1789	208	100
		load	1578	2371	1811	210	101
		domInteractive	13	85	40	18	9

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

desi

So the gist is that rather than exclude master and RC we only target 'develop' correct?

.circleci/scripts/git-diff-develop.ts

Gudahtt · 2024-09-03T11:53:48Z

So the gist is that rather than exclude master and RC we only target 'develop' correct?

Previously we would run this "get changed tests" script for all branches except develop, master, and RCs. This would include branches targeting an RC, or branches targeting other feature branches, where we don't really want the quality gate to apply (the quality gate was just meant to stop flaky tests from landing in develop, that's it).

Now we decide whether this script applies based upon the PR target / base ref, rather than the branch name itself. If a branch is targeting develop, we want this script to run (resulting in extra test runs for changed tests, to help prevent flakiness).

danjm

Good catch! Solution looks good.

seaona

LGTM! Very nice catch and improvement 😍 thank you!

Gudahtt · 2024-09-03T12:30:06Z

.circleci/scripts/git-diff-develop.ts

+  // 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'`);


I tried using the CIRCLE_PR_NUMBER environment variable first, but it doesn't appear to be present for our workflows for some reason, despite being documented.

Sadly there is no way to reference the base ref directly from CircleCI either. It's a highly requested feature apparently, but is not supported. But this works fine as a workaround.

metamaskbot · 2024-09-03T13:08:51Z

Missing release label release-12.2.0 on PR. Adding release label release-12.2.0 on PR and removing other release labels(release-12.6.0), as PR was cherry-picked in branch 12.2.0.

Gudahtt added the team-extension-platform Extension Platform team label Sep 2, 2024

github-actions bot added the team-wallet-framework label Sep 2, 2024

Gudahtt force-pushed the prevent-unwanted-e2e-quality-gate branch from d9d3bf9 to 062e76c Compare September 2, 2024 20:05

Gudahtt force-pushed the prevent-unwanted-e2e-quality-gate branch from 062e76c to 2a83a56 Compare September 2, 2024 20:17

Gudahtt marked this pull request as ready for review September 2, 2024 20:25

Gudahtt requested review from a team and kumavis as code owners September 2, 2024 20:25

desi reviewed Sep 2, 2024

View reviewed changes

.circleci/scripts/git-diff-develop.ts Show resolved Hide resolved

danjm approved these changes Sep 3, 2024

View reviewed changes

Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 3, 2024

seaona approved these changes Sep 3, 2024

View reviewed changes

Gudahtt commented Sep 3, 2024

View reviewed changes

Gudahtt merged commit 594d580 into develop Sep 3, 2024

Gudahtt deleted the prevent-unwanted-e2e-quality-gate branch September 3, 2024 12:30

github-actions bot locked and limited conversation to collaborators Sep 3, 2024

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 3, 2024

metamaskbot added release-12.6.0 Issue or pull request that will be included in release 12.6.0 release-12.2.0 Issue or pull request that will be included in release 12.2.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Prevent E2E timeouts on release changes#26846

ci: Prevent E2E timeouts on release changes#26846
Gudahtt merged 1 commit intodevelopfrom
prevent-unwanted-e2e-quality-gate

Gudahtt commented Sep 2, 2024 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Sep 2, 2024

Uh oh!

codecov bot commented Sep 2, 2024 •

edited

Loading

Uh oh!

metamaskbot commented Sep 2, 2024

Uh oh!

desi left a comment

Uh oh!

Uh oh!

Gudahtt commented Sep 3, 2024

Uh oh!

danjm left a comment

Uh oh!

seaona left a comment

Uh oh!

Gudahtt Sep 3, 2024

Uh oh!

metamaskbot commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Gudahtt commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

sonarqubecloud bot commented Sep 2, 2024

Quality Gate passed

Uh oh!

codecov bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Sep 2, 2024

Uh oh!

desi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gudahtt commented Sep 3, 2024

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

seaona left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Gudahtt commented Sep 2, 2024 •

edited

Loading

codecov bot commented Sep 2, 2024 •

edited

Loading