Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: exclude running git diff job for the e2e quality gate in develop, master and release branches #25605

Merged
merged 14 commits into from
Jul 2, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Jul 1, 2024

Description

This PR excludes running the git-diff script for the e2e quality gate added here, for develop as well as for master and release branches.
We don't want to run this in develop, master or release branch, since there is no point on re-running the new/changed tests there, as the changes are already in develop branch. This could add up more credits and also slow down ci in those branches.

Context: in develop branch the current quality gate fails, since the diffResult is empty, and we were throwing the following error as we were entering in the !diffResult condition.
However, this PR fixes the issue on the higher level, by skipping entirely the git diff for the mentioned branches above.

  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.');
  }

Open in GitHub Codespaces

Related issues

Fixes: current develop ci

Manual testing steps

  • ci should continue to work
  • once merged, fixes develop

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Copy link
Contributor

github-actions bot commented Jul 1, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@seaona seaona self-assigned this Jul 1, 2024
@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jul 1, 2024
@seaona seaona changed the title chore: ignore quality gate for develop, master and RC branches chore: fix quality gate for develop branch Jul 1, 2024
@seaona seaona removed the team-confirmations Push issues to confirmations team label Jul 1, 2024
@seaona seaona marked this pull request as ready for review July 1, 2024 11:43
@seaona seaona requested review from kumavis and a team as code owners July 1, 2024 11:43
@seaona seaona marked this pull request as draft July 1, 2024 12:30
@seaona seaona changed the title chore: fix quality gate for develop branch chore: exclude running git diff job for the e2e quality gate in develop, master and release branches Jul 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [3783ebf]
Page Load Metrics (250 ± 245 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint692681144019
domContentLoaded106029147
load431862250511245
domInteractive106029147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.60%. Comparing base (fe12ae4) to head (7d28ce5).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #25605   +/-   ##
========================================
  Coverage    69.60%   69.60%           
========================================
  Files         1364     1364           
  Lines        48172    48172           
  Branches     13291    13291           
========================================
  Hits         33526    33526           
  Misses       14646    14646           

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

@@ -20,7 +20,7 @@ async function readChangedFiles() {
return changedFiles;
} catch (error) {
console.error('Error reading from file:', error);
return '';
return [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning an empty array, in order to handle properly the subsequent filter, when there is no file (in develop, master and RC branch)

@@ -243,7 +252,6 @@ workflows:
- /^Version-v(\d+)[.](\d+)[.](\d+)/
requires:
- prep-build
- get-changed-files-with-git-diff
Copy link
Contributor Author

@seaona seaona Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed here, since the vault decrypt is a special test, that only runs on develop and RC branches, and only runs this single test (doesn't use the run-all script)

@seaona seaona marked this pull request as ready for review July 2, 2024 07:42
@seaona seaona requested a review from a team as a code owner July 2, 2024 07:42
@@ -243,7 +243,6 @@ workflows:
- /^Version-v(\d+)[.](\d+)[.](\d+)/
requires:
- prep-build
- get-changed-files-with-git-diff
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also remove get-changed-files-with-git-diff as a requirement for the test-e2e-firefox-flask and test-e2e-firefox-confirmation-redesign jobs, which also only run on develop/master/rc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a great point! Removed too 👍

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more suggested change from me

@@ -497,7 +495,15 @@ jobs:
at: .
- run:
name: Get changed files with git diff
command: npx tsx .circleci/scripts/git-diff-develop.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simpler and easier to maintain code, I think that instead of this code change to the command, we can do the following change above:

diff --git a/.circleci/config.yml b/.circleci/config.yml
index d9f0754335..9fe95a7f6b 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -106,6 +106,12 @@ workflows:
       - check-pr-tag
       - prep-deps
       - get-changed-files-with-git-diff:
+          filters:
+            branches:
+              ignore:
+                - develop
+                - master
+                - /^Version-v(\d+)[.](\d+)[.](\d+)/
           requires:
             - prep-deps
       - test-deps-audit:

And the other jobs that depend on get-changed-files-with-git-diff should still run, see the docs on that points: "Note: When jobs in the current workflow that are listed as dependencies are not executed (due to a filter function for example), their requirement as a dependency for other jobs will be ignored by the requires option. However, if all dependencies of a job are filtered, then that job will not be executed either." https://circleci.com/docs/configuration-reference/#requires

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wooa that's what I was looking for! I fully agree with this approach. I initially had it like this, but couldn't find anywhere if the jobs requiring the skipped job would succeed. Pushing the changes now. Thank you!!

Copy link

sonarcloud bot commented Jul 2, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [7d28ce5]
Page Load Metrics (166 ± 205 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint753001146230
domContentLoaded108925168
load402022166427205
domInteractive98825168
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@seaona seaona merged commit 4886a3f into develop Jul 2, 2024
76 checks passed
@seaona seaona deleted the chore-quality-gate-ignore-branches branch July 2, 2024 12:18
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2024
@metamaskbot metamaskbot added the release-12.2.0 Issue or pull request that will be included in release 12.2.0 label Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.2.0 Issue or pull request that will be included in release 12.2.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants