Branch coverage #361
Branch coverage #361
Conversation
|
I need to handle the case where the VM is too old and the branch coverage source report isn't available. Something analogous to how we check if reportLines is available in collect.dart. If it's not available, I'll need to report a warning to the user. I'll also need to disable all those failing tests for the v2.14.0 bot that's failing. |
Done |
| * Add flag `--pretty-print-branch` to format_coverage that works | ||
| similarly to pretty print, but outputs branch level coverage, rather than | ||
| line level. | ||
| * Update `--lcov` (abbr `-l`) in format_coverage to output branch level |
There was a problem hiding this comment.
I don't think so. I'm adding significant functionality, but not changing existing functionality, so I think that makes it a minor version bump.
There was a problem hiding this comment.
Does any of our internal testing infrastructure consume lcov reports? Will they be broken with the change in format? If not, then it's probably fine to model as a non breaking releases.
There was a problem hiding this comment.
The lcov output was also modified when I added function coverage, and that didn't seem to be an issue, so I guess not.
|
Should we consider bumping the min SDK constraint in the pubspec? |
Hmmm. Not sure about that. We'd be bumping to a version that isn't actually released yet. The latest version of the VM is only required for branch coverage. If most users don't use branch coverage, then it seems unnecessary to me. |
test/collect_coverage_api_test.dart
Outdated
| return map; | ||
| }); | ||
|
|
||
| if (platformVersionCheck(2, 17)) { |
There was a problem hiding this comment.
how about making it a separate test and add skip: !platformVersionCheck(2, 17)? Tests should ideally have only 1 reason to fail.
go/unit-testing-practices#behavior-testing
If it's infeasible to split to separate tests, the expect call also has a skip argument.
test/collect_coverage_api_test.dart
Outdated
|
|
||
| if (platformVersionCheck(2, 17)) { | ||
| // Dart VM versions before 2.17 don't support branch coverage. | ||
| for (var sampleCoverageData in sources[_sampleAppFileUri]!) { |
There was a problem hiding this comment.
it would be better to avoid for loops in the tests. If this fails the message wont give much information.
We could do something like
expect(sources[_sampleAppFileUri], everyElement(containsPair('branchHits', isNotEmpty)));
test/collect_coverage_test.dart
Outdated
| @@ -179,6 +179,11 @@ void main() { | |||
| 28: 'fooAsync', | |||
| 38: 'isolateTask' | |||
| }); | |||
| if (platformVersionCheck(2, 17)) { | |||
There was a problem hiding this comment.
As above, can this be a separate test, or can you pass skip: !platformVersionCheck(2, 17) to the expect call?
* WIP: branch coverage * Pretty printing of branch coverage * WIP: testing * Finish testing * Changelog and pubspec * Warn user if their VM is too old to support branch coverage * Update readme to mention function and branch coverage * Fix some of the failing tests on older VM versions * Fix run_and_collect_test * Actually fix run_and_collect_test * Use skips instead of ifs in tests
Implement branch coverage, using the new branch coverage source report from the VM service API.
To collect branch coverage information, pass
--branch-coverageto both collect_coverage and the Dart command you're gathering coverage from. Branch coverage information can be pretty printed or included in lcov info. You'll also need a very recent version of the VM (service API v3.56, Dart VM v2.17).Fixes dart-lang/tools#575