Skip to content

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

Merged
bugzpodder merged 3 commits into
react:masterfrom
stipsan:patch-1
Apr 2, 2019
Merged

Remove --coverage + --watch workaround for the test command#4176
bugzpodder merged 3 commits into
react:masterfrom
stipsan:patch-1

Conversation

@stipsan

@stipsanstipsan commented Mar 18, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown

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

@stipsan

stipsan commented May 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@TimerTimer added this to the 2.0.0 milestone Jun 1, 2018
@TimerTimer modified the milestones: 2.0.x, 2.xSep 26, 2018
@TimerTimer removed this from the 2.x milestone Nov 2, 2018
@stale

staleBot commented Dec 2, 2018

Copy link
Copy Markdown

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.

@stalestaleBot added the stale label Dec 2, 2018
@stalestaleBot removed the stale label Dec 3, 2018
@netlify

netlifyBot commented Dec 3, 2018

Copy link
Copy Markdown

Deploy preview for gallant-davinci-8f9bd9 ready!

Built with commit 6497d66

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

@stipsan

Copy link
Copy Markdown
ContributorAuthor

@bugzpodder looks like CRA is on 23+ now? 🙂

@bugzpodder

Copy link
Copy Markdown

Yep, I think so!

@stale

staleBot commented Jan 2, 2019

Copy link
Copy Markdown

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.

@stalestaleBot added the stale label Jan 2, 2019
@stipsan

Copy link
Copy Markdown
ContributorAuthor

@bugzpodder then we could probably merge this 😄

@stalestaleBot removed the stale label Jan 2, 2019
@mrmckeb
mrmckeb self-requested a review February 1, 2019 07:44
@mrmckebmrmckeb self-assigned this Feb 1, 2019
@mrmckeb

Copy link
Copy Markdown
Contributor

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

bugzpodder commented Feb 1, 2019

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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.

@mrmckebmrmckeb mentioned this pull request Feb 3, 2019
@ianschmitzianschmitz added this to the 3.0 milestone Feb 7, 2019
@mrmckeb

Copy link
Copy Markdown
Contributor

@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 react:masterApr 2, 2019
@locklockBot locked and limited conversation to collaborators Apr 7, 2019
@stipsan
stipsan deleted the patch-1 branch July 1, 2019 11:27
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@stipsan@bugzpodder@mrmckeb@iansu@Timer@ianschmitz@facebook-github-bot