[flutter_plugin_tools] Track and log exclusions #4205
Conversation
Makes commands that use the package-looping base command track and report exclusions. This will make it much easier to debug/audit situations where tests aren't running when expected (e.g., when enabling a new type of test for a package that previously had to be explicitly excluded from that test to avoid failing for having no tests, but forgetting to remove the package from the exclusion list). Also fixes a latent issue with using different exclusion lists on different commands in a single CI task when using sharding could cause unexpected failures due to different sets of plugins being included for each step (e.g., build+drive with an exclude list on drive could potentially try to drive a plugin that hadn't been built in that shard) by sharding before filtering out excluded packages. Adds testing for sharding in general, as there was previously none.
|
@gaaclarke Ping? |
There was a problem hiding this comment.
LGTM, there are a handful of nits, probably the biggest feedback is it looks like we are working around the lack of a concept of the Package where the word "package" can mean either a string name, a Directory, or a PackageEnumerationEntry depending on the situation.
| @@ -15,6 +15,19 @@ import 'core.dart'; | |||
| import 'git_version_finder.dart'; | |||
| import 'process_runner.dart'; | |||
|
|
|||
| /// An entry in pacakge enumeration for APIs that need to include extra | |||
| @@ -15,6 +15,19 @@ import 'core.dart'; | |||
| import 'git_version_finder.dart'; | |||
| import 'process_runner.dart'; | |||
|
|
|||
| /// An entry in pacakge enumeration for APIs that need to include extra | |||
| /// data about the entry. | |||
| class PackageEnumerationEntry { | |||
There was a problem hiding this comment.
It's a bit odd that this abstraction name includes the context that it is used. Couldn't this just be a Package?
There was a problem hiding this comment.
This is public throughout the entire codebase, which would make calling it just "Package" confusing; most references to packages in the tool are not enumerations, just individual entries.
The reason the context is part of the name is that the extra data is specific to enumeration—it's what it is excluded from. It's indicating that even though the package is in the enumeration, it shouldn't really be handled as part of it. (Originally I wanted to separate excluded packages out, since having them in the list is inherently somewhat confusing, but that doesn't allow for solving the sharding problem—their place in the list must be preserved.)
| /// 3./4. Either of the above, but in a third_party/packages/ directory that | ||
| /// is a sibling of the packages directory. This is used for a small number | ||
| /// of packages in the flutter/packages repository. | ||
| Stream<Directory> _getAllPlugins() async* { | ||
| Stream<PackageEnumerationEntry> _getAllPackages() async* { |
There was a problem hiding this comment.
Here we are calling PackageEnumerationEntrys "packages" fwiw.
There was a problem hiding this comment.
I felt like the type being PackageEnumerationEntry in the context of a method whose sole purpose is to enumerate packages was straightforward enough that the ambiguity wouldn't be an issue (especially given that we are following Flutter style, so use explicit types everywhere).
| // getPackages, as the current naming is very confusing. | ||
| Stream<Directory> getPlugins() async* { | ||
| /// Returns the set of plugins to exclude based on the `--exclude` argument. | ||
| Set<String> _getExcludedPackages() { |
There was a problem hiding this comment.
getExcludedPackagePaths? getExcludedPackageNames?
There was a problem hiding this comment.
Yep, changed to getExcludedPackageNames. ("name" is actually itself a bit of a weird concept currently in the tool code currently, due to the retro-fitting of federated packages that happened at some point; I would like to someday go back and clean that up. But it's definitely better than just "package".)
| @@ -185,6 +185,23 @@ void main() { | |||
| package.childDirectory('example').path, | |||
| ])); | |||
| }); | |||
|
|
|||
| test('excludes subpackages when main package is excluded', () async { | |||
| createFakePlugin('a_plugin', packagesDir, | |||
There was a problem hiding this comment.
IMO this would be slightly more clear if you retained a reference to a_plugin and asserted that it didn't show up in the output. Not capturing the output to createFakePlugin makes it a bit less clear where the side effects are.
Makes commands that use the package-looping base command track and report exclusions. This will make it much easier to debug/audit situations where tests aren't running when expected (e.g., when enabling a new type of test for a package that previously had to be explicitly excluded from that test to avoid failing for having no tests, but forgetting to remove the package from the exclusion list). Also fixes a latent issue with using different exclusion lists on different commands in a single CI task when using sharding could cause unexpected failures due to different sets of plugins being included for each step (e.g., build+drive with an exclude list on drive could potentially try to drive a plugin that hadn't been built in that shard) by sharding before filtering out excluded packages. Adds testing for sharding in general, as there was previously none.
Makes commands that use the package-looping base command track and report exclusions. This will make it much easier to debug/audit situations where tests aren't running when expected (e.g., when enabling a new type of test for a package that previously had to be explicitly excluded from that test to avoid failing for having no tests, but forgetting to remove the package from the exclusion list). Also fixes a latent issue with using different exclusion lists on different commands in a single CI task when using sharding could cause unexpected failures due to different sets of plugins being included for each step (e.g., build+drive with an exclude list on drive could potentially try to drive a plugin that hadn't been built in that shard) by sharding before filtering out excluded packages. Adds testing for sharding in general, as there was previously none.
Makes commands that use the package-looping base command track and
report exclusions. This will make it much easier to debug/audit
situations where tests aren't running when expected (e.g., when enabling
a new type of test for a package that previously had to be explicitly
excluded from that test to avoid failing for having no tests, but
forgetting to remove the package from the exclusion list).
Also fixes a latent issue with using different exclusion lists on
different commands in a single CI task when using sharding could cause
unexpected failures due to different sets of plugins being included for
each step (e.g., build+drive with an exclude list on drive could
potentially try to drive a plugin that hadn't been built in that shard)
by sharding before filtering out excluded packages.
Adds testing for sharding in general, as there was previously none.
Pre-launch Checklist
dart format.)[shared_preferences]///).