Skip to content

Commit c4c3e4e

Browse files
[tool] Fix filter-packages-to when everything is changed (#5182)
`filter-packages-to` didn't correctly the handle the case where the set of target packages is empty, meaning that all packages should be tested. This broke it for cases such as changing a CI configuration file, making the filter not take effect.
1 parent 24654d3 commit c4c3e4e

File tree

2 files changed

+94
-12
lines changed

2 files changed

+94
-12
lines changed

script/tool/lib/src/common/package_command.dart

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -436,16 +436,26 @@ abstract class PackageCommand extends Command<void> {
436436
packages = <String>{currentPackageName};
437437
}
438438

439-
Set<String> excludedPackageNames = getExcludedPackageNames();
440-
441-
final Set<String> filter =
442-
_expandYamlInPackageList(getStringListArg(_filterPackagesArg));
443-
if (filter.isNotEmpty) {
444-
final List<String> sortedList = filter.toList()..sort();
439+
final Set<String> excludedPackageNames = getExcludedPackageNames();
440+
final bool hasFilter = argResults?.wasParsed(_filterPackagesArg) ?? false;
441+
final Set<String>? excludeAllButPackageNames = hasFilter
442+
? _expandYamlInPackageList(getStringListArg(_filterPackagesArg))
443+
: null;
444+
if (excludeAllButPackageNames != null &&
445+
excludeAllButPackageNames.isNotEmpty) {
446+
final List<String> sortedList = excludeAllButPackageNames.toList()
447+
..sort();
445448
print('--$_filterPackagesArg is excluding packages that are not '
446449
'included in: ${sortedList.join(',')}');
447-
excludedPackageNames =
448-
excludedPackageNames.union(packages.difference(filter));
450+
}
451+
// Returns true if a package that could be identified by any of
452+
// `possibleNames` should be excluded.
453+
bool isExcluded(Set<String> possibleNames) {
454+
if (excludedPackageNames.intersection(possibleNames).isNotEmpty) {
455+
return true;
456+
}
457+
return excludeAllButPackageNames != null &&
458+
excludeAllButPackageNames.intersection(possibleNames).isEmpty;
449459
}
450460

451461
for (final Directory dir in <Directory>[
@@ -459,7 +469,7 @@ abstract class PackageCommand extends Command<void> {
459469
if (packages.isEmpty || packages.contains(p.basename(entity.path))) {
460470
yield PackageEnumerationEntry(
461471
RepositoryPackage(entity as Directory),
462-
excluded: excludedPackageNames.contains(entity.basename));
472+
excluded: isExcluded(<String>{entity.basename}));
463473
}
464474
} else if (entity is Directory) {
465475
// Look for Dart packages under this top-level directory; this is the
@@ -481,9 +491,7 @@ abstract class PackageCommand extends Command<void> {
481491
packages.intersection(possibleMatches).isNotEmpty) {
482492
yield PackageEnumerationEntry(
483493
RepositoryPackage(subdir as Directory),
484-
excluded: excludedPackageNames
485-
.intersection(possibleMatches)
486-
.isNotEmpty);
494+
excluded: isExcluded(possibleMatches));
487495
}
488496
}
489497
}

script/tool/test/common/package_command_test.dart

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,6 +825,80 @@ packages/plugin3/plugin3.dart
825825

826826
expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
827827
});
828+
829+
test(
830+
'honors --filter-packages-to flag when a file is changed that makes '
831+
'all packages potentially changed', () async {
832+
processRunner.mockProcessesForExecutable['git-diff'] =
833+
<FakeProcessInfo>[
834+
FakeProcessInfo(MockProcess(stdout: '''
835+
.ci.yaml
836+
''')),
837+
];
838+
final RepositoryPackage plugin1 =
839+
createFakePlugin('plugin1', packagesDir.childDirectory('plugin1'));
840+
createFakePlugin('plugin2', packagesDir);
841+
createFakePlugin('plugin3', packagesDir);
842+
await runCapturingPrint(runner, <String>[
843+
'sample',
844+
'--filter-packages-to=plugin1',
845+
'--base-sha=main',
846+
'--run-on-changed-packages'
847+
]);
848+
849+
expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
850+
});
851+
852+
test('--filter-packages-to handles federated plugin groups', () async {
853+
processRunner.mockProcessesForExecutable['git-diff'] =
854+
<FakeProcessInfo>[
855+
FakeProcessInfo(MockProcess(stdout: '''
856+
packages/a_plugin/a_plugin/lib/foo.dart
857+
packages/a_plugin/a_plugin_impl/lib/foo.dart
858+
packages/a_plugin/a_plugin_platform_interface/lib/foo.dart
859+
''')),
860+
];
861+
final Directory groupDir = packagesDir.childDirectory('a_plugin');
862+
final RepositoryPackage plugin1 =
863+
createFakePlugin('a_plugin', groupDir);
864+
final RepositoryPackage plugin2 =
865+
createFakePlugin('a_plugin_impl', groupDir);
866+
final RepositoryPackage plugin3 =
867+
createFakePlugin('a_plugin_platform_interface', groupDir);
868+
await runCapturingPrint(runner, <String>[
869+
'sample',
870+
'--filter-packages-to=a_plugin',
871+
'--base-sha=main',
872+
'--run-on-changed-packages'
873+
]);
874+
875+
expect(
876+
command.plugins,
877+
unorderedEquals(
878+
<String>[plugin1.path, plugin2.path, plugin3.path]));
879+
});
880+
881+
test('--filter-packages-to and --exclude work together', () async {
882+
processRunner.mockProcessesForExecutable['git-diff'] =
883+
<FakeProcessInfo>[
884+
FakeProcessInfo(MockProcess(stdout: '''
885+
.ci.yaml
886+
''')),
887+
];
888+
final RepositoryPackage plugin1 =
889+
createFakePlugin('plugin1', packagesDir.childDirectory('plugin1'));
890+
createFakePlugin('plugin2', packagesDir);
891+
createFakePlugin('plugin3', packagesDir);
892+
await runCapturingPrint(runner, <String>[
893+
'sample',
894+
'--filter-packages-to=plugin1,plugin2',
895+
'--exclude=plugin2',
896+
'--base-sha=main',
897+
'--run-on-changed-packages'
898+
]);
899+
900+
expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
901+
});
828902
});
829903

830904
group('test run-on-dirty-packages', () {

0 commit comments

Comments
 (0)