From 821cb4362f918672ec3c0a5a94fbbf16087f4f19 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 26 Jun 2024 16:26:39 -0400 Subject: [PATCH 1/2] [tools] Fix vm test requirement 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 explictly fail for anything unrecognized, to make it much harder to re-introduce a bug like this in the future. --- script/tool/lib/src/dart_test_command.dart | 72 +++++---- script/tool/test/dart_test_command_test.dart | 145 ++++++++++++++++++- 2 files changed, 184 insertions(+), 33 deletions(-) diff --git a/script/tool/lib/src/dart_test_command.dart b/script/tool/lib/src/dart_test_command.dart index 1c188ff38ad..b782abd1102 100644 --- a/script/tool/lib/src/dart_test_command.dart +++ b/script/tool/lib/src/dart_test_command.dart @@ -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. @@ -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.'); @@ -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.'); @@ -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); } @@ -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; } } diff --git a/script/tool/test/dart_test_command_test.dart b/script/tool/test/dart_test_command_test.dart index d56bb44b0fb..adbaf782bd3 100644 --- a/script/tool/test/dart_test_command_test.dart +++ b/script/tool/test/dart_test_command_test.dart @@ -249,6 +249,68 @@ void main() { ); }); + test('throws for an unrecognized test_on type', () async { + final RepositoryPackage package = createFakePackage( + 'a_package', + packagesDir, + extraFiles: ['test/empty_test.dart'], + ); + package.directory.childFile('dart_test.yaml').writeAsStringSync(''' +test_on: unknown +'''); + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['dart-test', '--platform=vm'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + + expect( + output, + containsAllInOrder( + [ + 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: ['test/empty_test.dart'], + ); + package.directory.childFile('dart_test.yaml').writeAsStringSync(''' +test_on: vm && browser +'''); + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['dart-test', '--platform=vm'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + + expect( + output, + containsAllInOrder( + [ + 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', @@ -265,7 +327,12 @@ void main() { orderedEquals([ ProcessCall( getFlutterCommand(mockPlatform), - const ['test', '--color', '--platform=chrome', '--web-renderer=html'], + const [ + 'test', + '--color', + '--platform=chrome', + '--web-renderer=html' + ], package.path), ]), ); @@ -289,7 +356,12 @@ void main() { orderedEquals([ ProcessCall( getFlutterCommand(mockPlatform), - const ['test', '--color', '--platform=chrome', '--web-renderer=html'], + const [ + 'test', + '--color', + '--platform=chrome', + '--web-renderer=html' + ], plugin.path), ]), ); @@ -314,7 +386,12 @@ void main() { orderedEquals([ ProcessCall( getFlutterCommand(mockPlatform), - const ['test', '--color', '--platform=chrome', '--web-renderer=html'], + const [ + 'test', + '--color', + '--platform=chrome', + '--web-renderer=html' + ], plugin.path), ]), ); @@ -339,7 +416,12 @@ void main() { orderedEquals([ ProcessCall( getFlutterCommand(mockPlatform), - const ['test', '--color', '--platform=chrome', '--web-renderer=html'], + const [ + 'test', + '--color', + '--platform=chrome', + '--web-renderer=html' + ], plugin.path), ]), ); @@ -409,7 +491,12 @@ void main() { orderedEquals([ ProcessCall( getFlutterCommand(mockPlatform), - const ['test', '--color', '--platform=chrome', '--web-renderer=html'], + const [ + 'test', + '--color', + '--platform=chrome', + '--web-renderer=html' + ], plugin.path), ]), ); @@ -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: ['test/empty_test.dart'], + ); + package.directory.childFile('dart_test.yaml').writeAsStringSync(''' +test_on: vm +'''); + + final List output = await runCapturingPrint( + runner, ['dart-test', '--platform=vm']); + + expect( + output, + isNot(containsAllInOrder([ + 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', @@ -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: ['test/empty_test.dart'], + ); + package.directory.childFile('dart_test.yaml').writeAsStringSync(''' +test_on: browser +'''); + + final List output = await runCapturingPrint( + runner, ['dart-test', '--platform=browser']); + + expect( + output, + isNot(containsAllInOrder([ + 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( From 5af1338720016fadb7ac4a88dc8447de44c1e26f Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 26 Jun 2024 19:21:26 -0400 Subject: [PATCH 2/2] Update stale package dependencies in flutter_migrate --- packages/flutter_migrate/pubspec.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/flutter_migrate/pubspec.yaml b/packages/flutter_migrate/pubspec.yaml index 4b4090b80ae..e0aea7889b2 100644 --- a/packages/flutter_migrate/pubspec.yaml +++ b/packages/flutter_migrate/pubspec.yaml @@ -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