Skip to content

[tool] Add initial file-based command skipping #8928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Collect common helpers
  • Loading branch information
stuartmorgan-g committed Apr 17, 2025
commit f62c27839f971224722f8b349ffb63c39c4a6c71
19 changes: 4 additions & 15 deletions script/tool/lib/src/analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import 'dart:io' as io;

import 'package:file/file.dart';

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/repository_package.dart';
Expand Down Expand Up @@ -95,20 +95,9 @@ class AnalyzeCommand extends PackageLoopingCommand {

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
// Native code, which is not part of Dart analysis.
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 analysis.
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
return isRepoLevelNonCodeImpactingFile(path) ||
isNativeCodeFile(path) ||
isPackageSupportFile(path);
}

@override
Expand Down
6 changes: 2 additions & 4 deletions script/tool/lib/src/build_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:file/file.dart';
import 'package:yaml/yaml.dart';

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/plugin_utils.dart';
Expand Down Expand Up @@ -136,10 +137,7 @@ class BuildExamplesCommand extends PackageLoopingCommand {

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

@override
Expand Down
21 changes: 0 additions & 21 deletions script/tool/lib/src/common/core.dart
Original file line number Diff line number Diff line change
Expand Up @@ -140,24 +140,3 @@ 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',
];
53 changes: 53 additions & 0 deletions script/tool/lib/src/common/file_filters.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/// Returns true for repository-level paths of 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.
bool isRepoLevelNonCodeImpactingFile(String path) {
return <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',
].contains(path);
}

/// Returns true for native (non-Dart) code files, for use in command-ignored-
/// files lists for commands that aren't affected by native code (e.g., Dart
/// analysis and unit tests).
bool isNativeCodeFile(String path) {
return path.endsWith('.c') ||
path.endsWith('.cc') ||
path.endsWith('.cpp') ||
path.endsWith('.h') ||
path.endsWith('.m') ||
path.endsWith('.swift') ||
path.endsWith('.java') ||
path.endsWith('.kt');
}

/// Returns true for package-level human-focused support files, for use in
/// command-ignored-files lists for commands that aren't affected by files that
/// aren't used in any builds.
///
/// This must *not* include metadata files that do affect builds, such as
/// pubspec.yaml.
bool isPackageSupportFile(String path) {
return path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/CONTRIBUTING.md') ||
path.endsWith('/README.md');
}
18 changes: 4 additions & 14 deletions script/tool/lib/src/dart_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:file/file.dart';

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/plugin_utils.dart';
Expand Down Expand Up @@ -67,20 +68,9 @@ class DartTestCommand extends PackageLoopingCommand {

@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');
return isRepoLevelNonCodeImpactingFile(path) ||
isNativeCodeFile(path) ||
isPackageSupportFile(path);
}

@override
Expand Down
6 changes: 2 additions & 4 deletions script/tool/lib/src/drive_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'dart:io';
import 'package:file/file.dart';

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/plugin_utils.dart';
Expand Down Expand Up @@ -70,10 +71,7 @@ class DriveExamplesCommand extends PackageLoopingCommand {

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

@override
Expand Down
7 changes: 3 additions & 4 deletions script/tool/lib/src/firebase_test_lab_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import 'package:file/file.dart';
import 'package:uuid/uuid.dart';

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/flutter_command_utils.dart';
import 'common/gradle.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
Expand Down Expand Up @@ -123,10 +125,7 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {

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

@override
Expand Down
11 changes: 11 additions & 0 deletions script/tool/lib/src/lint_android_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// found in the LICENSE file.

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

@override
bool shouldIgnoreFile(String path) {
return isRepoLevelNonCodeImpactingFile(path) ||
isPackageSupportFile(path) ||
// These are part of the build, but don't affect native code analysis.
path.endsWith('/pubspec.yaml') ||
path.endsWith('.dart');
}

@override
Future<PackageResult> runForPackage(RepositoryPackage package) async {
if (!pluginSupportsPlatform(platformAndroid, package,
Expand Down
7 changes: 3 additions & 4 deletions script/tool/lib/src/native_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import 'package:meta/meta.dart';

import 'common/cmake.dart';
import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/flutter_command_utils.dart';
import 'common/gradle.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
Expand Down Expand Up @@ -115,10 +117,7 @@ this command.

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
return isRepoLevelNonCodeImpactingFile(path) || isPackageSupportFile(path);
// It may seem tempting to filter out *.dart, but that would skip critical
// testing since native integration tests run the full compiled application.
}
Expand Down
11 changes: 7 additions & 4 deletions script/tool/lib/src/xcode_analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// found in the LICENSE file.

import 'common/core.dart';
import 'common/file_filters.dart';
import 'common/flutter_command_utils.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/plugin_utils.dart';
Expand Down Expand Up @@ -48,10 +50,11 @@ class XcodeAnalyzeCommand extends PackageLoopingCommand {

@override
bool shouldIgnoreFile(String path) {
return repoLevelNonCodeImpactingFiles.contains(path) ||
path.endsWith('/AUTHORS') ||
path.endsWith('/CHANGELOG.md') ||
path.endsWith('/README.md');
return isRepoLevelNonCodeImpactingFile(path) ||
isPackageSupportFile(path) ||
// These are part of the build, but don't affect native code analysis.
path.endsWith('/pubspec.yaml') ||
path.endsWith('.dart');
}

@override
Expand Down
59 changes: 58 additions & 1 deletion script/tool/test/lint_android_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ void main() {
late CommandRunner<void> runner;
late MockPlatform mockPlatform;
late RecordingProcessRunner processRunner;
late RecordingProcessRunner gitProcessRunner;

setUp(() {
mockPlatform = MockPlatform();
final GitDir gitDir;
(:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) =
(:packagesDir, :processRunner, :gitProcessRunner, :gitDir) =
configureBaseCommandMocks(platform: mockPlatform);
final LintAndroidCommand command = LintAndroidCommand(
packagesDir,
Expand Down Expand Up @@ -241,5 +242,61 @@ void main() {
],
));
});

group('file filtering', () {
const List<String> files = <String>[
'foo.java',
'foo.kt',
];
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
''')),
];

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

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
packages/package_a/lib/foo.dart
''')),
];

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

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Running for package_a'),
])));
expect(
output,
containsAllInOrder(<Matcher>[
contains('SKIPPING ALL PACKAGES'),
]));
});
});
});
}
5 changes: 1 addition & 4 deletions script/tool/test/xcode_analyze_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -493,10 +493,6 @@ void main() {

group('file filtering', () {
const List<String> files = <String>[
'pubspec.yaml',
'foo.dart',
'foo.java',
'foo.kt',
'foo.m',
'foo.swift',
'foo.cc',
Expand Down Expand Up @@ -534,6 +530,7 @@ packages/package_a/$file
README.md
CODEOWNERS
packages/package_a/CHANGELOG.md
packages/package_a/lib/foo.dart
''')),
];

Expand Down