-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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. |
test/e2e/run-all.js
Outdated
@@ -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 🙇♀️
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.
changes added 👍
…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?
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.
changed 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24556 +/- ##
========================================
Coverage 65.70% 65.70%
========================================
Files 1369 1369
Lines 54366 54366
Branches 14149 14149
========================================
Hits 35718 35718
Misses 18648 18648 ☔ 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
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.
logic for git fetch with depth and git diff has been added
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
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 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.
ℹ️ the problem was that the branch was way behind the depth. Updating the branch fixed it
} | ||
|
||
async function gitDiffWithRetry() { | ||
const depths = [5, 10, 15, 20, 30, 40, 50]; |
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.
to consume fewer resources, we start with small depth jumps +5 and later on we increment with +10 jumps
console.error('An error occurred:', error.message); | ||
process.exit(1); | ||
} | ||
} |
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.
leaving all the logs so we are able to debug ci if needed
Builds ready [c3dbacf]
Page Load Metrics (49 ± 3 ms)
Bundle size diffs
|
I have reviewed the latest version and LGTM. I think it would be worthwhile to tag @danjm, @HowardBraham, and @legobeat to give another review as you have commented. Let's try to proceed with this PR. Thanks all! |
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:
git diff
and using these 2 filters:file.filename.startsWith('test/e2e/') &&
file.filename.endsWith('.spec.js') || file.filename.endsWith('.spec.ts')
--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 so the new tests fail fast without retrialsIncremental git fetch depth within shallow clone
We use git fetch with incremental depth as @danjm suggested. The ci environment uses a shallow clone, meaning we won't be able to succeed just by using git diff as it won't find the merge base. For fixing that, we start with a git fetch depth of 5, and keep incrementing the depth it the error is
no merge base
up until 50. Beyond 50, the job will fail and the PR should Update branch, so we are able to do the git diff successfully. The assumption here is that if the branch is that behind, they would need to update their branch anyway possibly due to conflicts with develop.Related issues
Fixes: #24009
Manual testing steps
Screenshots/Recordings
Git diff with incremental git fetch
Example on how this works: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/85809/workflows/1d7a4658-1dd9-460f-9fd4-518858982329/jobs/3115301
=============================================== [UPDATE with the new code changes]
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/85823/workflows/d5de5e43-48b4-4b98-b2f8-863e7e30e39e/jobs/3116106/parallel-runs/7?filterBy=ALL
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/85823/workflows/d5de5e43-48b4-4b98-b2f8-863e7e30e39e/jobs/3116106/parallel-runs/7?filterBy=ALL
reruns-old-no-reruns-new.mp4
Note: since we are using parallelization, this means that the same test new/edit spec can be placed in different buckets, so if it fails, it can fail in different buckets at the same time.
Pre-merge author checklist
Pre-merge reviewer checklist