[flutter_tools] Support coverage collection for dependencies #129513
Conversation
github-actions
bot
added
the
tool
p-mazhnik
force-pushed
the
coverage-packages
branch
from
bb89f47 to
959247d
Compare
p-mazhnik
force-pushed
the
coverage-packages
branch
from
959247d to
5b32237
Compare
| final Iterable<String> packagesToInclude; | ||
| if (packagesRegExps.isNotEmpty) { | ||
| final RegExp combinedRegExp = RegExp('(${packagesRegExps.join(")|(")})'); | ||
| packagesToInclude = packageConfig.packages |
There was a problem hiding this comment.
shouldn't we include projectName too?
There was a problem hiding this comment.
I believe packageConfig.packages include current package as well.
There was a problem hiding this comment.
but that's not guaranteed to match the provided regexes, right?
There was a problem hiding this comment.
Yes. I thought about coverage-package option as a generic option that defaults to the current package, so we can exclude current package from the report if we want to. But that is not necessary of course.
So if we wanna always include the current package, I can update the code. What do you think?
There was a problem hiding this comment.
Hmm, I see what you mean. At least to my mind, this is counter-intuitive behavior, because I assume you would always want to include the current package. Can you think of a use case when you would NOT want coverage of the current package? Otherwise this behavior would just mean users would always have to specify another two arguments to include the current package.
There was a problem hiding this comment.
Either way we go I think we should make it explicit in the help text.
There was a problem hiding this comment.
Can you think of a use case when you would NOT want coverage of the current package?
This is what comes to my mind:
For plugins/packages, we usually have an example app inside package (e.g. camera_android plugin), where most of the tests are located. When launching tests from the example app and collecting coverage, what we really want is to collect coverage for the package itself, and we don't really care (I think) about the code inside the example.
Anyway, I don't think it would be harmful to have coverage data for the example as well.
There was a problem hiding this comment.
I've updated PR to always include current package, but we can continue discussion
There was a problem hiding this comment.
sorry to flip flop, but per discussion in the discord, I think the way you originally had it, without the current package is probably better
There was a problem hiding this comment.
Current package is included only by default in the new changes
|
@liamappelbe could you take a look at this PR? |
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
The overall approach looks good, from the package:coverage perspective. Doesn't look like this is going to inadvertently do extra RPCs and slow things down, just change which files are included after coverage has already been gathered.
I agree with Chris's comments about the regexs tho.
Thanks Liam! I'll take over code review, just wanted to double check the approach was sound. |
| help: 'Where to store coverage information (if coverage is enabled).', | ||
| ) | ||
| ..addMultiOption('coverage-package', | ||
| help: 'A regular expression matching packages names ' |
There was a problem hiding this comment.
I think regular expressions here is surprising. Since this is a multi-option, why not just use multiple package names?
There was a problem hiding this comment.
As I wrote in the Discord, this is to simplify matching of the multiple packages and avoid editing ci/cd scripts for the coverage collection each time new package is introduced.
Additionally, with the current solution it is possible to report each package that was discovered during testing using the .* regular expression. Even though I personally can't think of a use case atm, but I don't understand why we have to limit this functionality.
There was a problem hiding this comment.
Ok, I agree, I think we probably need regexes here.
packages/flutter_tools/test/commands.shard/hermetic/test_test.dart
Outdated
Show resolved
Hide resolved
|
Re-running |
Manual roll requested by [email protected] flutter/flutter@f842ed9...6f09064 2023-07-17 [email protected] Stand-alone widget tree with multiple render trees to enable multi-view rendering (flutter/flutter#125003) 2023-07-17 [email protected] Update to valid build tools variant and update lockfiles (flutter/flutter#125825) 2023-07-17 [email protected] Roll Packages from 369ee7e to 6889cca (5 revisions) (flutter/flutter#130721) 2023-07-17 [email protected] [Reland] - Update `DialogTheme` tests for M2/M3 (flutter/flutter#130711) 2023-07-17 [email protected] Roll Flutter Engine from 683087731feb to e4cae43c9c7a (9 revisions) (flutter/flutter#130716) 2023-07-17 [email protected] [flutter_tools] Support coverage collection for dependencies (flutter/flutter#129513) 2023-07-17 [email protected] Fix `DatePicker` uses incorrect overlay color from `DatePickerTheme` and add missing tests (flutter/flutter#130584) 2023-07-17 [email protected] Update `DropdownMenu`, `SnackBarTheme` and `Stepper` tests for M2/M3 (flutter/flutter#130464) 2023-07-17 [email protected] Clarify the whole "CustomPainters default to Size.zero" thing. (flutter/flutter#130624) 2023-07-16 [email protected] Update list of CoC contacts. (flutter/flutter#130630) 2023-07-15 [email protected] Manual roll Flutter Engine from 403866d16137 to 683087731feb (16 revisions) (flutter/flutter#130666) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…#129513) PR provides a new option to the `test` command to include coverage info of specified packages. It helps collecting coverage info in test setups where test code lives in separate packages or for multi-package projects. At present, only current package is included to the final report. Usage: Consider an app with two packages: `app`, `common`. Some of the tests in `app` use (indirectly) code that is located in `common`. When running with `--coverage` flag, that code is not included in the coverage report by default. To include `common` package in report, we can run: ```sh flutter test --coverage --coverage-package app --coverage-package common ``` Note that `--coverage-package` accepts regular expression. Fixes flutter#79661 Fixes flutter#101486 Fixes flutter#93619
4 participants
PR provides a new option to the
testcommand to include coverage info of specified packages.It helps collecting coverage info in test setups where test code lives in separate packages or for multi-package projects.
At present, only current package is included to the final report.
Usage:
Consider an app with two packages:
app,common.Some of the tests in
appuse (indirectly) code that is located incommon. When running with--coverageflag, that code is not included in the coverage report by default. To includecommonpackage in report, we can run:flutter test --coverage --coverage-package app --coverage-package commonNote that
--coverage-packageaccepts regular expression.Fixes #79661
Fixes #101486
Fixes #93619
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.