Facilitate code coverage in test runner

[travissanderson-wf]

I'm sure you guys are thinking about this but I didn't see an open issue to talk about it. The alpha runner looks great but would like to see the ability to generate test coverage from it by providing a remote debugging port (or some other more elegant solution!).

[zoechi]

👍

[kevmoo]

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

[Sharom]

👍

[donny-dont]

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

[nex3]

#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.

[avoivo]

@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?

[nex3]

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.

[avoivo]

@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

[nex3]

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.

[avoivo]

@nex3 ok thanx a lot for the info

[eseidel]

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.

[eseidelGoogle]

(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.

[kevmoo]

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.

[nex3]

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.

[eseidelGoogle]

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.

[nex3]

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.

[daniel-v]

Any updates on this?

[eseidelGoogle]

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?

[joeconwaystk]

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.

[robbecker-wf]

Thanks for checking @grouma.

[grouma]

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.

[smaifullerton-wk]

Any update on this issue @grouma ?

[grouma]

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.

[willdrach-wk]

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

[grouma]

Fantastic. Happy to help out where needed here.

…

On Wed, Sep 18, 2019 at 9:36 AM willdrach-wk ***@***.***> wrote: FYI @grouma <https://github.com/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 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36?email_source=notifications&email_token=AAOBNBK4YPU4MKTMI7HD4Q3QKJKHBA5CNFSM4A5XLJYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7AVWEA#issuecomment-532765456>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOBNBMBLKKUGZVKXRDUYADQKJKHBANCNFSM4A5XLJYA> .

[willdrach-wk]

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.

[grouma]

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.

[willdrach-wk]

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!

[olesiathoms-wk]

@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.

[grouma]

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

[willdrach-wk]

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

[eseidelGoogle]

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

[zanderso]

/cc @jonahwilliams

[jonahwilliams]

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.

[willdrach-wk]

@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?

[grouma]

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.

[willdrach-wk]

@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.