Skip to content

Commit

Permalink
[tools] Fix vm test requirement (flutter#6995)
Browse files Browse the repository at this point in the history 
The logic for handling Dart unit test `test_on` directives was incorrect, causing it to skip packages that required vm testing, even when run in vm mode. This PR:
- Adds the missing tests for false negatives.
- Reworks the logic to explicitly fail for anything that isn't one of the exact patterns we are expecting, to make it much harder to re-introduce a bug like this in the future.
  • Loading branch information
@stuartmorgan
stuartmorgan authored Jun 27, 2024
1 parent 5549eba commit 69e7fc1
Show file tree
Hide file tree
Showing 3 changed files with 192 additions and 41 deletions.
16 changes: 8 additions & 8 deletions packages/flutter_migrate/pubspec.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,17 @@ environment:

dependencies:
args: ^2.3.1
convert: 3.0.2
file: 6.1.4
intl: 0.17.0
meta: 1.8.0
convert: ^3.0.2
file: ">=6.0.0 <8.0.0"
intl: ">=0.17.0 <0.20.0"
meta: ^1.8.0
path: ^1.8.0
process: 4.2.4
vm_service: 9.3.0
yaml: 3.1.1
process: ^4.2.4
vm_service: ^9.3.0
yaml: ^3.1.1

dev_dependencies:
collection: 1.16.0
collection: ^1.16.0
file_testing: 3.0.0
lints: ^2.0.0
test: ^1.16.0
72 changes: 44 additions & 28 deletions script/tool/lib/src/dart_test_command.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ import 'common/plugin_utils.dart';
import 'common/pub_utils.dart';
import 'common/repository_package.dart';

const int _exitUnknownTestPlatform = 3;

enum _TestPlatform {
// Must run in the command-line VM.
vm,
// Must run in a browser.
browser,
}

/// A command to run Dart unit tests for packages.
class DartTestCommand extends PackageLoopingCommand {
/// Creates an instance of the test command.
Expand Down Expand Up @@ -76,7 +85,7 @@ class DartTestCommand extends PackageLoopingCommand {
return PackageResult.skip(
"Non-web plugin tests don't need web testing.");
}
if (_requiresVM(package)) {
if (_testOnTarget(package) == _TestPlatform.vm) {
// This explict skip is necessary because trying to run tests in a mode
// that the package has opted out of returns a non-zero exit code.
return PackageResult.skip('Package has opted out of non-vm testing.');
Expand All @@ -85,7 +94,8 @@ class DartTestCommand extends PackageLoopingCommand {
if (isWebOnlyPluginImplementation) {
return PackageResult.skip("Web plugin tests don't need vm testing.");
}
if (_requiresNonVM(package)) {
final _TestPlatform? target = _testOnTarget(package);
if (target != null && _testOnTarget(package) != _TestPlatform.vm) {
// This explict skip is necessary because trying to run tests in a mode
// that the package has opted out of returns a non-zero exit code.
return PackageResult.skip('Package has opted out of vm testing.');
Expand All @@ -102,7 +112,8 @@ class DartTestCommand extends PackageLoopingCommand {
final String? webRenderer = (platform == 'chrome') ? 'html' : null;
bool passed;
if (package.requiresFlutter()) {
passed = await _runFlutterTests(package, platform: platform, webRenderer: webRenderer);
passed = await _runFlutterTests(package,
platform: platform, webRenderer: webRenderer);
} else {
passed = await _runDartTests(package, platform: platform);
}
Expand Down Expand Up @@ -156,34 +167,39 @@ class DartTestCommand extends PackageLoopingCommand {
return exitCode == 0;
}

bool _requiresVM(RepositoryPackage package) {
final File testConfig = package.directory.childFile('dart_test.yaml');
if (!testConfig.existsSync()) {
return false;
}
// test_on lines can be very complex, but in pratice the packages in this
// repo currently only need the ability to require vm or not, so that
// simple directive is all that is currently supported.
final RegExp vmRequrimentRegex = RegExp(r'^test_on:\s*vm$');
return testConfig
.readAsLinesSync()
.any((String line) => vmRequrimentRegex.hasMatch(line));
}

bool _requiresNonVM(RepositoryPackage package) {
/// Returns the required test environment, or null if none is specified.
///
/// Throws if the target is not recognized.
_TestPlatform? _testOnTarget(RepositoryPackage package) {
final File testConfig = package.directory.childFile('dart_test.yaml');
if (!testConfig.existsSync()) {
return false;
return null;
}
// test_on lines can be very complex, but in pratice the packages in this
// repo currently only need the ability to require vm or not, so a simple
// one-target directive is all that's supported currently. Making it
// deliberately strict avoids the possibility of accidentally skipping vm
// coverage due to a complex expression that's not handled correctly.
final RegExp testOnRegex = RegExp(r'^test_on:\s*([a-z])*\s*$');
return testConfig.readAsLinesSync().any((String line) {
final RegExp testOnRegex = RegExp(r'^test_on:\s*([a-z].*[a-z])\s*$');
for (final String line in testConfig.readAsLinesSync()) {
final RegExpMatch? match = testOnRegex.firstMatch(line);
return match != null && match.group(1) != 'vm';
});
if (match != null) {
final String targetFilter = match.group(1)!;
// test_on lines can be very complex, but in pratice the packages in
// this repo currently only need the ability to require vm or not, so a
// simple one-target directive is all that's supported currently.
// Making it deliberately strict avoids the possibility of accidentally
// skipping vm coverage due to a complex expression that's not handled
// correctly.
switch (targetFilter) {
case 'vm':
return _TestPlatform.vm;
case 'browser':
return _TestPlatform.browser;
default:
printError('Unknown "test_on" value: "$targetFilter"\n'
"If this value needs to be supported for this package's tests, "
'please update the repository tooling to support more test_on '
'modes.');
throw ToolExit(_exitUnknownTestPlatform);
}
}
}
return null;
}
}
145 changes: 140 additions & 5 deletions script/tool/test/dart_test_command_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,68 @@ void main() {
);
});

test('throws for an unrecognized test_on type', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
packagesDir,
extraFiles: <String>['test/empty_test.dart'],
);
package.directory.childFile('dart_test.yaml').writeAsStringSync('''
test_on: unknown
''');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['dart-test', '--platform=vm'],
errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());

expect(
output,
containsAllInOrder(
<Matcher>[
contains('Unknown "test_on" value: "unknown"\n'
"If this value needs to be supported for this package's "
'tests, please update the repository tooling to support more '
'test_on modes.'),
],
));
});

test('throws for an valid but complex test_on directive', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
packagesDir,
extraFiles: <String>['test/empty_test.dart'],
);
package.directory.childFile('dart_test.yaml').writeAsStringSync('''
test_on: vm && browser
''');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['dart-test', '--platform=vm'],
errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());

expect(
output,
containsAllInOrder(
<Matcher>[
contains('Unknown "test_on" value: "vm && browser"\n'
"If this value needs to be supported for this package's "
'tests, please update the repository tooling to support more '
'test_on modes.'),
],
));
});

