Skip to content

Commit 1199f13

Browse files
[flutter_plugin_tools] Make unit tests pass on Windows (#4149)
The purpose of this PR is to make running all unit tests on Windows pass (vs failing a large portion of the tests as currently happens). This does not mean that the commands actually work when run on Windows, or that Windows support is tested, only that it's possible to actually run the tests themselves. This is prep for actually supporting parts of the tool on Windows in future PRs. Major changes: - Make the tests significantly more hermetic: - Make almost all tools take a `Platform` constructor argument that can be used to inject a mock platform to control what OS the command acts like it is running on under test. - Add a path `Context` object to the base command, whose style matches the `Platform`, and use that almost everywhere instead of the top-level `path` functions. - In cases where Posix behavior is always required (such as parsing `git` output), explicitly use the `posix` context object for `path` functions. - Start laying the groundwork for actual Windows support: - Replace all uses of `flutter` as a command with a getter that returns `flutter` or `flutter.bat` as appropriate. - For user messages that include relative paths, use a helper that always uses Posix-style relative paths for consistent output. This bumps the version since quite a few changes have built up, and having a cut point before starting to make more changes to the commands to support Windows seems like a good idea. Part of #86113
1 parent 731b93d commit 1199f13

36 files changed

+428
-250
lines changed

script/tool/lib/src/analyze_command.dart

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import 'dart:async';
66

77
import 'package:file/file.dart';
8-
import 'package:path/path.dart' as p;
8+
import 'package:platform/platform.dart';
99

1010
import 'common/core.dart';
1111
import 'common/package_looping_command.dart';
@@ -19,7 +19,8 @@ class AnalyzeCommand extends PackageLoopingCommand {
1919
AnalyzeCommand(
2020
Directory packagesDir, {
2121
ProcessRunner processRunner = const ProcessRunner(),
22-
}) : super(packagesDir, processRunner: processRunner) {
22+
Platform platform = const LocalPlatform(),
23+
}) : super(packagesDir, processRunner: processRunner, platform: platform) {
2324
argParser.addMultiOption(_customAnalysisFlag,
2425
help:
2526
'Directories (comma separated) that are allowed to have their own analysis options.',
@@ -57,9 +58,8 @@ class AnalyzeCommand extends PackageLoopingCommand {
5758

5859
final bool allowed = (getStringListArg(_customAnalysisFlag)).any(
5960
(String directory) =>
60-
directory != null &&
6161
directory.isNotEmpty &&
62-
p.isWithin(
62+
path.isWithin(
6363
packagesDir.childDirectory(directory).path, file.path));
6464
if (allowed) {
6565
continue;
@@ -90,7 +90,7 @@ class AnalyzeCommand extends PackageLoopingCommand {
9090
});
9191
for (final Directory package in packageDirectories) {
9292
final int exitCode = await processRunner.runAndStream(
93-
'flutter', <String>['packages', 'get'],
93+
flutterCommand, <String>['packages', 'get'],
9494
workingDir: package);
9595
if (exitCode != 0) {
9696
return false;
@@ -109,7 +109,8 @@ class AnalyzeCommand extends PackageLoopingCommand {
109109

110110
// Use the Dart SDK override if one was passed in.
111111
final String? dartSdk = argResults![_analysisSdk] as String?;
112-
_dartBinaryPath = dartSdk == null ? 'dart' : p.join(dartSdk, 'bin', 'dart');
112+
_dartBinaryPath =
113+
dartSdk == null ? 'dart' : path.join(dartSdk, 'bin', 'dart');
113114
}
114115

115116
@override

script/tool/lib/src/build_examples_command.dart

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import 'dart:async';
66

77
import 'package:file/file.dart';
8-
import 'package:path/path.dart' as p;
8+
import 'package:platform/platform.dart';
99

1010
import 'common/core.dart';
1111
import 'common/package_looping_command.dart';
@@ -23,7 +23,8 @@ class BuildExamplesCommand extends PackageLoopingCommand {
2323
BuildExamplesCommand(
2424
Directory packagesDir, {
2525
ProcessRunner processRunner = const ProcessRunner(),
26-
}) : super(packagesDir, processRunner: processRunner) {
26+
Platform platform = const LocalPlatform(),
27+
}) : super(packagesDir, processRunner: processRunner, platform: platform) {
2728
argParser.addFlag(kPlatformLinux);
2829
argParser.addFlag(kPlatformMacos);
2930
argParser.addFlag(kPlatformWeb);
@@ -127,7 +128,7 @@ class BuildExamplesCommand extends PackageLoopingCommand {
127128

128129
for (final Directory example in getExamplesForPlugin(package)) {
129130
final String packageName =
130-
p.relative(example.path, from: packagesDir.path);
131+
getRelativePosixPath(example, from: packagesDir);
131132

132133
for (final _PlatformDetails platform in buildPlatforms) {
133134
String buildPlatform = platform.label;

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'package:colorize/colorize.dart';
88
import 'package:file/file.dart';
99
import 'package:git/git.dart';
1010
import 'package:path/path.dart' as p;
11+
import 'package:platform/platform.dart';
1112

1213
import 'core.dart';
1314
import 'plugin_command.dart';
@@ -63,8 +64,10 @@ abstract class PackageLoopingCommand extends PluginCommand {
6364
PackageLoopingCommand(
6465
Directory packagesDir, {
6566
ProcessRunner processRunner = const ProcessRunner(),
67+
Platform platform = const LocalPlatform(),
6668
GitDir? gitDir,
67-
}) : super(packagesDir, processRunner: processRunner, gitDir: gitDir);
69+
}) : super(packagesDir,
70+
processRunner: processRunner, platform: platform, gitDir: gitDir);
6871

6972
/// Packages that had at least one [logWarning] call.
7073
final Set<Directory> _packagesWithWarnings = <Directory>{};
@@ -158,8 +161,8 @@ abstract class PackageLoopingCommand extends PluginCommand {
158161
/// an exact format (e.g., published name, or basename) is required, that
159162
/// should be used instead.
160163
String getPackageDescription(Directory package) {
161-
String packageName = p.relative(package.path, from: packagesDir.path);
162-
final List<String> components = p.split(packageName);
164+
String packageName = getRelativePosixPath(package, from: packagesDir);
165+
final List<String> components = p.posix.split(packageName);
163166
// For the common federated plugin pattern of `foo/foo_subpackage`, drop
164167
// the first part since it's not useful.
165168
if (components.length == 2 &&
@@ -169,6 +172,16 @@ abstract class PackageLoopingCommand extends PluginCommand {
169172
return packageName;
170173
}
171174

175+
/// Returns the relative path from [from] to [entity] in Posix style.
176+
///
177+
/// This should be used when, for example, printing package-relative paths in
178+
/// status or error messages.
179+
String getRelativePosixPath(
180+
FileSystemEntity entity, {
181+
required Directory from,
182+
}) =>
183+
p.posix.joinAll(path.split(path.relative(entity.path, from: from.path)));
184+
172185
/// The suggested indentation for printed output.
173186
String get indentation => hasLongOutput ? '' : ' ';
174187

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ abstract class PluginCommand extends Command<void> {
2121
PluginCommand(
2222
this.packagesDir, {
2323
this.processRunner = const ProcessRunner(),
24+
this.platform = const LocalPlatform(),
2425
GitDir? gitDir,
2526
}) : _gitDir = gitDir {
2627
argParser.addMultiOption(
@@ -79,6 +80,11 @@ abstract class PluginCommand extends Command<void> {
7980
/// This can be overridden for testing.
8081
final ProcessRunner processRunner;
8182

83+
/// The current platform.
84+
///
85+
/// This can be overridden for testing.
86+
final Platform platform;
87+
8288
/// The git directory to use. If unset, [gitDir] populates it from the
8389
/// packages directory's enclosing repository.
8490
///
@@ -88,9 +94,11 @@ abstract class PluginCommand extends Command<void> {
8894
int? _shardIndex;
8995
int? _shardCount;
9096

97+
/// A context that matches the default for [platform].
98+
p.Context get path => platform.isWindows ? p.windows : p.posix;
99+
91100
/// The command to use when running `flutter`.
92-
String get flutterCommand =>
93-
const LocalPlatform().isWindows ? 'flutter.bat' : 'flutter';
101+
String get flutterCommand => platform.isWindows ? 'flutter.bat' : 'flutter';
94102

95103
/// The shard of the overall command execution that this instance should run.
96104
int get shardIndex {
@@ -240,9 +248,9 @@ abstract class PluginCommand extends Command<void> {
240248
// plugins under 'my_plugin'. Also match if the exact plugin is
241249
// passed.
242250
final String relativePath =
243-
p.relative(subdir.path, from: dir.path);
244-
final String packageName = p.basename(subdir.path);
245-
final String basenamePath = p.basename(entity.path);
251+
path.relative(subdir.path, from: dir.path);
252+
final String packageName = path.basename(subdir.path);
253+
final String basenamePath = path.basename(entity.path);
246254
if (!excludedPlugins.contains(basenamePath) &&
247255
!excludedPlugins.contains(packageName) &&
248256
!excludedPlugins.contains(relativePath) &&

script/tool/lib/src/create_all_plugins_app_command.dart

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import 'dart:io' as io;
66

77
import 'package:file/file.dart';
8+
import 'package:path/path.dart' as p;
89
import 'package:pub_semver/pub_semver.dart';
910
import 'package:pubspec_parse/pubspec_parse.dart';
1011

@@ -51,7 +52,7 @@ class CreateAllPluginsAppCommand extends PluginCommand {
5152

5253
Future<int> _createApp() async {
5354
final io.ProcessResult result = io.Process.runSync(
54-
'flutter',
55+
flutterCommand,
5556
<String>[
5657
'create',
5758
'--template=app',
@@ -156,7 +157,7 @@ class CreateAllPluginsAppCommand extends PluginCommand {
156157
<String, PathDependency>{};
157158

158159
await for (final Directory package in getPlugins()) {
159-
final String pluginName = package.path.split('/').last;
160+
final String pluginName = package.basename;
160161
final File pubspecFile = package.childFile('pubspec.yaml');
161162
final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
162163

@@ -172,6 +173,7 @@ class CreateAllPluginsAppCommand extends PluginCommand {
172173
### Generated file. Do not edit. Run `pub global run flutter_plugin_tools gen-pubspec` to update.
173174
name: ${pubspec.name}
174175
description: ${pubspec.description}
176+
publish_to: none
175177
176178
version: ${pubspec.version}
177179
@@ -197,7 +199,21 @@ dev_dependencies:${_pubspecMapString(pubspec.devDependencies)}
197199
buffer.write(' ${entry.key}: \n sdk: ${dep.sdk}');
198200
} else if (entry.value is PathDependency) {
199201
final PathDependency dep = entry.value as PathDependency;
200-
buffer.write(' ${entry.key}: \n path: ${dep.path}');
202+
String depPath = dep.path;
203+
if (path.style == p.Style.windows) {
204+
// Posix-style path separators are preferred in pubspec.yaml (and
205+
// using a consistent format makes unit testing simpler), so convert.
206+
final List<String> components = path.split(depPath);
207+
final String firstComponent = components.first;
208+
// path.split leaves a \ on drive components that isn't necessary,
209+
// and confuses pub, so remove it.
210+
if (firstComponent.endsWith(r':\')) {
211+
components[0] =
212+
firstComponent.substring(0, firstComponent.length - 1);
213+
}
214+
depPath = p.posix.joinAll(components);
215+
}
216+
buffer.write(' ${entry.key}: \n path: $depPath');
201217
} else {
202218
throw UnimplementedError(
203219
'Not available for type: ${entry.value.runtimeType}',

script/tool/lib/src/drive_examples_command.dart

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import 'dart:convert';
66
import 'dart:io';
77

88
import 'package:file/file.dart';
9-
import 'package:path/path.dart' as p;
9+
import 'package:platform/platform.dart';
1010

1111
import 'common/core.dart';
1212
import 'common/package_looping_command.dart';
@@ -22,7 +22,8 @@ class DriveExamplesCommand extends PackageLoopingCommand {
2222
DriveExamplesCommand(
2323
Directory packagesDir, {
2424
ProcessRunner processRunner = const ProcessRunner(),
25-
}) : super(packagesDir, processRunner: processRunner) {
25+
Platform platform = const LocalPlatform(),
26+
}) : super(packagesDir, processRunner: processRunner, platform: platform) {
2627
argParser.addFlag(kPlatformAndroid,
2728
help: 'Runs the Android implementation of the examples');
2829
argParser.addFlag(kPlatformIos,
@@ -148,7 +149,7 @@ class DriveExamplesCommand extends PackageLoopingCommand {
148149
for (final Directory example in getExamplesForPlugin(package)) {
149150
++examplesFound;
150151
final String exampleName =
151-
p.relative(example.path, from: packagesDir.path);
152+
getRelativePosixPath(example, from: packagesDir);
152153

153154
final List<File> drivers = await _getDrivers(example);
154155
if (drivers.isEmpty) {
@@ -172,11 +173,10 @@ class DriveExamplesCommand extends PackageLoopingCommand {
172173

173174
if (testTargets.isEmpty) {
174175
final String driverRelativePath =
175-
p.relative(driver.path, from: package.path);
176+
getRelativePosixPath(driver, from: package);
176177
printError(
177178
'Found $driverRelativePath, but no integration_test/*_test.dart files.');
178-
errors.add(
179-
'No test files for ${p.relative(driver.path, from: package.path)}');
179+
errors.add('No test files for $driverRelativePath');
180180
continue;
181181
}
182182

@@ -185,7 +185,7 @@ class DriveExamplesCommand extends PackageLoopingCommand {
185185
example, driver, testTargets,
186186
deviceFlags: deviceFlags);
187187
for (final File failingTarget in failingTargets) {
188-
errors.add(p.relative(failingTarget.path, from: package.path));
188+
errors.add(getRelativePosixPath(failingTarget, from: package));
189189
}
190190
}
191191
}
@@ -296,9 +296,9 @@ class DriveExamplesCommand extends PackageLoopingCommand {
296296
if (enableExperiment.isNotEmpty)
297297
'--enable-experiment=$enableExperiment',
298298
'--driver',
299-
p.relative(driver.path, from: example.path),
299+
getRelativePosixPath(driver, from: example),
300300
'--target',
301-
p.relative(target.path, from: example.path),
301+
getRelativePosixPath(target, from: example),
302302
],
303303
workingDir: example);
304304
if (exitCode != 0) {

script/tool/lib/src/firebase_test_lab_command.dart

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import 'dart:async';
66
import 'dart:io' as io;
77

88
import 'package:file/file.dart';
9-
import 'package:path/path.dart' as p;
9+
import 'package:platform/platform.dart';
1010
import 'package:uuid/uuid.dart';
1111

1212
import 'common/core.dart';
@@ -21,16 +21,18 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
2121
FirebaseTestLabCommand(
2222
Directory packagesDir, {
2323
ProcessRunner processRunner = const ProcessRunner(),
24-
}) : super(packagesDir, processRunner: processRunner) {
24+
Platform platform = const LocalPlatform(),
25+
}) : super(packagesDir, processRunner: processRunner, platform: platform) {
2526
argParser.addOption(
2627
'project',
2728
defaultsTo: 'flutter-infra',
2829
help: 'The Firebase project name.',
2930
);
3031
final String? homeDir = io.Platform.environment['HOME'];
3132
argParser.addOption('service-key',
32-
defaultsTo:
33-
homeDir == null ? null : p.join(homeDir, 'gcloud-service-key.json'),
33+
defaultsTo: homeDir == null
34+
? null
35+
: path.join(homeDir, 'gcloud-service-key.json'),
3436
help: 'The path to the service key for gcloud authentication.\n'
3537
r'If not provided, \$HOME/gcloud-service-key.json will be '
3638
r'assumed if $HOME is set.');
@@ -150,7 +152,7 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
150152
// test file's run.
151153
int resultsCounter = 0;
152154
for (final File test in _findIntegrationTestFiles(package)) {
153-
final String testName = p.relative(test.path, from: package.path);
155+
final String testName = getRelativePosixPath(test, from: package);
154156
print('Testing $testName...');
155157
if (!await _runGradle(androidDirectory, 'app:assembleDebug',
156158
testFile: test)) {
@@ -203,7 +205,7 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
203205
print('Running flutter build apk...');
204206
final String experiment = getStringArg(kEnableExperiment);
205207
final int exitCode = await processRunner.runAndStream(
206-
'flutter',
208+
flutterCommand,
207209
<String>[
208210
'build',
209211
'apk',

0 commit comments

Comments
 (0)