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.

Sign up for GitHub

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

Jump to bottom

[tools] Fix vm test requirement #6995

Merged
merged 2 commits into from
Jun 27, 2024
Merged

[tools] Fix vm test requirement #6995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
821cb43
[tools] Fix vm test requirement
stuartmorgan Jun 26, 2024
5af1338
Update stale package dependencies in flutter_migrate
stuartmorgan Jun 26, 2024
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
16 changes: 8 additions & 8 deletions packages/flutter_migrate/pubspec.yaml
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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