Skip to content

Commit

Permalink
[tool] Move changed file detection to base command class
Browse files Browse the repository at this point in the history
Consolidates the code to find all changed file paths into the
`PackageLoopingCommand` class that is the base of almost all of the repo
tooling commands. This in a preparatory PR for a future change to allow
each command to define a list of files or file patterns that
definitively *don't* affect that test, so that CI can be smarter about
what tests to run (e.g., not running expensive integration tests for
README changes).

A side effect of this change is that tests of almost all commands now
need a mock `GitDir` instance. This would add a lot of copy/pasted
boilerplate to the test setup, and there is already too much of that, so
instead this refactors common test setup:
- Creating a memory file system
- Populating it with a packages directory
- Creating a RecordingProcessRunner to mock out process calls
- Creating a mock GitDir that forwards to a RecordingProcessRunner

into a helper method (using records and destructuring to easily return
multiple values). While some tests don't need all of these steps, those
that don't can easily ignore parts of it, and it will make it much
easier to update tests in the future if they need them, and it makes the
setup much more consistent which makes it easier to reason about test
setup in general.

Prep for flutter/flutter#136394
  • Loading branch information
stuartmorgan committed Feb 27, 2025
1 parent c44c228 commit 571e1af
Show file tree
Hide file tree
Showing 65 changed files with 586 additions and 632 deletions.
1 change: 1 addition & 0 deletions script/tool/lib/src/analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class AnalyzeCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addMultiOption(_customAnalysisFlag,
help:
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/build_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class BuildExamplesCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addFlag(platformLinux);
argParser.addFlag(platformMacOS);
Expand Down
9 changes: 0 additions & 9 deletions script/tool/lib/src/common/git_version_finder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ class GitVersionFinder {
/// The base branche used to find a merge point if baseSha is not provided.
final String _baseBranch;

static bool _isPubspec(String file) {
return file.trim().endsWith('pubspec.yaml');
}

/// Get a list of all the pubspec.yaml file that is changed.
Future<List<String>> getChangedPubSpecs() async {
return (await getChangedFiles()).where(_isPubspec).toList();
}

/// Get a list of all the changed files.
Future<List<String>> getChangedFiles(
{bool includeUncommitted = false}) async {
Expand Down
17 changes: 17 additions & 0 deletions script/tool/lib/src/common/package_looping_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:path/path.dart' as p;
import 'package:pub_semver/pub_semver.dart';

import 'core.dart';
import 'git_version_finder.dart';
import 'output_utils.dart';
import 'package_command.dart';
import 'repository_package.dart';
Expand Down Expand Up @@ -226,6 +227,17 @@ abstract class PackageLoopingCommand extends PackageCommand {
/// The suggested indentation for printed output.
String get indentation => hasLongOutput ? '' : ' ';

/// The base SHA used to calculate changed files.
///
/// This is guaranteed to be populated before [initializeRun] is called.
late String baseSha;

/// The repo-relative paths (using Posix separators) of all files changed
/// relative to [baseSha].
///
/// This is guaranteed to be populated before [initializeRun] is called.
late List<String> changedFiles;

// ----------------------------------------

@override
Expand Down Expand Up @@ -264,6 +276,11 @@ abstract class PackageLoopingCommand extends PackageCommand {

final DateTime runStart = DateTime.now();

// Populate the list of changed files for subclasses to use.
final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
baseSha = await gitVersionFinder.getBaseSha();
changedFiles = await gitVersionFinder.getChangedFiles();

await initializeRun();

final List<PackageEnumerationEntry> targetPackages =
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/custom_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class CustomTestCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
});

@override
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/dart_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class DartTestCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addOption(
kEnableExperiment,
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/drive_examples_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class DriveExamplesCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addFlag(platformAndroid,
help: 'Runs the Android implementation of the examples',
Expand Down
3 changes: 1 addition & 2 deletions script/tool/lib/src/federation_safety_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,8 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand {
@override
Future<void> initializeRun() async {
final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
final String baseSha = await gitVersionFinder.getBaseSha();
print('Validating changes relative to "$baseSha"\n');
for (final String path in await gitVersionFinder.getChangedFiles()) {
for (final String path in changedFiles) {
// Git output always uses Posix paths.
final List<String> allComponents = p.posix.split(path);
final int packageIndex = allComponents.indexOf('packages');
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/fetch_deps_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class FetchDepsCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addFlag(_dartFlag, defaultsTo: true, help: 'Run "pub get"');
argParser.addFlag(_supportingTargetPlatformsOnlyFlag,
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/firebase_test_lab_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addOption(
_gCloudProjectArg,
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/fix_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class FixCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
});

@override
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/format_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class FormatCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) {
argParser.addFlag(_failonChangeArg, hide: true);
argParser.addFlag(_dartArg, help: 'Format Dart files', defaultsTo: true);
Expand Down
5 changes: 4 additions & 1 deletion script/tool/lib/src/gradle_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ final Version minKotlinVersion = Version(1, 7, 10);
/// A command to enforce gradle file conventions and best practices.
class GradleCheckCommand extends PackageLoopingCommand {
/// Creates an instance of the gradle check command.
GradleCheckCommand(super.packagesDir);
GradleCheckCommand(
super.packagesDir, {
super.gitDir,
});

@override
final String name = 'gradle-check';
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/lint_android_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class LintAndroidCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
});

@override
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/native_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class NativeTestCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
Abi? abi,
}) : _abi = abi ?? Abi.current(),
_xcode = Xcode(processRunner: processRunner, log: true) {
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/podspec_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class PodspecCheckCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
});

@override
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/publish_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class PublishCheckCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
http.Client? httpClient,
}) : _pubVersionFinder =
PubVersionFinder(httpClient: httpClient ?? http.Client()) {
Expand Down
9 changes: 4 additions & 5 deletions script/tool/lib/src/publish_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import 'package:yaml/yaml.dart';

import 'common/core.dart';
import 'common/file_utils.dart';
import 'common/git_version_finder.dart';
import 'common/output_utils.dart';
import 'common/package_command.dart';
import 'common/package_looping_command.dart';
Expand Down Expand Up @@ -175,12 +174,12 @@ class PublishCommand extends PackageLoopingCommand {
@override
Stream<PackageEnumerationEntry> getPackagesToProcess() async* {
if (getBoolArg(_allChangedFlag)) {
final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
final String baseSha = await gitVersionFinder.getBaseSha();
print(
'Publishing all packages that have changed relative to "$baseSha"\n');
final List<String> changedPubspecs =
await gitVersionFinder.getChangedPubSpecs();

final List<String> changedPubspecs = changedFiles
.where((String file) => file.trim().endsWith('pubspec.yaml'))
.toList();

for (final String pubspecPath in changedPubspecs) {
// git outputs a relativa, Posix-style path.
Expand Down
5 changes: 4 additions & 1 deletion script/tool/lib/src/remove_dev_dependencies_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import 'common/repository_package.dart';
/// clients of the library, but not for development of the library.
class RemoveDevDependenciesCommand extends PackageLoopingCommand {
/// Creates a publish metadata updater command instance.
RemoveDevDependenciesCommand(super.packagesDir);
RemoveDevDependenciesCommand(
super.packagesDir, {
super.gitDir,
});

@override
final String name = 'remove-dev-dependencies';
Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/update_dependency_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class UpdateDependencyCommand extends PackageLoopingCommand {
UpdateDependencyCommand(
super.packagesDir, {
super.processRunner,
super.gitDir,
http.Client? httpClient,
}) : _pubVersionFinder =
PubVersionFinder(httpClient: httpClient ?? http.Client()) {
Expand Down
5 changes: 4 additions & 1 deletion script/tool/lib/src/update_min_sdk_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ const int _exitUnknownVersion = 3;
/// A command to update the minimum Flutter and Dart SDKs of packages.
class UpdateMinSdkCommand extends PackageLoopingCommand {
/// Creates a publish metadata updater command instance.
UpdateMinSdkCommand(super.packagesDir) {
UpdateMinSdkCommand(
super.packagesDir, {
super.gitDir,
}) {
argParser.addOption(_flutterMinFlag,
mandatory: true,
help: 'The minimum version of Flutter to set SDK constraints to.');
Expand Down
15 changes: 1 addition & 14 deletions script/tool/lib/src/update_release_info_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import 'package:file/file.dart';
import 'package:pub_semver/pub_semver.dart';
import 'package:yaml_edit/yaml_edit.dart';

import 'common/git_version_finder.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/package_state_utils.dart';
Expand Down Expand Up @@ -76,11 +75,6 @@ class UpdateReleaseInfoCommand extends PackageLoopingCommand {
// If null, either there is no version change, or it is dynamic (`minimal`).
_VersionIncrementType? _versionChange;

// The cache of changed files, for dynamic version change determination.
//
// Only set for `minimal` version change.
late final List<String> _changedFiles;

@override
final String name = 'update-release-info';

Expand All @@ -103,12 +97,6 @@ class UpdateReleaseInfoCommand extends PackageLoopingCommand {
case _versionBugfix:
_versionChange = _VersionIncrementType.bugfix;
case _versionMinimal:
final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
// If the line below fails with "Not a valid object name FETCH_HEAD"
// run "git fetch", FETCH_HEAD is a temporary reference that only exists
// after a fetch. This can happen when a branch is made locally and
// pushed but never fetched.
_changedFiles = await gitVersionFinder.getChangedFiles();
// Anothing other than a fixed change is null.
_versionChange = null;
case _versionNext:
Expand All @@ -133,8 +121,7 @@ class UpdateReleaseInfoCommand extends PackageLoopingCommand {
final String relativePackagePath =
getRelativePosixPath(package.directory, from: gitRoot);
final PackageChangeState state = await checkPackageChangeState(package,
changedPaths: _changedFiles,
relativePackagePath: relativePackagePath);
changedPaths: changedFiles, relativePackagePath: relativePackagePath);

if (!state.hasChanges) {
return PackageResult.skip('No changes to package');
Expand Down
12 changes: 4 additions & 8 deletions script/tool/lib/src/version_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,6 @@ class VersionCheckCommand extends PackageLoopingCommand {
final PubVersionFinder _pubVersionFinder;

late final GitVersionFinder _gitVersionFinder;
late final String _mergeBase;
late final List<String> _changedFiles;

late final Set<String> _prLabels = _getPRLabels();

Expand All @@ -177,8 +175,6 @@ class VersionCheckCommand extends PackageLoopingCommand {
@override
Future<void> initializeRun() async {
_gitVersionFinder = await retrieveVersionFinder();
_mergeBase = await _gitVersionFinder.getBaseSha();
_changedFiles = await _gitVersionFinder.getChangedFiles();
}

@override
Expand Down Expand Up @@ -279,7 +275,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
final String gitPath = path.style == p.Style.windows
? p.posix.joinAll(path.split(relativePath))
: relativePath;
return _gitVersionFinder.getPackageVersion(gitPath, gitRef: _mergeBase);
return _gitVersionFinder.getPackageVersion(gitPath, gitRef: baseSha);
}

/// Returns the state of the verison of [package] relative to the comparison
Expand All @@ -303,7 +299,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
'$indentation${pubspec.name}: Current largest version on pub: $previousVersion');
}
} else {
previousVersionSource = _mergeBase;
previousVersionSource = baseSha;
previousVersion =
await _getPreviousVersionFromGit(package) ?? Version.none;
}
Expand Down Expand Up @@ -535,15 +531,15 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
/// This should only be called if the version did not change.
Future<String?> _checkForMissingChangeError(RepositoryPackage package) async {
// Find the relative path to the current package, as it would appear at the
// beginning of a path reported by getChangedFiles() (which always uses
// beginning of a path reported by changedFiles (which always uses
// Posix paths).
final Directory gitRoot =
packagesDir.fileSystem.directory((await gitDir).path);
final String relativePackagePath =
getRelativePosixPath(package.directory, from: gitRoot);

final PackageChangeState state = await checkPackageChangeState(package,
changedPaths: _changedFiles,
changedPaths: changedFiles,
relativePackagePath: relativePackagePath,
git: await retrieveVersionFinder());

Expand Down
1 change: 1 addition & 0 deletions script/tool/lib/src/xcode_analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class XcodeAnalyzeCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
super.gitDir,
}) : _xcode = Xcode(processRunner: processRunner, log: true) {
argParser.addFlag(platformIOS, help: 'Analyze iOS');
argParser.addFlag(platformMacOS, help: 'Analyze macOS');
Expand Down
10 changes: 5 additions & 5 deletions script/tool/test/analyze_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,29 @@

import 'package:args/command_runner.dart';
import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:flutter_plugin_tools/src/analyze_command.dart';
import 'package:flutter_plugin_tools/src/common/core.dart';
import 'package:git/git.dart';
import 'package:test/test.dart';

import 'mocks.dart';
import 'util.dart';

void main() {
late FileSystem fileSystem;
late MockPlatform mockPlatform;
late Directory packagesDir;
late RecordingProcessRunner processRunner;
late CommandRunner<void> runner;

setUp(() {
fileSystem = MemoryFileSystem();
mockPlatform = MockPlatform();
packagesDir = createPackagesDirectory(fileSystem: fileSystem);
processRunner = RecordingProcessRunner();
final GitDir gitDir;
(:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) =
configureBaseCommandMocks(platform: mockPlatform);
final AnalyzeCommand analyzeCommand = AnalyzeCommand(
packagesDir,
processRunner: processRunner,
gitDir: gitDir,
platform: mockPlatform,
);

Expand Down
Loading

0 comments on commit 571e1af

Please sign in to comment.