From b9f601cc7b2e70168ea9087c1531e0a323e18baf Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Fri, 2 Jul 2021 07:50:24 -0700 Subject: [PATCH] [flutter_plugin_tools] Remove most exitOnError:true usage (#4130) The intent of package-looping commands is that we always want to run them for all the targeted packages, accumulating failures, and then report them all at the end, so we don't end up with the problem of only finding errors one at a time. However, some of them were using `exitOnError: true` for the underlying commands, causing the first error to be fatal to the test. This PR: - Removes all use of `exitOnError: true` from the package-looping commands. (It's still used in `format`, but fixing that is a larger issue, and less important due to the way `format` is currently structured.) - Fixes the mock process runner used in our tests to correctly simulate `exitOrError: true`; the fact that it didn't was hiding this problem in test (e.g., `drive-examples` had a test that asserted that all failures were summarized correctly, which passed because in tests it was behaving as if `exitOnError` were false) - Adjusts the mock process runner to allow setting a list of mock result for a specific executable instead of one result for all calls to anything, in order to fix some tests that were broken by the two changes above and unfixable without this (e.g., a test of a command that had been calling one executable with `exitOnError: true` then another with ``exitOnError: false`, where the test was asserting things about the second call failing, which only worked because the first call's failure wasn't actually checked). To limit the scope of the PR, the old method of setting a single result for all calls is still supported for now as a fallback. - Fixes the fact that the mock `run` and `runAndStream` had opposite default exit code behavior when no mock process was set (since that caused me a lot of confusion while fixing the above until I figured it out). --- script/tool/CHANGELOG.md | 2 + script/tool/lib/src/analyze_command.dart | 17 ++++++-- .../tool/lib/src/drive_examples_command.dart | 5 +-- .../lib/src/firebase_test_lab_command.dart | 9 +++- .../tool/lib/src/lint_podspecs_command.dart | 8 +++- .../tool/lib/src/publish_plugin_command.dart | 7 ++-- script/tool/lib/src/xctest_command.dart | 2 +- .../test/drive_examples_command_test.dart | 32 ++++++++++++++- .../test/firebase_test_lab_command_test.dart | 31 +++++++++++++- .../tool/test/lint_podspecs_command_test.dart | 34 ++++++++++++++- script/tool/test/util.dart | 41 +++++++++++++++---- 11 files changed, 160 insertions(+), 28 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index d64655f5b8..db9ecf4930 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -5,6 +5,8 @@ files, only `integration_test/*_test.dart`. - Add a summary to the end of successful command runs for commands using the new output format. +- Fixed some cases where a failure in a command for a single package would + immediately abort the test. ## 0.3.0 diff --git a/script/tool/lib/src/analyze_command.dart b/script/tool/lib/src/analyze_command.dart index 29e78f3ea4..adaeb1e616 100644 --- a/script/tool/lib/src/analyze_command.dart +++ b/script/tool/lib/src/analyze_command.dart @@ -12,6 +12,7 @@ import 'common/package_looping_command.dart'; import 'common/process_runner.dart'; const int _exitBadCustomAnalysisFile = 2; +const int _exitPackagesGetFailed = 3; /// A command to run Dart analysis on packages. class AnalyzeCommand extends PackageLoopingCommand { @@ -75,7 +76,7 @@ class AnalyzeCommand extends PackageLoopingCommand { /// Ensures that the dependent packages have been fetched for all packages /// (including their sub-packages) that will be analyzed. - Future _runPackagesGetOnTargetPackages() async { + Future _runPackagesGetOnTargetPackages() async { final List packageDirectories = await getPackages().toList(); final Set packagePaths = packageDirectories.map((Directory dir) => dir.path).toSet(); @@ -87,9 +88,14 @@ class AnalyzeCommand extends PackageLoopingCommand { packagePaths.contains(directory.parent.path); }); for (final Directory package in packageDirectories) { - await processRunner.runAndStream('flutter', ['packages', 'get'], - workingDir: package, exitOnError: true); + final int exitCode = await processRunner.runAndStream( + 'flutter', ['packages', 'get'], + workingDir: package); + if (exitCode != 0) { + return false; + } } + return true; } @override @@ -98,7 +104,10 @@ class AnalyzeCommand extends PackageLoopingCommand { _validateAnalysisOptions(); print('Fetching dependencies...'); - await _runPackagesGetOnTargetPackages(); + if (!await _runPackagesGetOnTargetPackages()) { + printError('Unabled to get dependencies.'); + throw ToolExit(_exitPackagesGetFailed); + } // Use the Dart SDK override if one was passed in. final String? dartSdk = argResults![_analysisSdk] as String?; diff --git a/script/tool/lib/src/drive_examples_command.dart b/script/tool/lib/src/drive_examples_command.dart index dc9774d846..1e19535f4a 100644 --- a/script/tool/lib/src/drive_examples_command.dart +++ b/script/tool/lib/src/drive_examples_command.dart @@ -203,7 +203,7 @@ class DriveExamplesCommand extends PackageLoopingCommand { final ProcessResult result = await processRunner.run( flutterCommand, ['devices', '--machine'], - stdoutEncoding: utf8, exitOnError: true); + stdoutEncoding: utf8); if (result.exitCode != 0) { return deviceIds; } @@ -295,8 +295,7 @@ class DriveExamplesCommand extends PackageLoopingCommand { '--target', p.relative(target.path, from: example.path), ], - workingDir: example, - exitOnError: true); + workingDir: example); if (exitCode != 0) { failures.add(target); } diff --git a/script/tool/lib/src/firebase_test_lab_command.dart b/script/tool/lib/src/firebase_test_lab_command.dart index 8d1ba995f1..3d68c85fbd 100644 --- a/script/tool/lib/src/firebase_test_lab_command.dart +++ b/script/tool/lib/src/firebase_test_lab_command.dart @@ -13,6 +13,8 @@ import 'common/core.dart'; import 'common/package_looping_command.dart'; import 'common/process_runner.dart'; +const int _exitGcloudAuthFailed = 2; + /// A command to run tests via Firebase test lab. class FirebaseTestLabCommand extends PackageLoopingCommand { /// Creates an instance of the test runner command. @@ -84,16 +86,19 @@ class FirebaseTestLabCommand extends PackageLoopingCommand { if (serviceKey.isEmpty) { print('No --service-key provided; skipping gcloud authorization'); } else { - await processRunner.run( + final io.ProcessResult result = await processRunner.run( 'gcloud', [ 'auth', 'activate-service-account', '--key-file=$serviceKey', ], - exitOnError: true, logOnError: true, ); + if (result.exitCode != 0) { + printError('Unable to activate gcloud account.'); + throw ToolExit(_exitGcloudAuthFailed); + } final int exitCode = await processRunner.runAndStream('gcloud', [ 'config', 'set', diff --git a/script/tool/lib/src/lint_podspecs_command.dart b/script/tool/lib/src/lint_podspecs_command.dart index 0bdb7c972c..82cce0bd13 100644 --- a/script/tool/lib/src/lint_podspecs_command.dart +++ b/script/tool/lib/src/lint_podspecs_command.dart @@ -14,6 +14,7 @@ import 'common/package_looping_command.dart'; import 'common/process_runner.dart'; const int _exitUnsupportedPlatform = 2; +const int _exitPodNotInstalled = 3; /// Lint the CocoaPod podspecs and run unit tests. /// @@ -53,13 +54,16 @@ class LintPodspecsCommand extends PackageLoopingCommand { throw ToolExit(_exitUnsupportedPlatform); } - await processRunner.run( + final ProcessResult result = await processRunner.run( 'which', ['pod'], workingDir: packagesDir, - exitOnError: true, logOnError: true, ); + if (result.exitCode != 0) { + printError('Unable to find "pod". Make sure it is in your path.'); + throw ToolExit(_exitPodNotInstalled); + } } @override diff --git a/script/tool/lib/src/publish_plugin_command.dart b/script/tool/lib/src/publish_plugin_command.dart index 740178829c..6de53ba269 100644 --- a/script/tool/lib/src/publish_plugin_command.dart +++ b/script/tool/lib/src/publish_plugin_command.dart @@ -355,7 +355,6 @@ Safe to ignore if the package is deleted in this commit. 'git', ['tag', tag], workingDir: packageDir, - exitOnError: false, logOnError: true, ); if (result.exitCode != 0) { @@ -402,7 +401,6 @@ Safe to ignore if the package is deleted in this commit. ['status', '--porcelain', '--ignored', packageDir.absolute.path], workingDir: packageDir, logOnError: true, - exitOnError: false, ); if (statusResult.exitCode != 0) { return false; @@ -423,9 +421,11 @@ Safe to ignore if the package is deleted in this commit. 'git', ['remote', 'get-url', remote], workingDir: packagesDir, - exitOnError: true, logOnError: true, ); + if (getRemoteUrlResult.exitCode != 0) { + return null; + } return getRemoteUrlResult.stdout as String?; } @@ -498,7 +498,6 @@ Safe to ignore if the package is deleted in this commit. 'git', ['push', remote.name, tag], workingDir: packagesDir, - exitOnError: false, logOnError: true, ); if (result.exitCode != 0) { diff --git a/script/tool/lib/src/xctest_command.dart b/script/tool/lib/src/xctest_command.dart index c7a454ab75..cd3b674f8d 100644 --- a/script/tool/lib/src/xctest_command.dart +++ b/script/tool/lib/src/xctest_command.dart @@ -184,7 +184,7 @@ class XCTestCommand extends PackageLoopingCommand { '$_kXCRunCommand ${xctestArgs.join(' ')}'; print(completeTestCommand); return processRunner.runAndStream(_kXCRunCommand, xctestArgs, - workingDir: example, exitOnError: false); + workingDir: example); } Future _findAvailableIphoneSimulator() async { diff --git a/script/tool/test/drive_examples_command_test.dart b/script/tool/test/drive_examples_command_test.dart index 97a39c6b2b..eeac96e56e 100644 --- a/script/tool/test/drive_examples_command_test.dart +++ b/script/tool/test/drive_examples_command_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' as io; + import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; @@ -53,7 +55,9 @@ void main() { final MockProcess mockDevicesProcess = MockProcess(); mockDevicesProcess.exitCodeCompleter.complete(0); mockDevicesProcess.stdoutController.close(); // ignore: unawaited_futures - processRunner.processToReturn = mockDevicesProcess; + processRunner.mockProcessesForExecutable['flutter'] = [ + mockDevicesProcess + ]; processRunner.resultStdout = output; } @@ -110,7 +114,31 @@ void main() { ); }); - test('fails if Android if no Android devices are present', () async { + test('fails for iOS if getting devices fails', () async { + setMockFlutterDevicesOutput(hasIosDevice: false); + + // Simulate failure from `flutter devices`. + final MockProcess mockProcess = MockProcess(); + mockProcess.exitCodeCompleter.complete(1); + processRunner.processToReturn = mockProcess; + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['drive-examples', '--ios'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('No iOS devices'), + ]), + ); + }); + + test('fails for Android if no Android devices are present', () async { + setMockFlutterDevicesOutput(hasAndroidDevice: false); Error? commandError; final List output = await runCapturingPrint( runner, ['drive-examples', '--android'], diff --git a/script/tool/test/firebase_test_lab_command_test.dart b/script/tool/test/firebase_test_lab_command_test.dart index 4285a0fee3..711c383f2d 100644 --- a/script/tool/test/firebase_test_lab_command_test.dart +++ b/script/tool/test/firebase_test_lab_command_test.dart @@ -33,10 +33,12 @@ void main() { runner.addCommand(command); }); - test('retries gcloud set', () async { + test('fails if gcloud auth fails', () async { final MockProcess mockProcess = MockProcess(); mockProcess.exitCodeCompleter.complete(1); - processRunner.processToReturn = mockProcess; + processRunner.mockProcessesForExecutable['gcloud'] = [ + mockProcess + ]; createFakePlugin('plugin', packagesDir, extraFiles: [ 'example/integration_test/foo_test.dart', 'example/android/gradlew', @@ -50,6 +52,31 @@ void main() { }); expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Unable to activate gcloud account.'), + ])); + }); + + test('retries gcloud set', () async { + final MockProcess mockAuthProcess = MockProcess(); + mockAuthProcess.exitCodeCompleter.complete(0); + final MockProcess mockConfigProcess = MockProcess(); + mockConfigProcess.exitCodeCompleter.complete(1); + processRunner.mockProcessesForExecutable['gcloud'] = [ + mockAuthProcess, + mockConfigProcess, + ]; + createFakePlugin('plugin', packagesDir, extraFiles: [ + 'example/integration_test/foo_test.dart', + 'example/android/gradlew', + 'example/android/app/src/androidTest/MainActivityTest.java', + ]); + + final List output = + await runCapturingPrint(runner, ['firebase-test-lab']); + expect( output, containsAllInOrder([ diff --git a/script/tool/test/lint_podspecs_command_test.dart b/script/tool/test/lint_podspecs_command_test.dart index 90a662d750..c61d6a9d92 100644 --- a/script/tool/test/lint_podspecs_command_test.dart +++ b/script/tool/test/lint_podspecs_command_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' as io; + import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; @@ -160,6 +162,34 @@ void main() { expect(output, contains('Linting plugin1.podspec')); }); + test('fails if pod is missing', () async { + createFakePlugin('plugin1', packagesDir, + extraFiles: ['plugin1.podspec']); + + // Simulate failure from `which pod`. + final MockProcess mockWhichProcess = MockProcess(); + mockWhichProcess.exitCodeCompleter.complete(1); + processRunner.mockProcessesForExecutable['which'] = [ + mockWhichProcess + ]; + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['podspecs'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + + expect( + output, + containsAllInOrder( + [ + contains('Unable to find "pod". Make sure it is in your path.'), + ], + )); + }); + test('fails if linting fails', () async { createFakePlugin('plugin1', packagesDir, extraFiles: ['plugin1.podspec']); @@ -167,7 +197,9 @@ void main() { // Simulate failure from `pod`. final MockProcess mockDriveProcess = MockProcess(); mockDriveProcess.exitCodeCompleter.complete(1); - processRunner.processToReturn = mockDriveProcess; + processRunner.mockProcessesForExecutable['pod'] = [ + mockDriveProcess + ]; Error? commandError; final List output = await runCapturingPrint( diff --git a/script/tool/test/util.dart b/script/tool/test/util.dart index e71a26fa4e..b65b1fcaa8 100644 --- a/script/tool/test/util.dart +++ b/script/tool/test/util.dart @@ -252,15 +252,22 @@ Future> runCapturingPrint( /// A mock [ProcessRunner] which records process calls. class RecordingProcessRunner extends ProcessRunner { - io.Process? processToReturn; final List recordedCalls = []; + /// Maps an executable to a list of processes that should be used for each + /// successive call to it via [run], [runAndStream], or [start]. + final Map> mockProcessesForExecutable = + >{}; + /// Populate for [io.ProcessResult] to use a String [stdout] instead of a [List] of [int]. String? resultStdout; /// Populate for [io.ProcessResult] to use a String [stderr] instead of a [List] of [int]. String? resultStderr; + // Deprecated--do not add new uses. Use mockProcessesForExecutable instead. + io.Process? processToReturn; + @override Future runAndStream( String executable, @@ -269,11 +276,17 @@ class RecordingProcessRunner extends ProcessRunner { bool exitOnError = false, }) async { recordedCalls.add(ProcessCall(executable, args, workingDir?.path)); - return Future.value( - processToReturn == null ? 0 : await processToReturn!.exitCode); + final io.Process? processToReturn = _getProcessToReturn(executable); + final int exitCode = + processToReturn == null ? 0 : await processToReturn.exitCode; + if (exitOnError && (exitCode != 0)) { + throw io.ProcessException(executable, args); + } + return Future.value(exitCode); } - /// Returns [io.ProcessResult] created from [processToReturn], [resultStdout], and [resultStderr]. + /// Returns [io.ProcessResult] created from [mockProcessesForExecutable], + /// [resultStdout], and [resultStderr]. @override Future run( String executable, @@ -286,12 +299,16 @@ class RecordingProcessRunner extends ProcessRunner { }) async { recordedCalls.add(ProcessCall(executable, args, workingDir?.path)); - final io.Process? process = processToReturn; + final io.Process? process = _getProcessToReturn(executable); final io.ProcessResult result = process == null - ? io.ProcessResult(1, 1, '', '') + ? io.ProcessResult(1, 0, '', '') : io.ProcessResult(process.pid, await process.exitCode, resultStdout ?? process.stdout, resultStderr ?? process.stderr); + if (exitOnError && (result.exitCode != 0)) { + throw io.ProcessException(executable, args); + } + return Future.value(result); } @@ -299,7 +316,17 @@ class RecordingProcessRunner extends ProcessRunner { Future start(String executable, List args, {Directory? workingDirectory}) async { recordedCalls.add(ProcessCall(executable, args, workingDirectory?.path)); - return Future.value(processToReturn); + return Future.value(_getProcessToReturn(executable)); + } + + io.Process? _getProcessToReturn(String executable) { + io.Process? process; + final List? processes = mockProcessesForExecutable[executable]; + if (processes != null && processes.isNotEmpty) { + process = mockProcessesForExecutable[executable]!.removeAt(0); + } + // Fall back to `processToReturn` for backwards compatibility. + return process ?? processToReturn; } }