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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove --coverage + --watch workaround for the test command #4176

Merged
merged 3 commits into from Apr 2, 2019
Merged

Remove --coverage + --watch workaround for the test command #4176

merged 3 commits into from Apr 2, 2019

Conversation

stipsan
Copy link
Contributor

@stipsan stipsan commented Mar 18, 2018

The workaround that removes --watch if --coverage is present in the test command will no longer be needed when jestjs/jest#5601 is merged and released.

# Do not merge this yet

I will update the PR once it's part of a stable jest release 馃槃

@bugzpodder
Copy link

@stipsan looks like jest@23 would have this change?

@stipsan
Copy link
Contributor Author

stipsan commented May 25, 2018

@bugzpodder correct! Once this project is upgraded to jest 23 then this PR can be merged 馃槃

@Timer Timer added this to the 2.0.0 milestone Jun 1, 2018
@Timer Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018
@Timer Timer removed this from the 2.x milestone Nov 2, 2018
@stale
Copy link

stale bot commented Dec 2, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 2, 2018
@stale stale bot removed the stale label Dec 3, 2018
@netlify
Copy link

netlify bot commented Dec 3, 2018

Deploy preview for gallant-davinci-8f9bd9 ready!

Built with commit 6497d66

https://deploy-preview-4176--gallant-davinci-8f9bd9.netlify.com

@stipsan
Copy link
Contributor Author

stipsan commented Dec 3, 2018

@bugzpodder looks like CRA is on 23+ now? 馃檪

@bugzpodder
Copy link

Yep, I think so!

@stale
Copy link

stale bot commented Jan 2, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 2, 2019
@stipsan
Copy link
Contributor Author

stipsan commented Jan 2, 2019

@bugzpodder then we could probably merge this 馃槃

@stale stale bot removed the stale label Jan 2, 2019
@mrmckeb mrmckeb self-requested a review February 1, 2019 07:44
@mrmckeb mrmckeb self-assigned this Feb 1, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 1, 2019

Perhaps I misunderstood, but when I ran this locally with test --coverage, I was still in watch mode - which would be somewhat of a breaking change for many users.

Can you give me a little more info about what this PR is trying to achieve?

@bugzpodder
Copy link

bugzpodder commented Feb 1, 2019

Sorry I was away and racked up 300 notifications from this repo :)
@mrmckeb, traditionally jest didn't work with --coverage and --watch in the same command until @stipsan fixed it: jestjs/jest#5601
But this wasn't introduced until jest@23 was released as part of CRA.

I tested this after jest@23 with --findRelatedTests and --coverage but it still broke occasionally so I had to disable this particular set of flags in our CI build. But I imagine this would happen rarely and I am ok with seeing this PR go through. I'll let you guys deal with the potential breaking change part.

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 3, 2019

OK, so we've just merged in a change to allow no-watch, which I think will help with this #6285 - but it's definitely a breaking change, so I don't think it can be in a minor release.

@stipsan can you please rebase? We'll need to wait for #6278 to merge first.

@mrmckeb mrmckeb mentioned this pull request Feb 3, 2019
@ianschmitz ianschmitz added this to the 3.0 milestone Feb 7, 2019
@iansu iansu added this to In progress in v3 Mar 10, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 20, 2019

@stipsan, if you could rebase this one (there's a small conflict), we can merge this in for 3.0! Thanks again.

@bugzpodder bugzpodder merged commit 9514cb8 into facebook:master Apr 2, 2019
v3 automation moved this from In progress to Done Apr 2, 2019
@lock lock bot locked and limited conversation to collaborators Apr 7, 2019
@stipsan stipsan deleted the patch-1 branch July 1, 2019 11:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants