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.

Sign up for GitHub

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

Jump to bottom

Fix test retries that contain snapshots #8019

Closed
wants to merge 1 commit into from
Closed

Fix test retries that contain snapshots #8019

wants to merge 1 commit into from

Conversation

palmerj3
Copy link
Contributor

@palmerj3 palmerj3 commented Mar 1, 2019 •
edited by thymikee

Summary

WIP DO NOT MERGE

This is an exploratory PR to solve the problem of snapshot data being written multiple times when tests are retried.

Fixes #7493
Closes #8015

Test plan

Nothing at the moment aside from the existing test suite. Collecting feedback with this PR on the approach.

thymikee
thymikee requested changes Mar 18, 2019
View reviewed changes
Copy link
Collaborator

@thymikee thymikee left a comment •
edited

Choose a reason for hiding this comment

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

I'm down with this approach :)

packages/jest-circus/src/eventHandler.ts
event.test.errors = [];

// Clear any snapshot data that occurred in previous test run
const tmpSnapshotState = global.expect.getState().snapshotState;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense for global.expect to expose clearState, call it here and let it do the job of cleaning up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree. I should have some time this week to look at this some more.

@thymikee
Copy link
Collaborator

Feel free to adapt testing approach from #8015

@itajaja itajaja mentioned this pull request Mar 18, 2019
Closed
@itajaja
Copy link

itajaja commented Mar 18, 2019 •
edited

is this really the right fix? if I have tests A and B in test.js, A passes but B doesn't, only B will be retried. but in your implementation you are actually resetting the state from scratch, disregarding the snapshot results of A.

The right solution would be to reset the snapshot not from scratch, but from after A (before B)

@palmerj3
Copy link
Contributor Author

@itajaja this is a work in progress PR as the heading makes clear. I'll be taking a look at this again this week.

@itajaja
Copy link

itajaja commented Mar 18, 2019

sounds good. thanks for looking into this 👍

@DeDuckProject
Copy link

Any updates about this?

@rogeliog
Copy link
Contributor

Hi @palmerj3 👋! Do we have an update on this by any chance? I'm also happy to take over this PR if needed.

rogeliog
rogeliog reviewed Jun 28, 2019
View reviewed changes
packages/jest-circus/src/eventHandler.ts

if (tmpSnapshotState) {
if (
tmpSnapshotState._snapshotPath &&
Copy link
Contributor

@rogeliog rogeliog Jun 28, 2019 •
edited by SimenB

Choose a reason for hiding this comment

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

I took the freedom to start working on finishing this PR (but let me know if you prefer me not to), I've made some good progress but one thing that is still not clear is: Why do we need to call fs.unlinkSync(tmpSnapshotState._snapshotPath);?

cc: @thymikee @palmerj3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Please feel free to work on it :) It's all of our code not my code.

Ultimately the issue this PR tries to fix is the fact that a snapshot file for a test will contain n-copies depending on how many times it's rerun. So this was one way we could fix that by removing the snapshot file before new data is written.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! I'll keep working on it and update the PR once I have something 😄

@rogeliog rogeliog mentioned this pull request Jul 1, 2019
Merged
@rogeliog
Copy link
Contributor

rogeliog commented Jul 5, 2019

Closed by #8629

@rogeliog rogeliog closed this Jul 5, 2019
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rogeliog rogeliog rogeliog left review comments

@thymikee thymikee thymikee requested changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jest.retryTimes() gets confused with snapshots
6 participants
@palmerj3 @thymikee @itajaja @DeDuckProject @rogeliog @facebook-github-bot