Tracking issue: stabilization of test runner code coverage #53924
Comments
cjihrig
added
the
feature request
github-project-automation
bot
moved this to Awaiting Triage
in Node.js feature requests
|
cc @MoLow @atlowChemi @benjamingr if you have other things to add to the list. |
RedYetiDev
added
coverage
This commit updates the test runner's code coverage so that coverage options are explicitly passed in instead of pulled from command line options. PR-URL: #53931 Refs: #53924 Refs: #53867 Refs: #53866 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit updates the test runner's code coverage so that coverage options are explicitly passed in instead of pulled from command line options. PR-URL: #53931 Refs: #53924 Refs: #53867 Refs: #53866 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit updates the test runner's code coverage so that coverage options are explicitly passed in instead of pulled from command line options. PR-URL: #53931 Refs: #53924 Refs: #53867 Refs: #53866 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #53937 Fixes: #53867 Refs: #53924 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Okay, coverage is now supported via the |
PR-URL: #53937 Fixes: #53867 Refs: #53924 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
@cjihrig You closed this as completed, is code coverage ready to be stablized? We don't have statement coverage yet tho |
|
I am tentatively planning to mark it as stable for v24. There is a performance issue that needs to be investigated more thoroughly, and maybe something related to source maps, but I don't remember off the top of my head. IMO statement coverage is a nice to have, but realistically I don't foresee it ever being added without some help from V8 because I don't believe we should add the overhead of reparsing all of the code with something like acorn. Plus my understanding is that C8 doesn't do it either, and exactly one person has ever brought it up that I'm aware of. Plus stable really just means that something can be reliably used and follows semver. |
IMHO source maps shouldn't be a priority, it doesn't have to marked stable at the same time, it's still hidden behind
Agreed with everything you said If you need any help stablizing, LMK. I'll look into some perf (and maybe add some benchmarks if I get a chance). |
PR-URL: nodejs#53937 Fixes: nodejs#53867 Refs: nodejs#53924 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
What is the problem this feature will solve?
This issue is for tracking remaining work to stabilize code coverage in the test runner. When this feature originally shipped, it had documented limitations. Those have all been resolved.
Note that stabilization does not prevent future semver-minor additions or bug fixes.
Here is the minimum list of things that I think need to be resolved:
run()API.lib/internal/test_runner/coverage.jsshould not callparseCommandLine(). The flags should be passed in. This is related to test_runner: do not read fromprocess.argvandprocess.cwd()in run() #53867.--experimental-test-coverageis replaced/aliased with--test-coverageWhat is the feature you are proposing to solve the problem?
Eventually stabilizing test runner code coverage
What alternatives have you considered?
No response
The text was updated successfully, but these errors were encountered: