-
-
Notifications
You must be signed in to change notification settings - Fork 679
Enhancement: Generate test coverage reports after npm test (#2289) #2294
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
Conversation
052a024 to
76d0c2e
Compare
package.json
Outdated
| "coverage": "ASC_FEATURES=\"*\" c8 npm run test:coverage", | ||
| "test": "npm run test:parser && npm run test:compiler -- --parallel && npm run test:browser && npm run test:asconfig && npm run test:transform", | ||
| "test:coverage": "npm run test:parser && npm run test:compiler && npm run test:browser && npm run test:asconfig && npm run test:transform", |
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.
Looks like the test:coverage step can be included in coverage as it isn't very useful on its own. Even better perhaps, if the test step could detect that it runs under c8 (looking at process.argv or .env perhaps) and then disables parallel execution - so the test step can be reused? Then we'd have
"coverage": "ASC_FEATURES=\"*\" c8 npm test",
A minor concern could be that sometimes, ASC_FEATURES can enable things that are not yet fully functioning, so setting it independently could make sense.
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.
Hi Dcode,
Considering the compatibility of Windows and POSIX system. I prefer the process.argv. currently the issue is, c8 need a something like zygote process process, so it cannot use && on its command line.
So I create coverage and test:coverage tasks, the test:coverage will have --coverage option for specific test test:compiler
The other option is we can create a test.js file and use it to control the environment variables and argument variables, with this option, we only need one coverage task.
What do you think?
Regards
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.
Toying around with c8 a little, I found that instead of having a separate --coverage option, we can detect that c8 is being used by checking process.env.NODE_V8_COVERAGE != null and then turn off parallel testing. Looks like this allows to reuse the test step?
(found this here)
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.
@dcodeIO
yes, we can use this option, I finished the modification
6e0a99d to
5cf3e68
Compare
tests/compiler.js
Outdated
|
|
||
| // Run tests in parallel if requested | ||
| if (args.parallel && coreCount > 2) { | ||
| if (args.parallel && coreCount > 2 && !args.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.
For example here:
| if (args.parallel && coreCount > 2 && !args.coverage) { | |
| const isCoverage = process.env.NODE_V8_COVERAGE != null; | |
| if (args.parallel && coreCount > 2 && !isCoverage) { |
|
The slightly modified PR above is based on this PR with a few slight changes so coverage remains light / optional for now. One would manually set |
…report generation
5cf3e68 to
836d548
Compare
|
Initial code coverage in the spirit of this PR has been added in #2517, with the difference that it is kept optional for now. Sorry for the strange co-authored-by in the commit message, I used the signed-off credentials from the first post that apparently link to a different account, hrm. |
Signed-off-by: Jesse xiang19890319@gmail.com
Issue details: #2289
Test report screen shot:
