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): use relative paths to manifest files in shorebird doctor #419

Merged
merged 11 commits into from
May 2, 2023
62 changes: 55 additions & 7 deletions packages/shorebird_cli/lib/src/commands/doctor_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin {
super.validators,
}) {
validators = _allValidators(baseValidators: validators);

argParser.addFlag(
'fix',
abbr: 'f',
help: 'Fix issues where possible.',
negatable: false,
);
}

late final List<Validator> _doctorValidators = [
Expand All @@ -36,33 +43,74 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin {

@override
Future<int> run() async {
final shouldFix = results['fix'] == true;

logger.info('''

Shorebird v$packageVersion
Shorebird Engine • revision ${ShorebirdEnvironment.shorebirdEngineRevision}''');

var numIssues = 0;
final allIssues = <ValidationIssue>[];
final allFixableIssues = <ValidationIssue>[];
for (final validator in validators) {
final progress = logger.progress(validator.description);
final issues = await validator.validate(process);
numIssues += issues.length;
if (issues.isEmpty) {
progress.complete();
} else {
progress.fail();
continue;
}

for (final issue in issues) {
logger.info(' ${issue.displayMessage}');
final fixableIssues = issues.where((issue) => issue.fix != null);
var unresolvedIssues = issues;
if (fixableIssues.isNotEmpty) {
if (shouldFix) {
// If --fix flag was used and there are fixable issues, fix them.
progress.update('Fixing');
for (final issue in fixableIssues) {
await issue.fix!();
}

// Re-run the validator to see if there are any remaining issues that
// we couldn't fix.
unresolvedIssues = await validator.validate(process);
if (unresolvedIssues.isEmpty) {
final numFixed = issues.length - unresolvedIssues.length;
final fixAppliedMessage =
'($numFixed fix${numFixed == 1 ? '' : 'es'} applied)';
progress.complete(
'''${validator.description} ${green.wrap(fixAppliedMessage)}''',
);
continue;
}
} else {
allFixableIssues.addAll(issues);
}
}

progress.fail();

for (final issue in unresolvedIssues) {
logger.info(' ${issue.displayMessage}');
}

allIssues.addAll(unresolvedIssues);
}

logger.info('');

if (numIssues == 0) {
if (allIssues.isEmpty) {
logger.info('No issues detected!');
} else {
final numIssues = allIssues.length;
logger.info('$numIssues issue${numIssues == 1 ? '' : 's'} detected.');

if (allFixableIssues.isNotEmpty && !shouldFix) {
final fixableIssueCount = allFixableIssues.length;
logger.info(
'''
$fixableIssueCount issue${fixableIssueCount == 1 ? '' : 'es'} can be fixed automatically with ${lightCyan.wrap('shorebird doctor --fix')}.''',
);
}
}

return ExitCode.success.code;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ class AndroidInternetPermissionValidator extends Validator {
.map(
(String manifestPath) => ValidationIssue(
severity: ValidationIssueSeverity.error,
message: '$manifestPath is missing the INTERNET permission.',
message:
'${p.relative(manifestPath, from: Directory.current.path)} '
'is missing the INTERNET permission.',
fix: () async => _addInternetPermissionToFile(manifestPath),
),
)
.toList();
Expand All @@ -90,4 +93,20 @@ class AndroidInternetPermissionValidator extends Validator {
return attribute.qualifiedName == 'android:name' &&
attribute.value == 'android.permission.INTERNET';
}

void _addInternetPermissionToFile(String path) {
final xmlDocument = XmlDocument.parse(File(path).readAsStringSync());
xmlDocument.rootElement.children.add(
XmlElement(
XmlName('uses-permission'),
[
XmlAttribute(
XmlName('android:name'),
'android.permission.INTERNET',
),
],
),
);
File(path).writeAsStringSync(xmlDocument.toXmlString(pretty: true));
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:mason_logger/mason_logger.dart';
import 'package:shorebird_cli/src/shorebird_environment.dart';
import 'package:shorebird_cli/src/shorebird_process.dart';
import 'package:shorebird_cli/src/validators/validators.dart';
Expand Down
9 changes: 8 additions & 1 deletion packages/shorebird_cli/lib/src/validators/validators.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@ extension Display on ValidationIssueSeverity {
/// A (potential) problem with the current Shorebird installation or project.
@immutable
class ValidationIssue {
const ValidationIssue({required this.severity, required this.message});
const ValidationIssue({
required this.severity,
required this.message,
this.fix,
});

/// How important it is to fix this issue.
final ValidationIssueSeverity severity;

/// A description of the issue.
final String message;

/// Fixes this issue.
final Future<void> Function()? fix;

/// A console-friendly description of this issue.
String? get displayMessage {
return '${severity.leading} $message';
Expand Down
155 changes: 153 additions & 2 deletions packages/shorebird_cli/test/src/commands/doctor_command_test.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:args/args.dart';
import 'package:mason_logger/mason_logger.dart';
import 'package:mocktail/mocktail.dart';
import 'package:shorebird_cli/src/commands/commands.dart';
Expand All @@ -6,6 +7,8 @@ import 'package:shorebird_cli/src/shorebird_process.dart';
import 'package:shorebird_cli/src/validators/validators.dart';
import 'package:test/test.dart';

class _MockArgResults extends Mock implements ArgResults {}

class _MockShorebirdVersionValidator extends Mock
implements ShorebirdVersionValidator {}

Expand All @@ -23,6 +26,7 @@ class _MockShorebirdProcess extends Mock implements ShorebirdProcess {}

void main() {
group('doctor', () {
late ArgResults argResults;
late Logger logger;
late Progress progress;
late DoctorCommand command;
Expand All @@ -32,11 +36,14 @@ void main() {
late ShorebirdProcess shorebirdProcess;

setUp(() {
argResults = _MockArgResults();
logger = _MockLogger();
progress = _MockProgress();

ShorebirdEnvironment.shorebirdEngineRevision = 'test-revision';

when(() => argResults['fix']).thenReturn(false);

when(() => logger.progress(any())).thenReturn(progress);
when(() => logger.info(any())).thenReturn(null);

Expand Down Expand Up @@ -76,6 +83,7 @@ void main() {
shorebirdFlutterValidator,
],
)
..testArgResults = argResults
..testProcess = shorebirdProcess
..testEngineConfig = const EngineConfig.empty();
});
Expand Down Expand Up @@ -113,16 +121,159 @@ void main() {
}

verify(
() => logger.info(any(that: contains('${yellow.wrap('[!]')} oh no!'))),
() => logger.info(any(that: stringContainsInOrder(['[!]', 'oh no!']))),
).called(1);

verify(
() => logger.info(any(that: contains('${red.wrap('[✗]')} OH NO!'))),
() => logger.info(any(that: stringContainsInOrder(['[✗]', 'OH NO!']))),
).called(1);

verify(
() => logger.info(any(that: contains('2 issues detected.'))),
).called(1);
});

test('tells the user we can fix issues if we can', () async {
when(
() => androidInternetPermissionValidator.validate(any()),
).thenAnswer(
(_) async => [
ValidationIssue(
severity: ValidationIssueSeverity.warning,
message: 'oh no!',
fix: () async {},
),
],
);

await command.run();

verify(
() => logger.info(
any(
that: stringContainsInOrder([
'1 issue can be fixed automatically',
'shorebird doctor --fix',
]),
),
),
).called(1);
});

test('does not tell the user we can fix issues if we cannot', () async {
when(
() => androidInternetPermissionValidator.validate(any()),
).thenAnswer(
(_) async => [
const ValidationIssue(
severity: ValidationIssueSeverity.warning,
message: 'oh no!',
),
],
);

await command.run();

verifyNever(
() => logger.info(
any(
that: stringContainsInOrder([
'We can fix some of these issues',
'shorebird doctor --fix',
]),
),
),
);
});

test('does not fix issues if --fix flag is not provided', () async {
when(() => argResults['fix']).thenReturn(false);

var fixCalled = false;
when(
() => androidInternetPermissionValidator.validate(any()),
).thenAnswer(
(_) async => [
ValidationIssue(
severity: ValidationIssueSeverity.warning,
message: 'oh no!',
fix: () async => fixCalled = true,
),
],
);

await command.run();

expect(fixCalled, isFalse);
verifyNever(() => progress.update('Fixing'));
verify(() => progress.fail()).called(1);
verify(
() => androidInternetPermissionValidator.validate(any()),
).called(1);
});

test('fixes issues if the --fix flag is provided', () async {
when(() => argResults['fix']).thenReturn(true);

var fixCalled = false;
final issues = [
ValidationIssue(
severity: ValidationIssueSeverity.warning,
message: 'oh no!',
fix: () async => fixCalled = true,
),
];
when(
() => androidInternetPermissionValidator.validate(any()),
).thenAnswer(
(_) async {
if (issues.isEmpty) return [];
return [issues.removeLast()];
},
);

await command.run();

expect(fixCalled, isTrue);
verify(() => progress.update('Fixing')).called(1);
verify(
() => progress.complete(any(that: contains('1 fix applied'))),
).called(1);
verify(
() => androidInternetPermissionValidator.validate(any()),
).called(2);
});

test('does not print "fixed" if fix fails', () async {
when(() => argResults['fix']).thenReturn(true);

var fixCalled = false;
when(
() => androidInternetPermissionValidator.validate(any()),
).thenAnswer(
(_) async => [
ValidationIssue(
severity: ValidationIssueSeverity.warning,
message: 'oh no!',
fix: () async => fixCalled = true,
),
],
);

await command.run();

expect(fixCalled, isTrue);
verify(() => progress.update('Fixing')).called(1);
verifyNever(
() => progress.complete(any(that: contains('fix applied'))),
);
verifyNever(
() => progress.complete(any(that: contains('fixes applied'))),
);
verify(() => progress.fail()).called(1);
verify(
() => androidInternetPermissionValidator.validate(any()),
).called(2);
});
});
}
Loading