From b063dc6b4217223fc2e1c99df011a40d54f569de Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 16:55:00 -0400 Subject: [PATCH 1/7] feat: Add doctor validator to ensure no issues exist with Flutter install --- .../lib/src/commands/doctor_command.dart | 1 + .../shorebird_flutter_validator.dart | 68 +++++++++ .../lib/src/doctor/validators/validators.dart | 1 + .../lib/src/shorebird_paths.dart | 34 +++-- .../lib/src/shorebird_process.dart | 2 +- packages/shorebird_cli/pubspec.yaml | 1 + .../shorebird_flutter_validator_test.dart | 129 ++++++++++++++++++ 7 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart create mode 100644 packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart diff --git a/packages/shorebird_cli/lib/src/commands/doctor_command.dart b/packages/shorebird_cli/lib/src/commands/doctor_command.dart index 10ad34d64..415f32246 100644 --- a/packages/shorebird_cli/lib/src/commands/doctor_command.dart +++ b/packages/shorebird_cli/lib/src/commands/doctor_command.dart @@ -23,6 +23,7 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin { ShorebirdVersionValidator( isShorebirdVersionCurrent: isShorebirdVersionCurrent, ), + ShorebirdFlutterValidator(runProcess: runProcess), AndroidInternetPermissionValidator(), ]; } diff --git a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart new file mode 100644 index 000000000..4d243bdb8 --- /dev/null +++ b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart @@ -0,0 +1,68 @@ +import 'dart:io'; + +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/shorebird_paths.dart'; +import 'package:shorebird_cli/src/shorebird_process.dart'; + +class ShorebirdFlutterValidator extends DoctorValidator { + ShorebirdFlutterValidator({required this.runProcess}); + + final RunProcess runProcess; + + @override + String get description => 'Flutter install is correct'; + + @override + Future> validate() async { + if (!ShorebirdPaths.flutterDirectory.existsSync()) { + return [ + ValidationIssue( + severity: ValidationIssueSeverity.error, + message: + 'No Flutter directory found at ${ShorebirdPaths.flutterDirectory}', + ), + ]; + } + + if (!await _flutterDirectoryIsClean()) { + return [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: '${ShorebirdPaths.flutterDirectory} has local modifications', + ), + ]; + } + + if (!await _flutterDirectoryTracksStable()) { + return [ + ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: + '${ShorebirdPaths.flutterDirectory} is not on the "stable" branch', + ), + ]; + } + + return []; + } + + Future _flutterDirectoryIsClean() async { + final result = await runProcess( + 'git', + ['status'], + workingDirectory: ShorebirdPaths.flutterDirectory.path, + ); + return result.stdout + .toString() + .contains('nothing to commit, working tree clean'); + } + + Future _flutterDirectoryTracksStable() async { + final result = await runProcess( + 'git', + ['--no-pager', 'branch'], + workingDirectory: ShorebirdPaths.flutterDirectory.path, + ); + return result.stdout.toString().contains('* stable'); + } +} diff --git a/packages/shorebird_cli/lib/src/doctor/validators/validators.dart b/packages/shorebird_cli/lib/src/doctor/validators/validators.dart index af3bbb52f..ef136e405 100644 --- a/packages/shorebird_cli/lib/src/doctor/validators/validators.dart +++ b/packages/shorebird_cli/lib/src/doctor/validators/validators.dart @@ -1,2 +1,3 @@ export 'android_internet_permission_validator.dart'; +export 'shorebird_flutter_validator.dart'; export 'shorebird_version_validator.dart'; diff --git a/packages/shorebird_cli/lib/src/shorebird_paths.dart b/packages/shorebird_cli/lib/src/shorebird_paths.dart index db71ba055..a1cd3e785 100644 --- a/packages/shorebird_cli/lib/src/shorebird_paths.dart +++ b/packages/shorebird_cli/lib/src/shorebird_paths.dart @@ -1,21 +1,35 @@ -import 'dart:io'; +import 'dart:io' hide Platform; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; +import 'package:platform/platform.dart'; abstract class ShorebirdPaths { + @visibleForTesting + static Platform platform = const LocalPlatform(); + /// The root directory of the Shorebird install. /// /// Assumes we are running from $ROOT/bin/cache. static Directory shorebirdRoot = - File(Platform.script.toFilePath()).parent.parent.parent; + File(platform.script.toFilePath()).parent.parent.parent; + + /// The root of the Shorebird-vended Flutter git checkout. + static Directory get flutterDirectory => Directory( + p.join( + shorebirdRoot.path, + 'bin', + 'cache', + 'flutter', + ), + ); - /// Path to the Shorebird-vended Flutter binary. - static String get flutterBinaryPath => p.join( - shorebirdRoot.path, - 'bin', - 'cache', - 'flutter', - 'bin', - 'flutter', + /// The Shorebird-vended Flutter binary. + static File get flutterBinaryFile => File( + p.join( + flutterDirectory.path, + 'bin', + 'flutter', + ), ); } diff --git a/packages/shorebird_cli/lib/src/shorebird_process.dart b/packages/shorebird_cli/lib/src/shorebird_process.dart index 386ee497c..2f76d3789 100644 --- a/packages/shorebird_cli/lib/src/shorebird_process.dart +++ b/packages/shorebird_cli/lib/src/shorebird_process.dart @@ -50,7 +50,7 @@ abstract class ShorebirdProcess { static String _resolveExecutable(String executable) { if (executable == 'flutter') { - return ShorebirdPaths.flutterBinaryPath; + return ShorebirdPaths.flutterBinaryFile.path; } return executable; diff --git a/packages/shorebird_cli/pubspec.yaml b/packages/shorebird_cli/pubspec.yaml index 6b1b189ed..bd1d30b3e 100644 --- a/packages/shorebird_cli/pubspec.yaml +++ b/packages/shorebird_cli/pubspec.yaml @@ -23,6 +23,7 @@ dependencies: mason_logger: ^0.2.4 meta: ^1.9.0 path: ^1.8.3 + platform: ^3.1.0 pubspec_parse: ^1.2.2 shorebird_code_push_client: path: ../shorebird_code_push_client diff --git a/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart new file mode 100644 index 000000000..2d86ad8bc --- /dev/null +++ b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart @@ -0,0 +1,129 @@ +import 'dart:io' hide Platform; + +import 'package:collection/collection.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:path/path.dart' as p; +import 'package:platform/platform.dart'; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/doctor/validators/validators.dart'; +import 'package:shorebird_cli/src/shorebird_paths.dart'; +import 'package:test/test.dart'; + +class _MockProcessResult extends Mock implements ProcessResult {} + +class _MockPlatform extends Mock implements Platform {} + +void main() { + group('ShorebirdFlutterValidator', () { + const gitStatusMessage = """ +On branch stable +Your branch is up to date with 'origin/stable'. + +nothing to commit, working tree clean +"""; + + const gitBranchMessage = ''' + main +* stable +'''; + + late ShorebirdFlutterValidator validator; + late Directory tempDir; + late ProcessResult gitBranchProcessResult; + late ProcessResult gitStatusProcessResult; + + Directory flutterDirectory(Directory root) => + Directory(p.join(root.path, 'bin', 'cache', 'flutter')); + + File shorebirdScriptFile(Directory root) => + File(p.join(root.path, 'bin', 'cache', 'shorebird.snapshot')); + + Directory setupTempDirectory() { + final tempDir = Directory.systemTemp.createTempSync(); + shorebirdScriptFile(tempDir).createSync(recursive: true); + flutterDirectory(tempDir).createSync(recursive: true); + return tempDir; + } + + setUp(() { + tempDir = setupTempDirectory(); + + ShorebirdPaths.platform = _MockPlatform(); + when(() => ShorebirdPaths.platform.script) + .thenReturn(shorebirdScriptFile(tempDir).uri); + + gitBranchProcessResult = _MockProcessResult(); + gitStatusProcessResult = _MockProcessResult(); + + validator = ShorebirdFlutterValidator( + runProcess: ( + executable, + arguments, { + bool runInShell = false, + workingDirectory, + }) async { + if (executable == 'git') { + if (arguments.equals(['status'])) { + return gitStatusProcessResult; + } else if (arguments.equals(['--no-pager', 'branch'])) { + return gitBranchProcessResult; + } + } + return _MockProcessResult(); + }, + ); + + when(() => gitBranchProcessResult.stdout).thenReturn(gitBranchMessage); + when(() => gitStatusProcessResult.stdout).thenReturn(gitStatusMessage); + }); + + test('returns no issues when the Flutter install is good', () async { + final results = await IOOverrides.runZoned( + () async => validator.validate(), + getCurrentDirectory: () => tempDir, + ); + + expect(results, isEmpty); + }); + + test('errors when Flutter does not exist', () async { + flutterDirectory(tempDir).deleteSync(); + + final results = await validator.validate(); + + expect(results, hasLength(1)); + expect(results.first.severity, ValidationIssueSeverity.error); + expect(results.first.message, contains('No Flutter directory found')); + }); + + test('warns when Flutter has local modifications', () async { + when(() => gitStatusProcessResult.stdout) + .thenReturn('Changes not staged for commit'); + + final results = await IOOverrides.runZoned( + () async => validator.validate(), + getCurrentDirectory: () => tempDir, + ); + + expect(results, hasLength(1)); + expect(results.first.severity, ValidationIssueSeverity.warning); + expect(results.first.message, contains('has local modifications')); + }); + + test('warns when Flutter does not track stable', () async { + when(() => gitBranchProcessResult.stdout).thenReturn(''' +* main + stable +'''); + + final results = await IOOverrides.runZoned( + () async => validator.validate(), + getCurrentDirectory: () => tempDir, + ); + + expect(results, hasLength(1)); + expect(results.first.severity, ValidationIssueSeverity.warning); + expect(results.first.message, contains('is not on the "stable" branch')); + }); + }); +} From c09b5c707413fa271e2ccd35c5d40f922cdb4772 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 16:57:56 -0400 Subject: [PATCH 2/7] Remove unused import --- .../lib/src/doctor/validators/shorebird_flutter_validator.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart index 4d243bdb8..1d663f22d 100644 --- a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart +++ b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart @@ -1,5 +1,3 @@ -import 'dart:io'; - import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; import 'package:shorebird_cli/src/shorebird_paths.dart'; import 'package:shorebird_cli/src/shorebird_process.dart'; From 2d13a477a78acf8db50b7f915a049e10963a95c0 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 16:58:03 -0400 Subject: [PATCH 3/7] Add trailing comma --- .../test/src/shorebird_process_test.dart | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/shorebird_cli/test/src/shorebird_process_test.dart b/packages/shorebird_cli/test/src/shorebird_process_test.dart index faaeb19b9..77b8d851a 100644 --- a/packages/shorebird_cli/test/src/shorebird_process_test.dart +++ b/packages/shorebird_cli/test/src/shorebird_process_test.dart @@ -94,11 +94,13 @@ void main() { test('replaces "flutter" with our local flutter', () async { await ShorebirdProcess.start('flutter', ['run'], runInShell: true); - verify(() => processWrapper.start( - 'flutter/bin/flutter', - ['run'], - runInShell: true, - )).called(1); + verify( + () => processWrapper.start( + 'flutter/bin/flutter', + ['run'], + runInShell: true, + ), + ).called(1); }); }); }); From 838323e9905e1e994bd97fa6473c63e7f57540a1 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 17:01:05 -0400 Subject: [PATCH 4/7] Test cleanup --- .../shorebird_flutter_validator_test.dart | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart index 2d86ad8bc..43497aadf 100644 --- a/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart +++ b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_flutter_validator_test.dart @@ -78,10 +78,7 @@ nothing to commit, working tree clean }); test('returns no issues when the Flutter install is good', () async { - final results = await IOOverrides.runZoned( - () async => validator.validate(), - getCurrentDirectory: () => tempDir, - ); + final results = await validator.validate(); expect(results, isEmpty); }); @@ -100,10 +97,7 @@ nothing to commit, working tree clean when(() => gitStatusProcessResult.stdout) .thenReturn('Changes not staged for commit'); - final results = await IOOverrides.runZoned( - () async => validator.validate(), - getCurrentDirectory: () => tempDir, - ); + final results = await validator.validate(); expect(results, hasLength(1)); expect(results.first.severity, ValidationIssueSeverity.warning); @@ -116,10 +110,7 @@ nothing to commit, working tree clean stable '''); - final results = await IOOverrides.runZoned( - () async => validator.validate(), - getCurrentDirectory: () => tempDir, - ); + final results = await validator.validate(); expect(results, hasLength(1)); expect(results.first.severity, ValidationIssueSeverity.warning); From ba8dc6004b68f81ce4cc258a28014ebfb5980b19 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 17:06:02 -0400 Subject: [PATCH 5/7] Make shorebirdRoot a computed property --- packages/shorebird_cli/lib/src/shorebird_paths.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shorebird_cli/lib/src/shorebird_paths.dart b/packages/shorebird_cli/lib/src/shorebird_paths.dart index a1cd3e785..d0a5ef1c4 100644 --- a/packages/shorebird_cli/lib/src/shorebird_paths.dart +++ b/packages/shorebird_cli/lib/src/shorebird_paths.dart @@ -11,7 +11,7 @@ abstract class ShorebirdPaths { /// The root directory of the Shorebird install. /// /// Assumes we are running from $ROOT/bin/cache. - static Directory shorebirdRoot = + static Directory get shorebirdRoot => File(platform.script.toFilePath()).parent.parent.parent; /// The root of the Shorebird-vended Flutter git checkout. From d7f78dcb84172565e9d9d1a262bd534ad14299c8 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 17:06:40 -0400 Subject: [PATCH 6/7] Delete todos now that they're todone --- bin/internal/shared.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/bin/internal/shared.sh b/bin/internal/shared.sh index bffaac530..6cd11b34e 100755 --- a/bin/internal/shared.sh +++ b/bin/internal/shared.sh @@ -10,8 +10,6 @@ unset CDPATH # Either clones or pulls the Shorebird Flutter repository, depending on whether FLUTTER_PATH exists. function update_flutter { - # TODO(bryanoltman): add doctor check that we're on stable - # TODO(bryanoltman): add doctor check for modified files if [[ -d "$FLUTTER_PATH" ]]; then git --git-dir="$FLUTTER_PATH/.git" pull else From 13e1a1d1e121c32e2e512037fe5cf303518fd96e Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 5 Apr 2023 17:10:35 -0400 Subject: [PATCH 7/7] Ignore test coverage for description --- .../lib/src/doctor/validators/shorebird_flutter_validator.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart index 1d663f22d..d5280bc56 100644 --- a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart +++ b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_flutter_validator.dart @@ -7,8 +7,10 @@ class ShorebirdFlutterValidator extends DoctorValidator { final RunProcess runProcess; + // coverage:ignore-start @override String get description => 'Flutter install is correct'; + // coverage:ignore-end @override Future> validate() async {