-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
feat: coverage #1899
feat: coverage #1899
Conversation
@rishabh3112 Can you rebase, I think we should merge it |
Rebasing. |
Done, but tests on windows never end 😞 . Running for 3hr+ https://github.com/webpack/webpack-cli/runs/1346351375 |
@rishabh3112 let's try to disable watch test |
Okay, on it. |
Something weird, all test passed (including watch and serve tests) on windows but |
I think it is old bug |
That did the trick 👍🏻 |
Great, ping me when it will be green I will merge it and fix configuration |
ping @evilebottnawi |
Thanks, I think we need look how configuration codecov for github actions, because not it is not showing in checks |
Anyway let's do it in separate PR |
@@ -4,7 +4,7 @@ const { stat } = require('fs'); | |||
const { resolve } = require('path'); | |||
|
|||
describe('webpack cli', () => { | |||
it( | |||
it.skip( |
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.
why skip?
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.
Not working with nyc
🤷🏻♂️ , need a fix.
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.
I don't understand why we need nyc when jest has it inbuilt?
/cc @evilebottnawi
Maybe we should remove it and use jest --coverage
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.
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.
ahh spawned process mess, we need to get to this
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.
Yep spawned process
Did PR on here #2032 |
WIP
What kind of change does this PR introduce?
Feature, adds coverage to CI
Did you add tests for your changes?
No
If relevant, did you update the documentation?
No
Summary
Attempt on adding coverage
Does this PR introduce a breaking change?
No
Other information
Part of Roadmap