Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions script/tool/lib/src/analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:io' as io;

import 'package:file/file.dart';

import 'common/core.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/repository_package.dart';
Expand Down Expand Up @@ -92,6 +93,24 @@ class AnalyzeCommand extends PackageLoopingCommand {
return false;
}

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
// Native code, which is not part of Dart analysis.
path.endsWith('.c') ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this could also be a list named something like sourceFileExtensions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea; extracted into a helper called isNativeCodeFile. I also changed the repoLevelNonCodeImpactingFiles list into a helper method for consistency, so everything can just call a set of helpers without it mattering whether it's an exact-match list or a more complicated pattern.

path.endsWith('.cc') ||
path.endsWith('.cpp') ||
path.endsWith('.h') ||
path.endsWith('.m') ||
path.endsWith('.swift') ||
path.endsWith('.java') ||
path.endsWith('.kt') ||
// Package metadata that doesn't impact analysis.
path.endsWith('/AUTHORS') ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Same for here. Its repeated in other commands, so this could be list like packageMetadataFiles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called it isPackageSupportFile; "metadata" to me suggests it would include pubspec.yaml, but pubspec.yaml can affect a bunch of things, including Dart analysis, so I don't generally want to include it.

path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<void> initializeRun() async {
_allowedCustomAnalysisDirectories = getYamlListArg(_customAnalysisFlag);
Expand Down
8 changes: 8 additions & 0 deletions script/tool/lib/src/build_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,14 @@ class BuildExamplesCommand extends PackageLoopingCommand {
return getNullableBoolArg(_swiftPackageManagerFlag);
}

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<void> initializeRun() async {
final List<String> platformFlags = _platforms.keys.toList();
Expand Down
21 changes: 21 additions & 0 deletions script/tool/lib/src/common/core.dart
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,24 @@ Directory? ciLogsDirectory(Platform platform, FileSystem fileSystem) {
}
return logsDirectory;
}

/// Full repo-relative paths to files that do not affect *any* code-related
/// commands (example builds, Dart analysis, native code analysis, native tests,
/// Dart tests, etc.) for use in command-ignored files lists for commands that
/// are only affected by package code.
const List<String> repoLevelNonCodeImpactingFiles = <String>[
'AUTHORS',
'CODEOWNERS',
'CONTRIBUTING.md',
'LICENSE',
'README.md',
// This deliberate lists specific files rather than excluding the whole
// .github directory since it's better to have false negatives than to
// accidentally skip tests if something is later added to the directory
// that could affect packages.
'.github/PULL_REQUEST_TEMPLATE.md',
'.github/dependabot.yml',
'.github/labeler.yml',
'.github/post_merge_labeler.yml',
'.github/workflows/pull_request_label.yml',
];
22 changes: 22 additions & 0 deletions script/tool/lib/src/common/package_looping_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,20 @@ abstract class PackageLoopingCommand extends PackageCommand {
/// The package currently being run by [runForPackage].
PackageEnumerationEntry? _currentPackageEntry;

/// When running against a merge base, this is called before [initializeRun]
/// for every changed file, to see if that file is a file that is guaranteed
/// *not* to require running this command.
///
/// If every changed file returns true, then the command will be skipped.
/// Because this causes tests not to run, subclasses should be very
/// consevative about what returns true; for anything borderline it is much
/// better to err on the side of running tests unnecessarily than to risk
/// losing test coverage.
///
/// [path] is a POSIX-style path regardless of the host platforrm, and is
/// relative to the git repo root.
bool shouldIgnoreFile(String path) => false;

/// Called during [run] before any calls to [runForPackage]. This provides an
/// opportunity to fail early if the command can't be run (e.g., because the
/// arguments are invalid), and to set up any run-level state.
Expand Down Expand Up @@ -281,6 +295,14 @@ abstract class PackageLoopingCommand extends PackageCommand {
baseSha = await gitVersionFinder.getBaseSha();
changedFiles = await gitVersionFinder.getChangedFiles();

// Check whether the command needs to run.
if (changedFiles.isNotEmpty && changedFiles.every(shouldIgnoreFile)) {
_printColorized(
'SKIPPING ALL PACKAGES: No changed files affect this command',
Styles.DARK_GRAY);
return true;
}

await initializeRun();

final List<PackageEnumerationEntry> targetPackages =
Expand Down
18 changes: 18 additions & 0 deletions script/tool/lib/src/dart_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,24 @@ class DartTestCommand extends PackageLoopingCommand {
PackageLoopingType get packageLoopingType =>
PackageLoopingType.includeAllSubpackages;

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
// Native code, which is not part of Dart unit testing.
path.endsWith('.c') ||
path.endsWith('.cc') ||
path.endsWith('.cpp') ||
path.endsWith('.h') ||
path.endsWith('.m') ||
path.endsWith('.swift') ||
path.endsWith('.java') ||
path.endsWith('.kt') ||
// Package metadata that doesn't impact Dart unit tests.
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<PackageResult> runForPackage(RepositoryPackage package) async {
if (!package.testDirectory.existsSync()) {
Expand Down
8 changes: 8 additions & 0 deletions script/tool/lib/src/drive_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ class DriveExamplesCommand extends PackageLoopingCommand {

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

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<void> initializeRun() async {
final List<String> platformSwitches = <String>[
Expand Down
8 changes: 8 additions & 0 deletions script/tool/lib/src/firebase_test_lab_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
_firebaseProjectConfigured = true;
}

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<PackageResult> runForPackage(RepositoryPackage package) async {
final List<PackageResult> results = <PackageResult>[];
Expand Down
10 changes: 10 additions & 0 deletions script/tool/lib/src/native_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,16 @@ this command.

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

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
// It may seem tempting to filter out *.dart, but that would skip critical
// testing since native integration tests run the full compiled application.
}

@override
Future<void> initializeRun() async {
_platforms = <String, _PlatformDetails>{
Expand Down
8 changes: 8 additions & 0 deletions script/tool/lib/src/xcode_analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ class XcodeAnalyzeCommand extends PackageLoopingCommand {
final String description =
'Runs Xcode analysis on the iOS and/or macOS example apps.';

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
}

@override
Future<void> initializeRun() async {
if (!(getBoolArg(platformIOS) || getBoolArg(platformMacOS))) {
Expand Down
89 changes: 88 additions & 1 deletion script/tool/test/analyze_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ void main() {
late MockPlatform mockPlatform;
late Directory packagesDir;
late RecordingProcessRunner processRunner;
late RecordingProcessRunner gitProcessRunner;
late CommandRunner<void> runner;

setUp(() {
mockPlatform = MockPlatform();
final GitDir gitDir;
(:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) =
(:packagesDir, :processRunner, :gitProcessRunner, :gitDir) =
configureBaseCommandMocks(platform: mockPlatform);
final AnalyzeCommand analyzeCommand = AnalyzeCommand(
packagesDir,
Expand Down Expand Up @@ -470,4 +471,90 @@ void main() {
]),
);
});

group('file filtering', () {
test('runs command for changes to Dart source', () async {
createFakePackage('package_a', packagesDir);

gitProcessRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
packages/package_a/foo.dart
''')),
];

final List<String> output =
await runCapturingPrint(runner, <String>['analyze']);

expect(
output,
containsAllInOrder(<Matcher>[
contains('Running for package_a'),
]));
});

const List<String> files = <String>[
'foo.java',
'foo.kt',
'foo.m',
'foo.swift',
'foo.c',
'foo.cc',
'foo.cpp',
'foo.h',
];
for (final String file in files) {
test('skips command for changes to non-Dart source $file', () async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen running test() in a for loop before. Does this pattern work well in IDEs? For example if I try to rerun a failing test, it typically passes the name of the test to flutter test.

Also, this include the $, which I believe doesn't work well with Visual Studio I think.

Same for the command tests below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we do it in a few other places already. VS Code supports it well; it looks like this before you run it:
Screenshot 2025-04-17 at 2 50 38 PM
and then this after:
Screenshot 2025-04-17 at 2 50 58 PM

For example if I try to rerun a failing test, it typically passes the name of the test to flutter test.

Re-running individual entries from the IDE works. (IIRC, the way Dart test works is that it runs through the whole program and handles all test calls by collecting the names they are called with, and then running tests uses that collected set, so if you pass a name matching a generated name, by the time it tries to do the match the generated name is guaranteed to exist.)

Also, this include the $, which I believe doesn't work well with Visual Studio I think.

The problem we had was with group ('test $SomeClass'), which confused the tree in VS. I'm not sure if that still happens or has been fixed, but it also wasn't buying us anything because we didn't actually want a dynamic name, unlike the loop case.

createFakePackage('package_a', packagesDir);

gitProcessRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
packages/package_a/$file
''')),
];

final List<String> output =
await runCapturingPrint(runner, <String>['analyze']);

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Running for package_a'),
])));
expect(
output,
containsAllInOrder(<Matcher>[
contains('SKIPPING ALL PACKAGES'),
]));
});
}

test('skips commands if all files should be ignored', () async {
createFakePackage('package_a', packagesDir);

gitProcessRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
README.md
CODEOWNERS
packages/package_a/CHANGELOG.md
''')),
];

final List<String> output =
await runCapturingPrint(runner, <String>['analyze']);

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Running for package_a'),
])));
expect(
output,
containsAllInOrder(<Matcher>[
contains('SKIPPING ALL PACKAGES'),
]));
});
});
}
70 changes: 69 additions & 1 deletion script/tool/test/build_examples_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ void main() {
late Directory packagesDir;
late CommandRunner<void> runner;
late RecordingProcessRunner processRunner;
late RecordingProcessRunner gitProcessRunner;

setUp(() {
mockPlatform = MockPlatform();
final GitDir gitDir;
(:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) =
(:packagesDir, :processRunner, :gitProcessRunner, :gitDir) =
configureBaseCommandMocks(platform: mockPlatform);
final BuildExamplesCommand command = BuildExamplesCommand(
packagesDir,
Expand Down Expand Up @@ -998,5 +999,72 @@ void main() {
pluginExampleDirectory.path),
]));
});

group('file filtering', () {
const List<String> files = <String>[
'pubspec.yaml',
'foo.dart',
'foo.java',
'foo.kt',
'foo.m',
'foo.swift',
'foo.cc',
'foo.cpp',
'foo.h',
];
for (final String file in files) {
test('runs command for changes to $file', () async {
createFakePackage('package_a', packagesDir);

gitProcessRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
packages/package_a/$file
''')),
];

// The target platform is irrelevant here; because this repo's
// packages are fully federated, there's no need to distinguish
// the ignore list by target (e.g., skipping iOS tests if only Java or
// Kotlin files change), because package-level filering will already
// accomplish the same goal.
final List<String> output = await runCapturingPrint(
runner, <String>['build-examples', '--web']);

expect(
output,
containsAllInOrder(<Matcher>[
contains('Running for package_a'),
]));
});
}

test('skips commands if all files should be ignored', () async {
createFakePackage('package_a', packagesDir);

gitProcessRunner.mockProcessesForExecutable['git-diff'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: '''
README.md
CODEOWNERS
packages/package_a/CHANGELOG.md
''')),
];

final List<String> output =
await runCapturingPrint(runner, <String>['build-examples']);

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Running for package_a'),
])));
expect(
output,
containsAllInOrder(<Matcher>[
contains('SKIPPING ALL PACKAGES'),
]));
});
});
});
}
Loading