Skip to content
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(unstable): deno test --coverage #6901

Merged
merged 69 commits into from
Sep 13, 2020

Conversation

caspervonb
Copy link
Contributor

@caspervonb caspervonb commented Jul 27, 2020

This adds a coverage option to the test command which prints a summary report of lines covered to stdout.

Closes #106

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2020

CLA assistant check
All committers have signed the CLA.

@buttercubz
Copy link

@caspervonb Would it be something similar to coverage in jest?

@caspervonb
Copy link
Contributor Author

@caspervonb Would it be something similar to coverage in jest?

Not sure what jest does but this collects block based call coverage directly from the runtime.

Output formats to be determined but lcov is fairly obvious as it's a ubiquitous format.

cli/flags.rs Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/coverage.rs Outdated
Comment on lines 17 to 18
// TODO(caspervonb) do not hard-code message ids.
// TODO yield then await each command response after sending the request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to implement something like inspector agent that can automatically track proper ids

Copy link
Contributor Author

@caspervonb caspervonb Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need anything particularly fancy for this case, just increment by one per message is fine.

Long term for the REPL etc yeah we'll definitively want a full blown inspector client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but then you need to receive messages from inspector for each sent message and I believe inspector can send other messages as well which should be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but then you need to receive messages from inspector for each sent message

The two TODO's tie together somewhat, e.g each socket send will loop awaiting the next response, breaking when a response with the sent id is received then error checking the result.

I believe inspector can send other messages as well which should be ignored.

It can.

Enabling a domain (e.g calling the Runtime.enable method) will enable event notifications for that domain. Events do not carry identifiers however so this doesn't interfere with the above.

cli/main.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

test coverage:
async/mux_async_iterator.ts 97.101%
async/deferred.ts 100.000%
io/writers.ts 100.000%
io/ioutil.ts 97.619%
io/util.ts 100.000%
io/streams.ts 100.000%
io/_iotest.ts 88.679%
fs/exists.ts 93.548%
encoding/toml.ts 98.995%
encoding/base64url.ts 100.000%
encoding/csv.ts 97.382%
encoding/binary.ts 83.083%

Very nice 👍

This already gets called later on in execute
@caspervonb
Copy link
Contributor Author

So this works, needs to be cleaned up a bit this approach will work.

However, it turns out tracefile sections (lcov format) can start with the test name.

A tracefile is made up of several human-readable lines of text, divided into sections. If available, a tracefile begins with the testname which is stored in the following format:

TN:<test name>

And we can't get at the test name since the test runner is implemented in JS; If we were to integrate the coverage collector directly in the test runner on the however we could support this.

I think implementing it in JS would be the cleaner implementation in the long run, for one the threading issue just goes away. Other things implemented in JS like the REPL are also in need of an inspector client for things like tab completion so it's somewhat inevitable that we'll end up with an internal inspector client in JS someday.

🤔

@bartlomieju
Copy link
Member

So this works, needs to be cleaned up a bit this approach will work.

However, it turns out tracefile sections (lcov format) can start with the test name.

A tracefile is made up of several human-readable lines of text, divided into sections. If available, a tracefile begins with the testname which is stored in the following format:
TN:

And we can't get at the test name since the test runner is implemented in JS; If we were to integrate the coverage collector directly in the test runner on the however we could support this.

I think implementing it in JS would be the cleaner implementation in the long run, for one the threading issue just goes away. Other things implemented in JS like the REPL are also in need of an inspector client for things like tab completion so it's somewhat inevitable that we'll end up with an internal inspector client in JS someday.

🤔

@caspervonb thanks for detailed description. Inspector client in JS is a desirable feat, but I think we could land this PR only with terminal output for the first pass. Could the report be made a bit more detailed, eg. showing distinct percentages for branches and statements?

@caspervonb
Copy link
Contributor Author

Could the report be made a bit more detailed, eg. showing distinct percentages for branches and statements?

Hmm, I'll clean this up first but it's doable, just have to walk the AST checking if node ranges are inside covered ranges to to calculate that.

@caspervonb

This comment has been minimized.

@bartlomieju bartlomieju added this to the 1.4.0 milestone Aug 10, 2020
@bartlomieju
Copy link
Member

@caspervonb could you rebase the branch? I'll look into the problems you described above.

@caspervonb

This comment has been minimized.

@caspervonb caspervonb force-pushed the feat-test-coverage branch 3 times, most recently from 9efba46 to 05b47d7 Compare August 24, 2020 19:38
@caspervonb
Copy link
Contributor Author

We shouldn't start the inspector web(socket)server when collecting coverage only.

Agreed; I was considering making the host argument of DenoInspector::new an Option and only register with the web server when it is Some as a quick and dirty hack pending a refactor (a lot, if not all of this can be generalised into a general purpose inspector client).

--coverage shouldn't require --inspect.

Agreed.

Nice that you managed to implement your own inspector::Channel. I see that it uses a mpsc to communicate, which suggests that multiple isolates in different threads share the same channel. Is this true?

Thought I had to thread it initially but turned out to be fine to just let it run on the same thread as the main isolate so this could probably just be a queue.

cli/inspector.rs Outdated Show resolved Hide resolved
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Sep 9, 2020
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Sep 9, 2020
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Sep 9, 2020
@bartlomieju bartlomieju changed the title feat(cli): add coverage option to the test command feat(test): add --coverage flag Sep 11, 2020
@bartlomieju bartlomieju changed the title feat(test): add --coverage flag feat(unstable): Support --coverage flag in deno test Sep 11, 2020
@bartlomieju bartlomieju changed the title feat(unstable): Support --coverage flag in deno test feat(unstable): deno test --coverage Sep 11, 2020
@caspervonb
Copy link
Contributor Author

caspervonb commented Sep 12, 2020

Thanks @bartlomieju for merging things into CoverageCollector which more or less what I had in mind.

I think this is good enough to land for 1.4, still things to be desired but I'll follow up on hardening the inspector session and separating inspector web socket server from the inspector along with lcov and json reporters etc for ~1.5 or ~1.6.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @caspervonb this is a massive feature! Looking forward to future iterations and other output formats support.

@bartlomieju bartlomieju merged commit 755cfa9 into denoland:master Sep 13, 2020
@antoniogamiz
Copy link

Where can I see the docs for this feature?

@bartlomieju
Copy link
Member

@antoniogamiz
Copy link

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature-request: first-class test-coverage support
8 participants