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

Conversation

@stipsan
Copy link
Contributor

commented Mar 18, 2018

The workaround that removes --watch if --coverage is present in the test command will no longer be needed when facebook/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

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2018

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

@stipsan

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

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

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2018

Yep, I think so!

@stale

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

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 Feb 1, 2019

@mrmckeb mrmckeb self-assigned this Feb 1, 2019

@mrmckeb

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

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: facebook/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

This comment has been minimized.

Copy link
Collaborator

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 referenced 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

This comment has been minimized.

Copy link
Collaborator

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.

@amyrlam amyrlam force-pushed the facebook:master branch from cecd762 to d3b19f9 Mar 24, 2019

@bugzpodder bugzpodder merged commit 9514cb8 into facebook:master Apr 2, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details

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 stipsan:patch-1 branch Jul 1, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can鈥檛 perform that action at this time.