-
Notifications
You must be signed in to change notification settings - Fork 44
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
Track coverage using C8 #96
Conversation
hey @johnhooks what do you think? we can merge this one? few lines involved but in that way we can track the new tests coverage |
@@ -21,6 +21,7 @@ | |||
"lint": "eslint src/*.js test/*.js", | |||
"test-generate-mo": "msgfmt test/fixtures/latin13.po -o test/fixtures/latin13.mo & msgfmt test/fixtures/utf8.po -o test/fixtures/utf8.mo & msgfmt test/fixtures/obsolete.po -o test/fixtures/obsolete.mo", | |||
"test": "mocha", | |||
"test:coverage": "npx c8 --check-coverage npm run test", |
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 not add c8
as a dev dependency?
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.
because I would not want to add more dependencies, the ideal thing for this repo would be to switch to the node's bundled test suite , which already has everything we need without installing nothing
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.e. I don't think it is really limiting and since its use is limited to certain cases it can speed up the installation by not having it in the package.json. and then I am of the opinion that less is better
in any case if you think this will cause problems I will include it in the json package, alternatively I would proceed with the merge on the development branch so that we can know the coverage for each new PR
I noticed an issue here... the tests are fired twice because the |
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.
Should fix the tests that runs twice
This PR adds the coverage for tests
long story short, since the most used with mocha is nyc I thought it would work whereas since it is esm you have to use c8
istanbuljs/nyc#1287 (comment)