-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix test retries that contain snapshots #8629
Conversation
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.
Looks good! I left small nits to address
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.
Looks great!!
event.test.errors = []; | ||
// Clear any snapshot data that occurred in previous test run | ||
global.expect.clearState(); |
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 forces expect
to be available, while circus is so far not tightly coupled to it (and in theory built to be used anywhere, the legacy-code
directory aside). Can we make it gracefully handle expect
/clearState
not being available and move it to a separate event handler?
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 a great point, you are right we should find to prevent jest-circus
of being coupled with expect
.
@@ -131,6 +132,8 @@ export const initialize = ({ | |||
}); | |||
setState({snapshotState, testPath}); | |||
|
|||
addEventHandler(handleSnapshotStateAfterRetry(snapshotState)); |
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.
oooh, I like this! good call
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 looks way better than I last saw it :D
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This PR is heavily inspired by @palmerj3's PR #8019 and most of the credit goes to him
This should make snapshots play better with
retryTimes
. I tried to add e2e tests for as much as I could. I don't know if I'm missing some use cases, would love some input on that.Fixes #7493
Closes #8015
Test plan
Added some e2e tests.