Skip to content

Commit fdc1ec7

Browse files
stuartmorgan-gstuartmorgan-g
authored
[tool] Add initial file-based command skipping (#8928)
Adds initial file-based filtering. This does not attempt to be comprehensive, just to get some low-hanging fruit, and to create a blueprint for anyone to follow in the future when adding more filtering. I expect that once this is in place, what will happen is that as we notice cases where PRs are hitting slow or flaky tests that they clearly don't need to, we can incrementally improve the filtering on demand. Fixes flutter/flutter#136394
1 parent 4988af5 commit fdc1ec7

19 files changed

+792
-28
lines changed

‎script/tool/lib/src/analyze_command.dart

+8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import 'dart:io' as io;
66

77
import 'package:file/file.dart';
88

9+
import 'common/file_filters.dart';
910
import 'common/output_utils.dart';
1011
import 'common/package_looping_command.dart';
1112
import 'common/repository_package.dart';
@@ -92,6 +93,13 @@ class AnalyzeCommand extends PackageLoopingCommand {
9293
return false;
9394
}
9495

96+
@override
97+
bool shouldIgnoreFile(String path) {
98+
return isRepoLevelNonCodeImpactingFile(path) ||
99+
isNativeCodeFile(path) ||
100+
isPackageSupportFile(path);
101+
}
102+
95103
@override
96104
Future<void> initializeRun() async {
97105
_allowedCustomAnalysisDirectories = getYamlListArg(_customAnalysisFlag);

‎script/tool/lib/src/build_examples_command.dart

+6
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import 'package:file/file.dart';
66
import 'package:yaml/yaml.dart';
77

88
import 'common/core.dart';
9+
import 'common/file_filters.dart';
910
import 'common/output_utils.dart';
1011
import 'common/package_looping_command.dart';
1112
import 'common/plugin_utils.dart';
@@ -134,6 +135,11 @@ class BuildExamplesCommand extends PackageLoopingCommand {
134135
return getNullableBoolArg(_swiftPackageManagerFlag);
135136
}
136137

138+
@override
139+
bool shouldIgnoreFile(String path) {
140+
return isRepoLevelNonCodeImpactingFile(path) || isPackageSupportFile(path);
141+
}
142+
137143
@override
138144
Future<void> initializeRun() async {
139145
final List<String> platformFlags = _platforms.keys.toList();

‎script/tool/lib/src/common/file_filters.dart

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
/// Returns true for repository-level paths of files that do not affect *any*
6+
/// code-related commands (example builds, Dart analysis, native code analysis,
7+
/// native tests, Dart tests, etc.) for use in command-ignored-files lists for
8+
/// commands that are only affected by package code.
9+
bool isRepoLevelNonCodeImpactingFile(String path) {
10+
return <String>[
11+
'AUTHORS',
12+
'CODEOWNERS',
13+
'CONTRIBUTING.md',
14+
'LICENSE',
15+
'README.md',
16+
// This deliberate lists specific files rather than excluding the whole
17+
// .github directory since it's better to have false negatives than to
18+
// accidentally skip tests if something is later added to the directory
19+
// that could affect packages.
20+
'.github/PULL_REQUEST_TEMPLATE.md',
21+
'.github/dependabot.yml',
22+
'.github/labeler.yml',
23+
'.github/post_merge_labeler.yml',
24+
'.github/workflows/pull_request_label.yml',
25+
].contains(path);
26+
}
27+
28+
/// Returns true for native (non-Dart) code files, for use in command-ignored-
29+
/// files lists for commands that aren't affected by native code (e.g., Dart
30+
/// analysis and unit tests).
31+
bool isNativeCodeFile(String path) {
32+
return path.endsWith('.c') ||
33+
path.endsWith('.cc') ||
34+
path.endsWith('.cpp') ||
35+
path.endsWith('.h') ||
36+
path.endsWith('.m') ||
37+
path.endsWith('.swift') ||
38+
path.endsWith('.java') ||
39+
path.endsWith('.kt');
40+
}
41+
42+
/// Returns true for package-level human-focused support files, for use in
43+
/// command-ignored-files lists for commands that aren't affected by files that
44+
/// aren't used in any builds.
45+
///
46+
/// This must *not* include metadata files that do affect builds, such as
47+
/// pubspec.yaml.
48+
bool isPackageSupportFile(String path) {
49+
return path.endsWith('/AUTHORS') ||
50+
path.endsWith('/CHANGELOG.md') ||
51+
path.endsWith('/CONTRIBUTING.md') ||
52+
path.endsWith('/README.md');
53+
}

‎script/tool/lib/src/common/package_looping_command.dart

+22
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,20 @@ abstract class PackageLoopingCommand extends PackageCommand {
109109
/// The package currently being run by [runForPackage].
110110
PackageEnumerationEntry? _currentPackageEntry;
111111

112+
/// When running against a merge base, this is called before [initializeRun]
113+
/// for every changed file, to see if that file is a file that is guaranteed
114+
/// *not* to require running this command.
115+
///
116+
/// If every changed file returns true, then the command will be skipped.
117+
/// Because this causes tests not to run, subclasses should be very
118+
/// consevative about what returns true; for anything borderline it is much
119+
/// better to err on the side of running tests unnecessarily than to risk
120+
/// losing test coverage.
121+
///
122+
/// [path] is a POSIX-style path regardless of the host platforrm, and is
123+
/// relative to the git repo root.
124+
bool shouldIgnoreFile(String path) => false;
125+
112126
/// Called during [run] before any calls to [runForPackage]. This provides an
113127
/// opportunity to fail early if the command can't be run (e.g., because the
114128
/// arguments are invalid), and to set up any run-level state.
@@ -281,6 +295,14 @@ abstract class PackageLoopingCommand extends PackageCommand {
281295
baseSha = await gitVersionFinder.getBaseSha();
282296
changedFiles = await gitVersionFinder.getChangedFiles();
283297

298+
// Check whether the command needs to run.
299+
if (changedFiles.isNotEmpty && changedFiles.every(shouldIgnoreFile)) {
300+
_printColorized(
301+
'SKIPPING ALL PACKAGES: No changed files affect this command',
302+
Styles.DARK_GRAY);
303+
return true;
304+
}
305+
284306
await initializeRun();
285307

286308
final List<PackageEnumerationEntry> targetPackages =

‎script/tool/lib/src/dart_test_command.dart

+8
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import 'package:file/file.dart';
66

77
import 'common/core.dart';
8+
import 'common/file_filters.dart';
89
import 'common/output_utils.dart';
910
import 'common/package_looping_command.dart';
1011
import 'common/plugin_utils.dart';
@@ -65,6 +66,13 @@ class DartTestCommand extends PackageLoopingCommand {
6566
PackageLoopingType get packageLoopingType =>
6667
PackageLoopingType.includeAllSubpackages;
6768

69+
@override
70+
bool shouldIgnoreFile(String path) {
71+
return isRepoLevelNonCodeImpactingFile(path) ||
72+
isNativeCodeFile(path) ||
73+
isPackageSupportFile(path);
74+
}
75+
6876
@override
6977
Future<PackageResult> runForPackage(RepositoryPackage package) async {
7078
if (!package.testDirectory.existsSync()) {

‎script/tool/lib/src/drive_examples_command.dart

+6
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import 'dart:io';
99
import 'package:file/file.dart';
1010

1111
import 'common/core.dart';
12+
import 'common/file_filters.dart';
1213
import 'common/output_utils.dart';
1314
import 'common/package_looping_command.dart';
1415
import 'common/plugin_utils.dart';
@@ -68,6 +69,11 @@ class DriveExamplesCommand extends PackageLoopingCommand {
6869

6970
Map<String, List<String>> _targetDeviceFlags = const <String, List<String>>{};
7071

72+
@override
73+
bool shouldIgnoreFile(String path) {
74+
return isRepoLevelNonCodeImpactingFile(path) || isPackageSupportFile(path);
75+
}
76+
7177
@override
7278
Future<void> initializeRun() async {
7379
final List<String> platformSwitches = <String>[

‎script/tool/lib/src/firebase_test_lab_command.dart

+6
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'package:file/file.dart';
88
import 'package:uuid/uuid.dart';
99

1010
import 'common/core.dart';
11+
import 'common/file_filters.dart';
1112
import 'common/flutter_command_utils.dart';
1213
import 'common/gradle.dart';
1314
import 'common/output_utils.dart';
@@ -122,6 +123,11 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
122123
_firebaseProjectConfigured = true;
123124
}
124125

126+
@override
127+
bool shouldIgnoreFile(String path) {
128+
return isRepoLevelNonCodeImpactingFile(path) || isPackageSupportFile(path);
129+
}
130+
125131
@override
126132
Future<PackageResult> runForPackage(RepositoryPackage package) async {
127133
final List<PackageResult> results = <PackageResult>[];

‎script/tool/lib/src/lint_android_command.dart

+10
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// found in the LICENSE file.
44

55
import 'common/core.dart';
6+
import 'common/file_filters.dart';
67
import 'common/flutter_command_utils.dart';
78
import 'common/gradle.dart';
89
import 'common/output_utils.dart';
@@ -29,6 +30,15 @@ class LintAndroidCommand extends PackageLoopingCommand {
2930
final String description = 'Runs "gradlew lint" on Android plugins.\n\n'
3031
'Requires the examples to have been build at least once before running.';
3132

33+
@override
34+
bool shouldIgnoreFile(String path) {
35+
return isRepoLevelNonCodeImpactingFile(path) ||
36+
isPackageSupportFile(path) ||
37+
// These are part of the build, but don't affect native code analysis.
38+
path.endsWith('/pubspec.yaml') ||
39+
path.endsWith('.dart');
40+
}
41+
3242
@override
3343
Future<PackageResult> runForPackage(RepositoryPackage package) async {
3444
if (!pluginSupportsPlatform(platformAndroid, package,

‎script/tool/lib/src/native_test_command.dart

+8
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import 'package:meta/meta.dart';
99

1010
import 'common/cmake.dart';
1111
import 'common/core.dart';
12+
import 'common/file_filters.dart';
1213
import 'common/flutter_command_utils.dart';
1314
import 'common/gradle.dart';
1415
import 'common/output_utils.dart';
@@ -114,6 +115,13 @@ this command.
114115

115116
Set<String> _xcodeWarningsExceptions = <String>{};
116117

118+
@override
119+
bool shouldIgnoreFile(String path) {
120+
return isRepoLevelNonCodeImpactingFile(path) || isPackageSupportFile(path);
121+
// It may seem tempting to filter out *.dart, but that would skip critical
122+
// testing since native integration tests run the full compiled application.
123+
}
124+
117125
@override
118126
Future<void> initializeRun() async {
119127
_platforms = <String, _PlatformDetails>{

‎script/tool/lib/src/xcode_analyze_command.dart

+10
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// found in the LICENSE file.
44

55
import 'common/core.dart';
6+
import 'common/file_filters.dart';
67
import 'common/flutter_command_utils.dart';
78
import 'common/output_utils.dart';
89
import 'common/package_looping_command.dart';
@@ -47,6 +48,15 @@ class XcodeAnalyzeCommand extends PackageLoopingCommand {
4748
final String description =
4849
'Runs Xcode analysis on the iOS and/or macOS example apps.';
4950

51+
@override
52+
bool shouldIgnoreFile(String path) {
53+
return isRepoLevelNonCodeImpactingFile(path) ||
54+
isPackageSupportFile(path) ||
55+
// These are part of the build, but don't affect native code analysis.
56+
path.endsWith('/pubspec.yaml') ||
57+
path.endsWith('.dart');
58+
}
59+
5060
@override
5161
Future<void> initializeRun() async {
5262
if (!(getBoolArg(platformIOS) || getBoolArg(platformMacOS))) {

‎script/tool/test/analyze_command_test.dart

+88-1
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ void main() {
1616
late MockPlatform mockPlatform;
1717
late Directory packagesDir;
1818
late RecordingProcessRunner processRunner;
19+
late RecordingProcessRunner gitProcessRunner;
1920
late CommandRunner<void> runner;
2021

2122
setUp(() {
2223
mockPlatform = MockPlatform();
2324
final GitDir gitDir;
24-
(:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) =
25+
(:packagesDir, :processRunner, :gitProcessRunner, :gitDir) =
2526
configureBaseCommandMocks(platform: mockPlatform);
2627
final AnalyzeCommand analyzeCommand = AnalyzeCommand(
2728
packagesDir,
@@ -470,4 +471,90 @@ void main() {
470471
]),
471472
);
472473
});
474+
475+
group('file filtering', () {
476+
test('runs command for changes to Dart source', () async {
477+
createFakePackage('package_a', packagesDir);
478+
479+
gitProcessRunner.mockProcessesForExecutable['git-diff'] =
480+
<FakeProcessInfo>[
481+
FakeProcessInfo(MockProcess(stdout: '''
482+
packages/package_a/foo.dart
483+
''')),
484+
];
485+
486+
final List<String> output =
487+
await runCapturingPrint(runner, <String>['analyze']);
488+
489+
expect(
490+
output,
491+
containsAllInOrder(<Matcher>[
492+
contains('Running for package_a'),
493+
]));
494+
});
495+
496+
const List<String> files = <String>[
497+
'foo.java',
498+
'foo.kt',
499+
'foo.m',
500+
'foo.swift',
501+
'foo.c',
502+
'foo.cc',
503+
'foo.cpp',
504+
'foo.h',
505+
];
506+
for (final String file in files) {
507+
test('skips command for changes to non-Dart source $file', () async {
508+
createFakePackage('package_a', packagesDir);
509+
510+
gitProcessRunner.mockProcessesForExecutable['git-diff'] =
511+
<FakeProcessInfo>[
512+
FakeProcessInfo(MockProcess(stdout: '''
513+
packages/package_a/$file
514+
''')),
515+
];
516+
517+
final List<String> output =
518+
await runCapturingPrint(runner, <String>['analyze']);
519+
520+
expect(
521+
output,
522+
isNot(containsAllInOrder(<Matcher>[
523+
contains('Running for package_a'),
524+
])));
525+
expect(
526+
output,
527+
containsAllInOrder(<Matcher>[
528+
contains('SKIPPING ALL PACKAGES'),
529+
]));
530+
});
531+
}
532+
533+
test('skips commands if all files should be ignored', () async {
534+
createFakePackage('package_a', packagesDir);
535+
536+
gitProcessRunner.mockProcessesForExecutable['git-diff'] =
537+
<FakeProcessInfo>[
538+
FakeProcessInfo(MockProcess(stdout: '''
539+
README.md
540+
CODEOWNERS
541+
packages/package_a/CHANGELOG.md
542+
''')),
543+
];
544+
545+
final List<String> output =
546+
await runCapturingPrint(runner, <String>['analyze']);
547+
548+
expect(
549+
output,
550+
isNot(containsAllInOrder(<Matcher>[
551+
contains('Running for package_a'),
552+
])));
553+
expect(
554+
output,
555+
containsAllInOrder(<Matcher>[
556+
contains('SKIPPING ALL PACKAGES'),
557+
]));
558+
});
559+
});
473560
}

0 commit comments

Comments
 (0)