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): don't run AndroidInternetPermissionValidator if no android project exists #995

Merged
merged 2 commits into from
Aug 2, 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
6 changes: 1 addition & 5 deletions packages/shorebird_cli/lib/src/doctor.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import 'package:mason_logger/mason_logger.dart';
import 'package:scoped/scoped.dart';
import 'package:shorebird_cli/src/logger.dart';
import 'package:shorebird_cli/src/shorebird_environment.dart';
import 'package:shorebird_cli/src/validators/validators.dart';

/// A reference to a [Doctor] instance.
Expand Down Expand Up @@ -36,15 +35,12 @@ class Doctor {
List<Validator> validators, {
bool applyFixes = false,
}) async {
final isInProject =
ShorebirdEnvironment.getShorebirdYamlFile().existsSync();

final allIssues = <ValidationIssue>[];
final allFixableIssues = <ValidationIssue>[];

var numIssuesFixed = 0;
for (final validator in validators) {
if (validator.scope == ValidatorScope.project && !isInProject) {
if (!validator.canRunInCurrentContext()) {
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,30 +22,13 @@ class AndroidInternetPermissionValidator extends Validator {
'AndroidManifest.xml files contain INTERNET permission';

@override
ValidatorScope get scope => ValidatorScope.project;
bool canRunInCurrentContext() => _androidSrcDirectory.existsSync();

@override
Future<List<ValidationIssue>> validate() async {
const manifestFileName = 'AndroidManifest.xml';
final androidSrcDir = Directory(
p.join(
Directory.current.path,
'android',
'app',
'src',
),
);

if (!androidSrcDir.existsSync()) {
return [
const ValidationIssue(
severity: ValidationIssueSeverity.error,
message: 'No Android project found',
),
];
}

final manifestFiles = androidSrcDir
final manifestFiles = _androidSrcDirectory
.listSync()
.whereType<Directory>()
.where(
Expand All @@ -61,7 +44,7 @@ class AndroidInternetPermissionValidator extends Validator {
ValidationIssue(
severity: ValidationIssueSeverity.error,
message:
'No AndroidManifest.xml files found in ${androidSrcDir.path}',
'''No AndroidManifest.xml files found in ${_androidSrcDirectory.path}''',
),
];
}
Expand All @@ -88,6 +71,15 @@ class AndroidInternetPermissionValidator extends Validator {
return [];
}

Directory get _androidSrcDirectory => Directory(
p.join(
Directory.current.path,
'android',
'app',
'src',
),
);

bool _androidManifestHasInternetPermission(String path) {
final xmlDocument = XmlDocument.parse(File(path).readAsStringSync());
return xmlDocument.rootElement.childElements
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class ShorebirdFlutterValidator extends Validator {
String get description => 'Flutter install is correct';

@override
ValidatorScope get scope => ValidatorScope.installation;
bool canRunInCurrentContext() => true;

@override
Future<List<ValidationIssue>> validate() async {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class ShorebirdVersionValidator extends Validator {
String get description => 'Shorebird is up-to-date';

@override
ValidatorScope get scope => ValidatorScope.installation;
bool canRunInCurrentContext() => true;

@override
Future<List<ValidationIssue>> validate() async {
Expand Down
9 changes: 1 addition & 8 deletions packages/shorebird_cli/lib/src/validators/validators.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ enum ValidationIssueSeverity {
warning,
}

/// The level at which validation is being performed.
enum ValidatorScope {
project,
installation,
}

/// Display helpers for printing [ValidationIssue]s.
extension Display on ValidationIssueSeverity {
String get leading {
Expand Down Expand Up @@ -91,6 +85,5 @@ abstract class Validator {
/// Not all validators use [process].
Future<List<ValidationIssue>> validate();

/// Whether this validator is project-specific or system-wide.
ValidatorScope get scope;
bool canRunInCurrentContext();
}
94 changes: 28 additions & 66 deletions packages/shorebird_cli/test/src/doctor_test.dart
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import 'dart:io';

import 'package:mason_logger/mason_logger.dart';
import 'package:mocktail/mocktail.dart';
import 'package:path/path.dart' as p;
import 'package:scoped/scoped.dart';
import 'package:shorebird_cli/src/doctor.dart';
import 'package:shorebird_cli/src/logger.dart';
Expand Down Expand Up @@ -55,24 +52,22 @@ void main() {
when(() => logger.info(any())).thenReturn(null);

when(noIssuesValidator.validate).thenAnswer((_) async => []);
when(() => noIssuesValidator.scope)
.thenReturn(ValidatorScope.installation);
when(noIssuesValidator.canRunInCurrentContext).thenReturn(true);
when(() => noIssuesValidator.description)
.thenReturn('no issues validator');

when(warningValidator.validate).thenAnswer(
(_) async => [validationWarning],
);
when(() => warningValidator.scope)
.thenReturn(ValidatorScope.installation);
when(warningValidator.canRunInCurrentContext).thenReturn(true);
when(() => warningValidator.description).thenReturn('warning validator');

when(errorValidator.validate).thenAnswer((_) async => [validationError]);
when(() => errorValidator.scope).thenReturn(ValidatorScope.installation);
when(errorValidator.canRunInCurrentContext).thenReturn(true);
when(() => errorValidator.description).thenReturn('error validator');
});

group('validate', () {
group('runValidators', () {
test('prints messages when warnings and errors found', () async {
final validators = [
warningValidator,
Expand All @@ -98,64 +93,32 @@ void main() {
).called(1);
});

group('validator scope', () {
const appId = 'test-app-id';
late Validator projectScopeValidator;
late Validator installationScopeValidator;

Directory setUpTempDir() {
final tempDir = Directory.systemTemp.createTempSync();
File(
p.join(tempDir.path, 'shorebird.yaml'),
).writeAsStringSync('app_id: $appId');
return tempDir;
}
test('only runs validators that can run in the current context',
() async {
final validators = [
noIssuesValidator,
warningValidator,
errorValidator,
];
when(() => warningValidator.canRunInCurrentContext()).thenReturn(false);

setUp(() {
projectScopeValidator = _MockValidator();
installationScopeValidator = _MockValidator();

when(projectScopeValidator.validate).thenAnswer((_) async => []);
when(() => projectScopeValidator.scope)
.thenReturn(ValidatorScope.project);
when(() => projectScopeValidator.description)
.thenReturn('project-scoped validator');

when(installationScopeValidator.validate).thenAnswer((_) async => []);
when(() => installationScopeValidator.scope)
.thenReturn(ValidatorScope.installation);
when(() => installationScopeValidator.description)
.thenReturn('installation-scoped validator');
});
await runWithOverrides(() async => doctor.runValidators(validators));

test('does not run project-scoped validators in project directory',
() async {
final validators = [
projectScopeValidator,
installationScopeValidator
];
await runWithOverrides(
() => doctor.runValidators(validators),
);
verify(installationScopeValidator.validate).called(1);
verifyNever(projectScopeValidator.validate);
});
verify(noIssuesValidator.validate).called(1);
verifyNever(warningValidator.validate);
verify(errorValidator.validate).called(1);
});

test('runs project-scoped validators in project directory', () async {
final tempDir = setUpTempDir();
final validators = [
projectScopeValidator,
installationScopeValidator
];
await runWithOverrides(
() async => IOOverrides.runZoned(
() => doctor.runValidators(validators),
getCurrentDirectory: () => tempDir,
),
);
verify(installationScopeValidator.validate).called(1);
verify(projectScopeValidator.validate).called(1);
});
test('tells the user when no issues are found', () async {
final validators = [
noIssuesValidator,
];

await runWithOverrides(() async => doctor.runValidators(validators));

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

group('fix', () {
Expand All @@ -180,10 +143,9 @@ void main() {
when(fixableWarningValidator.validate).thenAnswer(
(_) async => [fixableValidationWarning],
);
when(() => fixableWarningValidator.scope)
.thenReturn(ValidatorScope.installation);
when(() => fixableWarningValidator.description)
.thenReturn('fixable warning validator');
when(fixableWarningValidator.canRunInCurrentContext).thenReturn(true);
});

test(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
import 'dart:io';

import 'package:collection/collection.dart';
import 'package:mocktail/mocktail.dart';
import 'package:path/path.dart' as p;
import 'package:scoped/scoped.dart';
import 'package:shorebird_cli/src/process.dart';
import 'package:shorebird_cli/src/validators/validators.dart';
import 'package:test/test.dart';

class _MockShorebirdProcess extends Mock implements ShorebirdProcess {}

void main() {
const manifestWithInternetPermission = '''
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
Expand Down Expand Up @@ -39,21 +34,6 @@ void main() {
''';

group(AndroidInternetPermissionValidator, () {
late ShorebirdProcess shorebirdProcess;

R runWithOverrides<R>(R Function() body) {
return runScoped(
() => body(),
values: {
processRef.overrideWith(() => shorebirdProcess),
},
);
}

setUp(() {
shorebirdProcess = _MockShorebirdProcess();
});

Directory createTempDir() => Directory.systemTemp.createTempSync();

void writeManifestToPath(String manifestContents, String path) {
Expand All @@ -66,11 +46,32 @@ void main() {
expect(AndroidInternetPermissionValidator().description, isNotEmpty);
});

test('is a project-specific validator', () {
expect(
AndroidInternetPermissionValidator().scope,
ValidatorScope.project,
);
group('canRunInContext', () {
test('returns false if no android src directory exists', () {
final tempDirectory = createTempDir();

final result = IOOverrides.runZoned(
() => AndroidInternetPermissionValidator().canRunInCurrentContext(),
getCurrentDirectory: () => tempDirectory,
);

expect(result, isFalse);
});

test('returns true if an android src directory exists', () {
final tempDirectory = createTempDir();
writeManifestToPath(
manifestWithInternetPermission,
p.join(tempDirectory.path, 'android', 'app', 'src', 'main'),
);

final result = IOOverrides.runZoned(
() => AndroidInternetPermissionValidator().canRunInCurrentContext(),
getCurrentDirectory: () => tempDirectory,
);

expect(result, isTrue);
});
});

test(
Expand All @@ -88,31 +89,22 @@ void main() {
);

final results = await IOOverrides.runZoned(
() => runWithOverrides(AndroidInternetPermissionValidator().validate),
AndroidInternetPermissionValidator().validate,
getCurrentDirectory: () => tempDirectory,
);

expect(results.map((res) => res.severity), isEmpty);
},
);

test('returns an error if no android project is found', () async {
final results = await AndroidInternetPermissionValidator().validate();

expect(results, hasLength(1));
expect(results.first.severity, ValidationIssueSeverity.error);
expect(results.first.message, 'No Android project found');
expect(results.first.fix, isNull);
});

test('returns an error if no AndroidManifest.xml files are found',
() async {
final tempDirectory = createTempDir();
Directory(p.join(tempDirectory.path, 'android', 'app', 'src', 'debug'))
.createSync(recursive: true);

final results = await IOOverrides.runZoned(
() => runWithOverrides(AndroidInternetPermissionValidator().validate),
AndroidInternetPermissionValidator().validate,
getCurrentDirectory: () => tempDirectory,
);

Expand Down Expand Up @@ -168,7 +160,7 @@ void main() {
);

final results = await IOOverrides.runZoned(
() => runWithOverrides(AndroidInternetPermissionValidator().validate),
AndroidInternetPermissionValidator().validate,
getCurrentDirectory: () => tempDirectory,
);

Expand Down Expand Up @@ -204,7 +196,7 @@ void main() {
);

var results = await IOOverrides.runZoned(
() => runWithOverrides(AndroidInternetPermissionValidator().validate),
AndroidInternetPermissionValidator().validate,
getCurrentDirectory: () => tempDirectory,
);
expect(results, hasLength(1));
Expand All @@ -216,7 +208,7 @@ void main() {
);

results = await IOOverrides.runZoned(
() => runWithOverrides(AndroidInternetPermissionValidator().validate),
AndroidInternetPermissionValidator().validate,
getCurrentDirectory: () => tempDirectory,
);
expect(results, isEmpty);
Expand Down
Loading