Facilitate code coverage in test runner about test HOT 51 CLOSED

FYI for the thread: code coverage collection is now implemented for Dart VM based tests! See the README for more details: https://pub.dev/packages/test#collecting-code-coverage

from test.

I agree that the best approach here is for test to be involved in coverage collecting—that's the best way to ensure a smooth user interface and to avoid dependencies on test's internal structure, which is not a guaranteed stable interface.

from test.

Any updates on this?

from test.

In case it's useful, flutter's test wrapper does coverage collection:
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/test.dart
No clue if it would be implemented the same way in pub run test itself.

from test.

Thanks for the replies everyone. I'll try to circle back on this once v2 work is done.

from test.

FYI @grouma and everyone else:

The Dart platform team here at Workiva is facilitating our switch over to Dart 2, and coverage is a requirement for many of our teams to be confident in the switch. As such, we're looking at speccing out what implementing coverage within this package for VM and Chrome based tests involves. As of right now this includes:

• Implementing takePreciseCoverage and startPreciseCoverage from the Chrome Profiler API in dwds
• Implementing coverage reports within the test runner
• Formatting those coverage reports from multiple runs into a single lcov (or similar) report

from test.

(Helping @nilsdoehring work around the pub publish tar bug) I just published dart_coveralls 0.5.0 which works with latest Dart.

It's unclear to me what the right division between package:test and package:coverage and package:dart_coveralls is. Right now you have to use dart_coveralls report path/to/tests_all.dart to run and collect the coverage, but it only takes a single path arg requires you to use this tests_all.dart pattern.

pub run test has a much smarter crawler, but no way to collect coverage.

It seems like the right split would be to add a --colect_coverage=/into/path argument to package:test runners, which collected/dumped the VM's json format for then other things (like dart_coveralls) to consume. Presumably it would dump a separate .json for each test it ran and it would be the job of something like dart_coverage (using package:coverage) to merge those hitmaps together, etc. That would let package:test be in the find-and-run business w/o dart_coveralls needing to replicate that. And leave package:coverage to only handle dealing with coverage files themselves.

Thoughts from others on what the right divisions are here? I'm happy to help hack together some of the necessary patches in my (very limited) hacking time.

from test.

I'm interested if this is still being pursued. Perhaps with the JS and CSS coverage bits in Chrome devtools now, coverage could be extracted from that via the remote debugging protocol?

from test.

Supporting coverage through the test runner was intended to be my summer intern's project. Unfortunately their internship was delayed so this work was never picked up. It's still on our radar however. We are making great progress on package:dwds which will do most of the heavy lifting for us. It's just a matter of bandwidth at this point.

from test.

Great. Two questions:

1. Is coverage a VM-only feature? or are there other configurations of package:test which need to be coverage-aware?
2. Re: VM -- do you see conflicts between the current run-tests-in-a-same-process-isolate and collecting coverage? i.e. to support coverage in the current package:test would we need to add VM pause-on-exit-but-just-these-isolates support?

None of this is high priority, but with answers to the above, I might try to write up a patch to add coverage support to package:test one of these weekends in my free-hacking time.

from test.

To my knowledge nobody is working on this.

from test.

I provided coverage support internally for Google with Node.js, Istanbul and remap-istanbul. I think we can do something similar here, especially now that we leverage Node, but as @kevmoo pointed out it is not a priority.

from test.

Any update on this issue? I would be happy to contribute.

from test.

I see some action on https://bugs.chromium.org/p/chromium/issues/detail?id=717195#c20 does that mean coverage collection can continue here soon?

from test.

Having the linked bug resolved would certainly make the final solution easier, especially if Chrome properly handles source maps in the export. That being said there is a pretty straightforward alternative today. I would be more than happy to review PRs but don't have the bandwidth to implement it myself.

The alternative approach would make use of the webkit inspection protocol package. Through this package we can enable precise coverage and collect the result. Using the source maps package we should be able to map the JS coverage results back to Dart. Internally we do something similar with remap istanbul. I made a prototype of this solution quite a while back and things mostly worked as expected. I had some difficulty remapping the coverage results but I'll be the first to admit that my knowledge of source maps is quite limited.

from test.

from test.

