-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: adds quality gate for rerunning e2e spec files that are new or have been modified #24556
base: develop
Are you sure you want to change the base?
Conversation
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. |
@@ -212,12 +213,26 @@ async function main() { | |||
|
|||
console.log('My test list:', myTestList); | |||
|
|||
const changedOrNewTests = await fetchChangedE2eFiles(); | |||
console.log('Spec files that will be re-run:', changedOrNewTests); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is left for debugging purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to sacrifice some parallel execution speed by doing it this way. It also only has to be done if it's running on CircleCI, and not locally.
It would be a better approach if at the top of function runningOnCircleCI()
, you looked at testPaths
and then duplicated 5 times each thing that was also in changedOrNewTests
.
This would allow it to distribute the workload evenly across the VMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for your suggestion @HowardBraham That's definetly a good point! I was thinking it would be negligible given the small amount of retries, but it's true that we can benefit from parallelization witha small tweak. I'll add the changes 🙇♀️
…logic is working properly in failing tests
if (retryUntilFailure) { | ||
return null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before it assumed that if we reach the max retries, it was an error and throw the error with rejection message 'Retry limit reached'
, but in the case of using retryUntilFailure, reaching the max retry limit it's actually a good thing, as it means the test has not failed. That's why in this case I'm returning just null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this parameter stopAfterOneFailure
for better clarity?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24556 +/- ##
========================================
Coverage 67.40% 67.40%
========================================
Files 1289 1289
Lines 50248 50248
Branches 13011 13011
========================================
Hits 33865 33865
Misses 16383 16383 ☔ View full report in Codecov by Sentry. |
Builds ready [71bbf50]
Page Load Metrics (623 ± 464 ms)
Bundle size diffs
|
test/e2e/fetch-changed-files.js
Outdated
@@ -0,0 +1,33 @@ | |||
const axios = require('axios'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but we might be able to avoid a fetch to github here, and instead just use git. Not sure if we have the repo and git history in the right time and place on CI to do it, but it would be good if we could: more efficient than a network request, and not dependent on network conditions or the github api possibly being down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you Dan! That is a good point. I have an alternative branch where I explored a bit the git ci option, however it was not straight-forward and I believe we might need to tweak some things on the config.yml file in order to accomplish it. I decided to go for this option to not over-engineer it, but for the reason you mention it might be worth to try the ci option a bit further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't try but something like this might work?
gitDiff = await git.diffSummary(['--name-only', `origin/develop...${process.env.CIRCLE_SHA1}`, 'test/**/*.spec.*s']);
https://github.com/MetaMask/metamask-extension/blob/develop/development/generate-rc-commits.js#L2
https://github.com/steveukx/git-js?tab=readme-ov-file#git-diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the local git command like that work if you're doing a shallow checkout? Also, the way this is written, it will be hitting the GitHub API hundreds of times per workflow (because there are hundreds of parallel machines running the workflow). Perhaps do it in prep-deps
, and then persist_to_workspace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on the shallow checkout. Could prob be addressed by a clever enough git fetch
invocation (possibly in prep-deps
)..? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the suggestions 🙏 ❤️ In the shallow clone we do this:
git clone --depth 1 --no-checkout "$CIRCLE_REPOSITORY_URL" .
git fetch --depth 1 origin "$CIRCLE_SHA1"
I was thinking we would need to do git fetch develop explicitly to get available the develop branch commits in the ci environment? 🤔 I'm also not sure if we would need to increase the depth too here 🤔 I can investigate this further
const retryIndex = args.indexOf('--retries'); | ||
if (retryIndex !== -1) { | ||
args.splice(retryIndex, 2); | ||
} | ||
|
||
const extraArgs = isTestChangedOrNew | ||
? ['--retry-until-failure', `--retries=${retriesForChangedOrNewTests}`] | ||
: []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for this duct-taped workaround complexity. Look at line 191 in this file. Just put retries
into extraArgs
down here instead of args
up there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if you go with my other suggestion about runningOnCircleCI
though, you can probably remove most of this anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!! if we follow the approach of copying the spec files x5 times on runningOnCircleCI
then we can indeed remove all of this logic. Just something I'm wondering, would it be okay then to treat each of those new specs the same as rest (same flags)? or we still want them to fail immediately with no-retries?
Given that @legobeat has a PR for reducing test retries, maybe it's fine to treat all the tests equally; then there's no need to add any extra flag, and just copy the spec file x times in the test list from runningOnCiircleCi, and this would simplify a lot the logic
What do you think?
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24787?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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.
echo "$DIFF_RESULT" | ||
|
||
# Store the output of git diff | ||
git diff --name-only develop..."$CIRCLE_SHA1" >> changed-files/changed-files.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be a diff with origin/develop
? because above there is the code to git fetch origin develop
, but those commits are not pulled locally
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:
file.filename.startsWith('test/e2e/') &&
file.filename.endsWith('.spec.js') || file.filename.endsWith('.spec.ts')
Since we already had a flag which could support the re-running successful tests,
--retry-until-failure
I just leveraged this into thefor
loop for each test, and if that testcase was identified as new/modified, the flag is added together with the new retries number (in this case 5)Note: I am not adding any new job specific for this task. The problem of doing so is that, either:
That's why the approach of using the same existings jobs was taken.
This has been discussed together with @chloeYue .
Related issues
Fixes: #24009
Manual testing steps
Screenshots/Recordings
personal-sign-runx5-times.mp4
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81343/workflows/55ac5a22-b195-404d-951f-529af37100f8
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81343/workflows/55ac5a22-b195-404d-951f-529af37100f8
Spec files that will be re-run
is emptyPre-merge author checklist
Pre-merge reviewer checklist