From 7d1fbcae1aad78ee605dcb2762b6128cc99898b5 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 20 Mar 2020 13:05:19 -0700 Subject: [PATCH] Refactor exits happy (#52916) --- .../flutter_tools/lib/src/base/process.dart | 16 ++++--- packages/flutter_tools/lib/src/ios/mac.dart | 13 +----- .../test/general.shard/base/process_test.dart | 46 ++++++++++++++----- .../general.shard/ios/code_signing_test.dart | 2 + .../general.shard/macos/cocoapods_test.dart | 9 ++++ 5 files changed, 56 insertions(+), 30 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index 3ceff0cdff..ea4e7efe77 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/packages/flutter_tools/lib/src/base/process.dart @@ -537,14 +537,16 @@ class _DefaultProcessUtils implements ProcessUtils { Map environment, }) { _traceCommand(cli); + if (!_processManager.canRun(cli.first)) { + _logger.printTrace('$cli either does not exist or is not executable.'); + return false; + } + try { return _processManager.runSync(cli, environment: environment).exitCode == 0; } on Exception catch (error) { _logger.printTrace('$cli failed with $error'); return false; - } on ArgumentError catch (error) { - _logger.printTrace('$cli failed with $error'); - return false; } } @@ -554,14 +556,16 @@ class _DefaultProcessUtils implements ProcessUtils { Map environment, }) async { _traceCommand(cli); + if (!_processManager.canRun(cli.first)) { + _logger.printTrace('$cli either does not exist or is not executable.'); + return false; + } + try { return (await _processManager.run(cli, environment: environment)).exitCode == 0; } on Exception catch (error) { _logger.printTrace('$cli failed with $error'); return false; - } on ArgumentError catch (error) { - _logger.printTrace('$cli failed with $error'); - return false; } } diff --git a/packages/flutter_tools/lib/src/ios/mac.dart b/packages/flutter_tools/lib/src/ios/mac.dart index f506acdeea..7bdfee0341 100644 --- a/packages/flutter_tools/lib/src/ios/mac.dart +++ b/packages/flutter_tools/lib/src/ios/mac.dart @@ -35,18 +35,7 @@ class IMobileDevice { final String _idevicesyslogPath; final String _idevicescreenshotPath; - bool get isInstalled { - _isInstalled ??= processUtils.exitsHappySync( - [ - _idevicescreenshotPath, - '-h', - ], - environment: Map.fromEntries( - >[globals.cache.dyLdLibEntry] - ), - ); - return _isInstalled; - } + bool get isInstalled => _isInstalled ??= globals.processManager.canRun(_idevicescreenshotPath); bool _isInstalled; /// Starts `idevicesyslog` and returns the running process. diff --git a/packages/flutter_tools/test/general.shard/base/process_test.dart b/packages/flutter_tools/test/general.shard/base/process_test.dart index 7d5b17a5f2..27569b103d 100644 --- a/packages/flutter_tools/test/general.shard/base/process_test.dart +++ b/packages/flutter_tools/test/general.shard/base/process_test.dart @@ -336,7 +336,7 @@ void main() { }); group('exitsHappySync', () { - ProcessManager mockProcessManager; + MockProcessManager mockProcessManager; ProcessUtils processUtils; setUp(() { @@ -368,16 +368,27 @@ void main() { expect(processUtils.exitsHappySync(['boohoo']), isFalse); }); - testWithoutContext('catches ArgumentError and returns false', () { - when(mockProcessManager.runSync(['nonesuch'])).thenThrow( - ArgumentError('Invalid argument(s): Cannot find executable for nonesuch') - ); + testWithoutContext('does not throw Exception and returns false if binary cannot run', () { + mockProcessManager.canRunSucceeds = false; expect(processUtils.exitsHappySync(['nonesuch']), isFalse); + verifyNever( + mockProcessManager.runSync(any, environment: anyNamed('environment')), + ); + }); + + testWithoutContext('does not catch ArgumentError', () async { + when(mockProcessManager.runSync(['invalid'])).thenThrow( + ArgumentError('Bad input'), + ); + expect( + () => processUtils.exitsHappySync(['invalid']), + throwsArgumentError, + ); }); }); group('exitsHappy', () { - ProcessManager mockProcessManager; + MockProcessManager mockProcessManager; ProcessUtils processUtils; setUp(() { @@ -388,14 +399,14 @@ void main() { ); }); - testWithoutContext(' succeeds on success', () async { + testWithoutContext('succeeds on success', () async { when(mockProcessManager.run(['whoohoo'])).thenAnswer((_) { return Future.value(ProcessResult(0, 0, '', '')); }); expect(await processUtils.exitsHappy(['whoohoo']), isTrue); }); - testWithoutContext(' fails on failure', () async { + testWithoutContext('fails on failure', () async { when(mockProcessManager.run(['boohoo'])).thenAnswer((_) { return Future.value(ProcessResult(0, 1, '', '')); }); @@ -409,11 +420,22 @@ void main() { expect(await processUtils.exitsHappy(['boohoo']), isFalse); }); - testWithoutContext('catches ArgumentError and returns false', () async { - when(mockProcessManager.run(['nonesuch'])).thenThrow( - ArgumentError('Invalid argument(s): Cannot find executable for nonesuch'), - ); + testWithoutContext('does not throw Exception and returns false if binary cannot run', () async { + mockProcessManager.canRunSucceeds = false; expect(await processUtils.exitsHappy(['nonesuch']), isFalse); + verifyNever( + mockProcessManager.runSync(any, environment: anyNamed('environment')), + ); + }); + + testWithoutContext('does not catch ArgumentError', () async { + when(mockProcessManager.run(['invalid'])).thenThrow( + ArgumentError('Bad input'), + ); + expect( + () async => await processUtils.exitsHappy(['invalid']), + throwsArgumentError, + ); }); }); diff --git a/packages/flutter_tools/test/general.shard/ios/code_signing_test.dart b/packages/flutter_tools/test/general.shard/ios/code_signing_test.dart index f00d5aa77b..60e765019c 100644 --- a/packages/flutter_tools/test/general.shard/ios/code_signing_test.dart +++ b/packages/flutter_tools/test/general.shard/ios/code_signing_test.dart @@ -30,6 +30,8 @@ void main() { setUp(() async { mockProcessManager = MockProcessManager(); + // Assume all binaries exist and are executable + when(mockProcessManager.canRun(any)).thenReturn(true); mockConfig = MockConfig(); mockIosProject = MockIosProject(); when(mockIosProject.buildSettings).thenAnswer((_) { diff --git a/packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart b/packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart index b1e78a48e9..fbba453176 100644 --- a/packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart +++ b/packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart @@ -134,6 +134,11 @@ void main() { } group('Evaluate installation', () { + setUp(() { + // Assume all binaries can run + when(mockProcessManager.canRun(any)).thenReturn(true); + }); + testUsingContext('detects not installed, if pod exec does not exist', () async { pretendPodIsNotInstalled(); expect(await cocoaPodsUnderTest.evaluateCocoaPodsInstallation, CocoaPodsStatus.notInstalled); @@ -328,6 +333,8 @@ void main() { group('Process pods', () { setUp(() { podsIsInHomeDir(); + // Assume all binaries can run + when(mockProcessManager.canRun(any)).thenReturn(true); }); testUsingContext('throwsToolExit if CocoaPods is not installed', () async { @@ -674,6 +681,8 @@ Note: as of CocoaPods 1.0, `pod repo update` does not happen on `pod install` by String cocoapodsRepoDir; Map environment; setUp(() { + // Assume binaries exist and can run + when(mockProcessManager.canRun(any)).thenReturn(true); cocoapodsRepoDir = podsIsInCustomDir(); environment = { 'FLUTTER_FRAMEWORK_DIR': 'engine/path',