There's a good amount of work that needs to be done here, but the good news is that through my research I have found it to be... doable!

VM Coverage

VM coverage can be gathered through the dart-lang/coverage package, it just needs to be integrated into the test package. This is sitting behind #1082 however, as the VM needs to be run in debug mode in order to gather that coverage, from what I could see. The dart-lang/coverage package will allow us to gather and format coverage into lcov, so it's pretty much a turnkey solution.

Chrome Coverage

For Chrome coverage it gets a little more complicated, we need to integrate some APIs for working with the Chrome "Profiler" protocol (which is what allows us to gather coverage). That's described in dart-lang/webdev#674. Once that's done, it's a much more involved process:

Concerns with DWDS:

• DWDS, from what I understand, isn't used outside of webdev
• We would likely have to make another change in DWDS to open up a public API for interacting with this Profiler (you can see my extremely hacky solution to this here)

Concerns with Chrome's coverage report

• This coverage would need to be sourcemapped (dwds provides APIs for this, but I wasn't able to get them working properly when I was reasearching)
• Chrome reports coverage in its own format, and that would need to be converted to something readable, either the format that dart-lang/coverage uses, or lcov

General concerns

My last concern comes with combining the coverage reports. The easiest solution would be to just to spit out multiple lcov reports, considering that the separate reports might be useful ("was this test covered in VM but not Browser?") and lcov has utilities for combining reports: https://wiki.documentfoundation.org/Development/Lcov#Combine_lcov_tracefiles

However, I'm far from an expert on that front, so I'd be happy if anyone has strong opinions about the output of a potential test package coverage flag!

Updates

That's all I've got for today. This is definitely something that's on our radar, but is not prioritized or categorized at the moment. I can update when I have more news or code ready.

from test.

👍

from test.

May be blocked on https://code.google.com/p/dart/issues/detail?id=21791 – we're actively investigating

from test.

👍

from test.

@nex3 any updates on this? The blocker @kevmoo mentioned is closed.

from test.

#295 will provide access to the Observatory port for Dartium and content shell tests when run with --pause-after-load, which should make it possible to hook up to the coverage package. Doing the same on the VM is blocked on #50, which is in turn blocked on dart-lang/sdk#23320.

All of that will require manually hooking up the coverage collector to Observatory. I'd like to support something more automatic, but that's not going to happen for a while.

from test.

@nex3 The thing is that i also need to pause the test run after all tests executed in order to collect the complete coverage. Is there any way to achieve that?

from test.

You should be able to talk to observatory and tell it to do that, either by setting some sort of pause-before-exit flag or by adding a breakpoint somewhere.

from test.

@nex Thanx for the reply. --pause-before-exit sounds good but i cannot find a way to pass that argument to the VM.

Also using 'dart:developer' library does not stop on breakpoint when running the tests.

The way i am running the tests is the following

pub run test:test --pub-serve=<port> -p dartium --pause-after-load

from test.

Thanx for the reply. --pause-before-exit sounds good but i cannot find a way to pass that argument to the VM.

You can't pass flags directly to the VM from pub run, and besides you'd probably want to pause Dartium's VM rather than the VM running the test. You'd have to set this via Observatory, although I'm not sure what the UI for that is.

Also using 'dart:developer' library does not stop on breakpoint when running the tests.

That might be dart-lang/sdk#25369.

from test.

@nex3 ok thanx a lot for the info

from test.

It seems like the right split would be to add a --colect_coverage=/into/path argument to package:test runners, which collected/dumped the VM's json format for then other things (like dart_coveralls) to consume.

That sounds about right. pub run now runs tasks within isolates (not subprocesses) so one could (in theory) turn on observatory when running tests -- but then something needs to be watching isolates on shutdown to grab coverage info and close them down.

@nex3 should really comment here.

from test.

Is coverage a VM-only feature? or are there other configurations of package:test which need to be coverage-aware?

Probably yeah. If anyone knows of a way to get nice coverage reporting out of Chrome that would be rad, but the VM is the main focus.

Re: VM -- do you see conflicts between the current run-tests-in-a-same-process-isolate and collecting coverage? i.e. to support coverage in the current package:test would we need to add VM pause-on-exit-but-just-these-isolates support?

No, this should work fine. We can connect to the VM service which will be able to provide us fine-grained information for exactly the isolates we care about.

from test.

I've not had much time to look, sorry. What would help me is understanding:

1. Are existing examples in the test runner of connecting to the VM service protocol?
2. Does the VM runner run the tests in the same process (just different isolates) as the main test process?
3. If so, are there examples of dart programs connecting to their own service protocol I can crib from?

from test.

Just wanted to quickly chime in on question #2 and one of your previous comments - there was one package/suggestion out there (which I have since forgotten) that ran all of the test files on the same isolate. This creates an issue testing anything that modifies a static variable, since it carries to the next set of tests. Tests may then behave differently depending on the order their enclosing files are executed in.

from test.

Are existing examples in the test runner of connecting to the VM service protocol?

We do on the vm-debug branch (which is where you'd be working)—see this code.

Does the VM runner run the tests in the same process (just different isolates) as the main test process?

Yes. However, Flutter's runner does not. The coverage support should be flexible enough to support both modes. I don't think this will be super hard, though—each isolate reports its Observatory URL separately anyway.

If so, are there examples of dart programs connecting to their own service protocol I can crib from?

See the code link above. You may even be able to re-use the existing VM service connection.

from test.

Sadly, what @matanlurey said. We'd love to work on this, but we're crazy focused on v2 stuff.

from test.

We would like to provide coverage support by making use of the fairly new coverage feature of Chrome. I made a proof of concept a few weeks back. To enable it would require some non trivial work around our source maps. We were hoping to start building out some of that infrastructure this quarter but things are likely going to slip given all the immediate work required for Dart 2.

from test.

More progress from Chrome is certainly helpful. No promises on our side, though.

from test.

So far it doesn't look like sourcemapping is applied in the export. I'm able to get the coverage of the compiled JS but not the original dart files. That being said, in Chrome stable dev tools it does show the gutter coverage indicators on the original dart code. It's just not wired up to the export ability. @grouma
Could you talk to the right people on the Chrome team to make that happen (especially via the protocol) ?

Image shows sourcemapped coverage data is available in the dev tools

from test.

I'll reach out to the Chrome team to see the status of this feature.

from test.

I was able to reach out to the Chrome team.

I was told that all coverage information displayed in the DevTools is collected through the remote debugging protocol. There are no plans to export mapped coverage data from Chrome. One of the reasons for this is that the actual mapping of JS coverage information to source code is done in the front end of DevTools.

From what I gather, the solution I outlined above is basically what Chrome DevTools is doing today.

from test.

Thanks for checking @grouma.

from test.

cc @vsmenon

No worries. Note that this issue is definitely on our radar and has overlap with some of our other efforts. I'm hopeful we can address it in the not to distant future.

from test.

Any update on this issue @grouma ?

from test.

DWDS, from what I understand, isn't used outside of webdev

Nope. It's used by the Flutter CLI and soon from some internal tools. It's intended to be consumed outside of webdev.

We would likely have to make another change in DWDS to open up a public API for interacting with this Profiler

Not necessarily. We can alternatively expose this functionality through the Dart VM Service protocol. At the end of the day that's what package:coverage is going to consume. We can do something like what we did for screenshots and hot reloads.

This coverage would need to be sourcemapped (dwds provides APIs for this, but I wasn't able to get them working properly when I was reasearching)

What issues were you running into? If I recall correctly Chrome provides ranges which are covered within a JS file. Those ranges have to be converted to JS line numbers. Then we should be able to translate that into Dart line numbers using the linked logic.

Chrome reports coverage in its own format, and that would need to be converted to something readable, either the format that dart-lang/coverage uses, or lcov

See above comment. Basically we want to implement this in package:dwds and it should allow us to get lcov for free with package:coverage.

My last concern comes with combining the coverage reports. The easiest solution would be to just to spit out multiple lcov reports, considering that the separate reports might be useful ("was this test covered in VM but not Browser?") and lcov has utilities for combining reports: https://wiki.documentfoundation.org/Development/Lcov#Combine_lcov_tracefiles

I think that's the best first approach. We've run into several issues with trying to merge VM and Browser coverage internally. It's difficult to get both tools to consider the exact same Dart lines for coverage. When different lines are considered for coverage you end up with invalid merged reports.

from test.

It's intended to be consumed outside of webdev.

Nice, concern addressed!

We can alternatively expose this functionality through the Dart VM Service protocol.

Good idea, I can look into this more as it gets closer to a reality

What issues were you running into?

Mostly just ran out of time here, I believe I just wasn't passing the correct asset params in, so I was getting an empty sourcemap back.

I think that's the best first approach.

Nice, another concern addressed!

from test.

@grouma I have question regarding your comment

Not necessarily. We can alternatively expose this functionality through the Dart VM Service protocol. At the end of the day that's what package:coverage is going to consume. We can do something like what we did for screenshots and hot reloads.

Is there an example somewhere where you actually using screenshot/hot reload. We are having hard time finding the way to trigger from the test package.

from test.

Take a look at the screenshot test:
https://github.com/dart-lang/webdev/blob/master/dwds/test/screenshot_test.dart#L21

I also think the Flutter CLI delegates to this in some form but I'm not super familiar with the code:
https://github.com/flutter/flutter/blob/e2340c641db23a1e97a6f6e83af809944ff5fe34/packages/flutter_tools/lib/src/commands/screenshot.dart

from test.

Neat! FYI @zanderso in case flutter test should eventually share this logic.

from test.

/cc @jonahwilliams

from test.

We can definitely get rid of the custom coverage script for measuring tool coverage with this! I'll see if we can also reuse it for flutter coverage too.

from test.

@grouma I've been looking a bit more into gathering browser based coverage, and it seems that interacting with dwds might be a bit overkill for our needs. My main concern is that dwds and test share a fundamentally different method of compilation and serving assets, dwds making use of build runner and test only making use of build runner in explicit circumstances. This causes some problems when setting up those VM service protocols:

• DWDS functionality can't be instantiated via the main dwds class because we don't have the required BuildResult list
• The DWDS VM client can't be initialized without changes, mainly building an asset handler for the test assets

It does seem doable to initialize the VM client in that way (creating a new AssetHandler implementation and passing that in), but I'm starting to get worried that adapting dwds to test and vice versa might be too opaque, and unnecessary.

It appears that test has its own sourcemap logic for mapping stack traces that we could tap into, and tapping into the Chrome DevTools protocol could be as simple as starting up a package:webkit_inspection_protocol WipConnection and dropping that into either the platform or environment configurations.

Would you be open to a non-dwds implementation of coverage?

from test.

DWDS functionality can't be instantiated via the main dwds class because we don't have the required BuildResult list

BuildResults are only used for the hot-reload / auto refresh functionality of package:dwds. For testing purposes I think this stream can be empty without any concern.

The DWDS VM client can't be initialized without changes, mainly building an asset handler for the test assets

Yup. We'll need to provide an asset handler but that should be fairly straightforward. It likely can be a simple proxy server or even somehow delegate to the existing test handler. This is how we integrate the package with some internal tools.

It appears that test has its own sourcemap logic for mapping stack traces that we could tap into, and tapping into the Chrome DevTools protocol could be as simple as starting up a package:webkit_inspection_protocol WipConnection and dropping that into either the platform or environment configurations.

Would you be open to a non-dwds implementation of coverage?

I'm open to a non-dwds implementation but I'm cautious of supporting duplicate logic in package:test, package:dwds and package:coverage.

I think we need to first nail down if this coverage feature will support both Dart2JS and DDC. If it's only the later than using dwds probably makes sense because we must use build_runner for DDC tests anyway.

Note that even internally we don't support coverage for Dart2JS. Our JS instrumentation approach to coverage does not work well with the tree shaken output of Dart2JS.

If you feel that we should support Dart2JS coverage than a non-dwds approach likely makes sense. We'll have to be thoughtful of how we share the logic to support the various use cases.

from test.

@grouma this is all good information. I'll take a few steps further down the dwds path and see what I can get going.

As far as Dart2JS goes, I don't have any strong opinions one way or another. The one example I can provide is that we did have a push to run our unit tests in Dart2JS for a little while and it would have been more CI overhead to run a coverage task on top of the normal unit test run, but that's not relevant anymore as we run in both DDC and Dart2JS now. Just an anecdote to why it might be nice to support it.

from test.