Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(shorebird_cli): handle exceptions during validation more gracefully #359

Merged
merged 3 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import 'dart:io';
import 'package:mason_logger/mason_logger.dart';
import 'package:shorebird_cli/src/auth_logger_mixin.dart';
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_build_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';

/// {@template build_apk_command}
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import 'dart:io';
import 'package:mason_logger/mason_logger.dart';
import 'package:shorebird_cli/src/auth_logger_mixin.dart';
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_build_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';

/// {@template build_app_bundle_command}
///
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/commands/build/build.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_build_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';

/// {@template build_command}
///
Expand Down
2 changes: 1 addition & 1 deletion packages/shorebird_cli/lib/src/commands/patch_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import 'package:mason_logger/mason_logger.dart';
import 'package:path/path.dart' as p;
import 'package:shorebird_cli/src/auth_logger_mixin.dart';
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_build_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_create_app_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';
import 'package:shorebird_code_push_client/shorebird_code_push_client.dart';

/// Metadata about a patch artifact that we are about to upload.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import 'package:mason_logger/mason_logger.dart';
import 'package:path/path.dart' as p;
import 'package:shorebird_cli/src/auth_logger_mixin.dart';
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_build_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_create_app_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';
import 'package:shorebird_code_push_client/shorebird_code_push_client.dart';

/// {@template release_command}
Expand Down
2 changes: 1 addition & 1 deletion packages/shorebird_cli/lib/src/commands/run_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import 'dart:convert';
import 'package:mason_logger/mason_logger.dart';
import 'package:shorebird_cli/src/auth_logger_mixin.dart';
import 'package:shorebird_cli/src/command.dart';
import 'package:shorebird_cli/src/flutter_validation_mixin.dart';
import 'package:shorebird_cli/src/shorebird_config_mixin.dart';
import 'package:shorebird_cli/src/shorebird_validation_mixin.dart';

/// {@template run_command}
/// `shorebird run`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,33 @@ class ShorebirdFlutterValidator extends Validator {
);
}

final shorebirdFlutterVersion = await _shorebirdFlutterVersion(process);
final pathFlutterVersion = await _pathFlutterVersion(process);
String? shorebirdFlutterVersion;
try {
shorebirdFlutterVersion = await _shorebirdFlutterVersion(process);
} catch (error) {
issues.add(
ValidationIssue(
severity: ValidationIssueSeverity.error,
message: 'Failed to determine Shorebird Flutter version. $error',
),
);
}

String? pathFlutterVersion;
try {
pathFlutterVersion = await _pathFlutterVersion(process);
} catch (error) {
issues.add(
ValidationIssue(
severity: ValidationIssueSeverity.error,
message: 'Failed to determine path Flutter version. $error',
),
);
}

if (shorebirdFlutterVersion != pathFlutterVersion) {
if (shorebirdFlutterVersion != null &&
pathFlutterVersion != null &&
shorebirdFlutterVersion != pathFlutterVersion) {
final message = '''
The version of Flutter that Shorebird includes and the Flutter on your path are different.
\tShorebird Flutter: $shorebirdFlutterVersion
Expand Down Expand Up @@ -147,7 +170,7 @@ This can cause unexpected behavior if you are switching between the tools and th
final match = _flutterVersionRegex.firstMatch(output);
if (match == null) {
throw FlutterValidationException(
'Could not find version match in $output',
'Could not find version number in $output',
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class _MockPlatform extends Mock implements Platform {}
class _MockShorebirdProcess extends Mock implements ShorebirdProcess {}

void main() {
group('ShorebirdFlutterValidator', () {
group(ShorebirdFlutterValidator, () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass a class name to group? til.

const flutterRevision = '45fc514f1a9c347a3af76b02baf980a4d88b7879';
const gitStatusMessage = '''
HEAD detached at 45fc514f
Expand Down Expand Up @@ -196,26 +196,76 @@ Tools • Dart 2.19.6 • DevTools 2.20.1
},
);

test('throws exception if flutter version output is malformed', () async {
test('throws exception if path flutter version output is malformed',
() async {
when(() => pathFlutterVersionProcessResult.stdout)
.thenReturn('OH NO THERE IS NO FLUTTER VERSION HERE');

expect(() async => validator.validate(shorebirdProcess), throwsException);
final results = await validator.validate(shorebirdProcess);

expect(results, hasLength(1));
expect(
results[0],
isA<ValidationIssue>().having(
(exception) => exception.message,
'message',
contains('Failed to determine path Flutter version'),
),
);
});

test('prints stderr output and throws if version check fails', () async {
test('prints stderr output and throws if path Flutterversion check fails',
() async {
when(() => pathFlutterVersionProcessResult.exitCode).thenReturn(1);
when(() => pathFlutterVersionProcessResult.stderr)
.thenReturn('error getting Flutter version');

final results = await validator.validate(shorebirdProcess);

expect(results, hasLength(1));
expect(
() async => validator.validate(shorebirdProcess),
throwsA(
isA<FlutterValidationException>().having(
(e) => e.message,
'message',
contains('error getting Flutter version'),
),
results[0],
isA<ValidationIssue>().having(
(exception) => exception.message,
'message',
contains('Failed to determine path Flutter version'),
),
);
});

test('throws exception if shorebird flutter version output is malformed',
() async {
when(() => shorebirdFlutterVersionProcessResult.stdout)
.thenReturn('OH NO THERE IS NO FLUTTER VERSION HERE');

final results = await validator.validate(shorebirdProcess);

expect(results, hasLength(1));
expect(
results[0],
isA<ValidationIssue>().having(
(exception) => exception.message,
'message',
contains('Failed to determine Shorebird Flutter version'),
),
);
});

test('prints stderr output and throws if path Flutterversion check fails',
() async {
when(() => shorebirdFlutterVersionProcessResult.exitCode).thenReturn(1);
when(() => shorebirdFlutterVersionProcessResult.stderr)
.thenReturn('error getting Flutter version');

final results = await validator.validate(shorebirdProcess);

expect(results, hasLength(1));
expect(
results[0],
isA<ValidationIssue>().having(
(exception) => exception.message,
'message',
contains('Failed to determine Shorebird Flutter version'),
),
);
});
Expand Down