test('runs in Chrome when requested for Flutter package', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
Expand All @@ -265,7 +327,12 @@ void main() {
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['test', '--color', '--platform=chrome', '--web-renderer=html'],
const <String>[
'test',
'--color',
'--platform=chrome',
'--web-renderer=html'
],
package.path),
]),
);
Expand All @@ -289,7 +356,12 @@ void main() {
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['test', '--color', '--platform=chrome', '--web-renderer=html'],
const <String>[
'test',
'--color',
'--platform=chrome',
'--web-renderer=html'
],
plugin.path),
]),
);
Expand All @@ -314,7 +386,12 @@ void main() {
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['test', '--color', '--platform=chrome', '--web-renderer=html'],
const <String>[
'test',
'--color',
'--platform=chrome',
'--web-renderer=html'
],
plugin.path),
]),
);
Expand All @@ -339,7 +416,12 @@ void main() {
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['test', '--color', '--platform=chrome', '--web-renderer=html'],
const <String>[
'test',
'--color',
'--platform=chrome',
'--web-renderer=html'
],
plugin.path),
]),
);
Expand Down Expand Up @@ -409,7 +491,12 @@ void main() {
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['test', '--color', '--platform=chrome', '--web-renderer=html'],
const <String>[
'test',
'--color',
'--platform=chrome',
'--web-renderer=html'
],
plugin.path),
]),
);
Expand Down Expand Up @@ -459,6 +546,30 @@ test_on: vm
);
});

test('does not skip running vm in vm mode', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
packagesDir,
extraFiles: <String>['test/empty_test.dart'],
);
package.directory.childFile('dart_test.yaml').writeAsStringSync('''
test_on: vm
''');

final List<String> output = await runCapturingPrint(
runner, <String>['dart-test', '--platform=vm']);

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Package has opted out'),
])));
expect(
processRunner.recordedCalls,
isNotEmpty,
);
});

test('skips running in vm mode if package opts out', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
Expand All @@ -483,6 +594,30 @@ test_on: browser
);
});

test('does not skip running browser in browser mode', () async {
final RepositoryPackage package = createFakePackage(
'a_package',
packagesDir,
extraFiles: <String>['test/empty_test.dart'],
);
package.directory.childFile('dart_test.yaml').writeAsStringSync('''
test_on: browser
''');

final List<String> output = await runCapturingPrint(
runner, <String>['dart-test', '--platform=browser']);

expect(
output,
isNot(containsAllInOrder(<Matcher>[
contains('Package has opted out'),
])));
expect(
processRunner.recordedCalls,
isNotEmpty,
);
});

test('tries to run for a test_on that the tool does not recognize',
() async {
final RepositoryPackage package = createFakePackage(
Expand Down

0 comments on commit 69e7fc1

Please sign in to comment.