[tools] Fix vm test requirement (#6995)

stuartmorgan-g · web-flow · commit 69e7fc159064 · 2024-06-27T14:32:38.000Z
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.
diff --git 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
diff --git 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
@@ -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',
@@ -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),
         ]),
       );
@@ -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),
         ]),
       );
@@ -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),
         ]),
       );
@@ -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),
         ]),
       );
@@ -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),
         ]),
       );
@@ -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',
@@ -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(
