feat(test): preserve test annotations in optimized bundle

.github/workflows/e2e.yaml

@@ -32,6 +32,7 @@ jobs:
           - test/commands/test/async_main/async_main_test.dart
           - test/commands/test/compilation_error/compilation_error_test.dart
           - test/commands/test/no_project/no_project_test.dart
+          - test/commands/test/optimization_annotations/optimization_annotations_test.dart
           - test/commands/test/optimization_exclude/optimization_exclude_test.dart
           - test/commands/test/spaced_golden_file_name/spaced_golden_file_name_test.dart
           - test/commands/test/very_good_config/very_good_config_test.dart

bricks/test_optimizer/hooks/lib/pre_gen.dart

@@ -1,18 +1,25 @@
 import 'dart:io';
 
 import 'package:hooks/dart_identifier_generator.dart';
+import 'package:hooks/test_metadata.dart';
 import 'package:mason/mason.dart';
 import 'package:path/path.dart' as path;
 
 typedef ExitFn = Never Function(int code);
 
 ExitFn exitFn = exit;
 
-String skipVeryGoodOptimizationTag = 'skip_very_good_optimization';
-RegExp skipVeryGoodOptimizationRegExp = RegExp(
-  "@Tags\\s*\\(\\s*\\[[\\s\\S]*?[\"']$skipVeryGoodOptimizationTag[\"'][\\s\\S]*?\\]\\s*\\)",
-  multiLine: true,
-);
+/// The tag that opts a test file out of the optimized bundle.
+const skipVeryGoodOptimizationTag = 'skip_very_good_optimization';
+
+extension TestMetadataBundle on TestMetadata {
+  bool get skipsOptimization => tagNames.contains(skipVeryGoodOptimizationTag);
+
+  /// The named arguments to append to a `group` call, each prefixed by `, `.
+  String get groupArguments {
+    return arguments.entries.map((e) => ', ${e.key}: ${e.value}').join();
+  }
+}
 
 Future<void> run(HookContext context) async {
   final packageRoot = context.vars['package-root'] as String;
@@ -34,69 +41,63 @@ Future<void> run(HookContext context) async {
   final isFlutter = flutterSdkRegExp.hasMatch(pubspecContents);
 
   final identifierGenerator = DartIdentifierGenerator();
-  final testIdentifierTable = <Map<String, String>>[];
+  final optimizedTests = <Map<String, String>>[];
+  final notOptimizedTests = <String>[];
+
   final tests = testDir
       .listSync(recursive: true)
-      .where((entity) => entity.isTest);
-
-  final notOptimizedTests = await getNotOptimizedTests(tests, testDir.path);
+      .where((entity) => entity.isTest)
+      .cast<File>();
+  final parsedTests = await Future.wait(tests.map(_parse));
 
-  for (final entity in tests) {
+  for (final (file, content, metadata) in parsedTests) {
     final relativePath = path
-        .relative(entity.path, from: testDir.path)
+        .relative(file.path, from: testDir.path)
         .replaceAll(r'\', '/');
-    testIdentifierTable.add({
+
+    if (metadata.skipsOptimization) {
+      notOptimizedTests.add(relativePath);
+      continue;
+    }
+
+    if (content.contains(skipVeryGoodOptimizationTag)) {
+      context.logger.warn(
+        '$relativePath names $skipVeryGoodOptimizationTag but was optimized '
+        'anyway: package:test reads @Tags only from the metadata of the '
+        "file's first directive.",
+      );
+    }
+
+    for (final annotation in metadata.droppedAnnotations) {
+      context.logger.warn(
+        '$relativePath: left $annotation out of the optimized bundle, which '
+        'cannot resolve every name it references.',
+      );
+    }
+
+    optimizedTests.add({
       'path': relativePath,
       'identifier': identifierGenerator.next(),
+      'groupArguments': metadata.groupArguments,
     });
   }
 
-  final optimizedTestsIdentifierTable = testIdentifierTable
-      .where((e) => !notOptimizedTests.contains(e['path']))
-      .toList();
-
   context.vars = {
-    'tests': optimizedTestsIdentifierTable,
+    'tests': optimizedTests,
     'isFlutter': isFlutter,
     'notOptimizedTests': notOptimizedTests,
   };
 }
 
+typedef _ParsedTest = (File file, String content, TestMetadata metadata);
+
+Future<_ParsedTest> _parse(File file) async {
+  final content = await file.readAsString();
+  return (file, content, parseTestMetadata(content, path: file.path));
+}
+
 extension on FileSystemEntity {
   bool get isTest {
     return this is File && path.basename(this.path).endsWith('_test.dart');
   }
 }
-
-Future<List<String>> getNotOptimizedTests(
-  Iterable<FileSystemEntity> tests,
-  String testDir,
-) async {
-  final paths = tests.map((e) => e.path).toList();
-  final formattedPaths = paths.map((e) => e.replaceAll('/./', '/')).toList();
-
-  final fileFutures = formattedPaths.map(_checkFileForSkipVeryGoodOptimization);
-  final fileResults = await Future.wait(fileFutures);
-
-  final testWithVeryGoodTest = <String>[];
-  for (var i = 0; i < formattedPaths.length; i++) {
-    if (fileResults[i]) {
-      testWithVeryGoodTest.add(formattedPaths[i]);
-    }
-  }
-
-  /// Format to relative path
-  final relativePaths = testWithVeryGoodTest
-      .map((e) => path.relative(e, from: testDir))
-      .toList();
-
-  return relativePaths;
-}
-
-/// Check if a single file contains skip_very_good_optimization tag
-Future<bool> _checkFileForSkipVeryGoodOptimization(String path) async {
-  final file = File(path);
-  if (!file.existsSync()) return false;
-  final content = await file.readAsString();
-  return skipVeryGoodOptimizationRegExp.hasMatch(content);
-}

bricks/test_optimizer/hooks/lib/test_metadata.dart

@@ -0,0 +1,189 @@
+import 'package:analyzer/dart/analysis/utilities.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/visitor.dart';
+
+/// {@template test_metadata}
+/// The library level annotations `package:test` reads from a test file, held
+/// as source text so they can be forwarded to the `group` that wraps the file
+/// in the optimized bundle.
+/// {@endtemplate}
+class TestMetadata {
+  /// {@macro test_metadata}
+  const new({
+    this.arguments = const {},
+    this.tagNames = const {},
+    this.droppedAnnotations = const [],
+  });
+
+  /// `group` arguments keyed by parameter name, in a fixed order rather than
+  /// the order the file wrote its annotations.
+  final Map<String, String> arguments;
+
+  /// The string literal tags of a `@Tags` annotation, read even when the
+  /// annotation itself was dropped.
+  final Set<String> tagNames;
+
+  /// The annotations left out because the bundle cannot resolve them.
+  final List<String> droppedAnnotations;
+}
+
+/// The names an annotation argument may reference.
+///
+/// The bundle imports only `package:test` (or `package:flutter_test`) and
+/// `dart:core`, so an argument naming anything else does not resolve there.
+const _resolvableNames = {
+  'Duration',
+  'Timeout',
+  'Skip',
+  'factor',
+  'none',
+  'zero',
+};
+
+/// The `group` parameter each forwarded annotation supplies.
+///
+/// Iteration order fixes the order [TestMetadata.arguments] emits.
+const _groupParameters = {
+  'TestOn': 'testOn',
+  'Timeout': 'timeout',
+  'Skip': 'skip',
+  'Tags': 'tags',
+  'OnPlatform': 'onPlatform',
+  'Retry': 'retry',
+};
+
+/// Parses the library level test annotations of [content], the source of the
+/// test file at [path]. Only the first directive's metadata is read, the one
+/// place `package:test` looks.
+///
+/// An annotation naming something the bundle cannot resolve, as in
+/// `@Timeout(kSlowSuite)`, is recorded in [TestMetadata.droppedAnnotations]
+/// instead of being forwarded, so one file cannot break the whole bundle.
+TestMetadata parseTestMetadata(String content, {String? path}) {
+  final unit = parseString(
+    content: content,
+    path: path,
+    throwIfDiagnostics: false,
+  ).unit;
+
+  final directives = unit.directives;
+  if (directives.isEmpty) return const TestMetadata();
+
+  final prefixes = directives
+      .whereType<ImportDirective>()
+      .map((directive) => directive.prefix?.name)
+      .nonNulls
+      .toSet();
+
+  var tagNames = const <String>{};
+  final droppedAnnotations = <String>[];
+  final sources = <String, String>{};
+
+  for (final annotation in directives.first.metadata) {
+    final (className, constructorName) = _resolveConstructor(
+      annotation.name,
+      annotation.constructorName,
+      prefixes,
+    );
+    if (!_groupParameters.containsKey(className)) continue;
+
+    final arguments = annotation.arguments;
+
+    if (className == 'Tags') {
+      final argument = arguments?.arguments.firstOrNull;
+      if (argument is ListLiteral) {
+        tagNames = argument.elements
+            .whereType<StringLiteral>()
+            .map((element) => element.stringValue)
+            .nonNulls
+            .toSet();
+      }
+    }
+
+    if (!_isResolvable(arguments, constructorName)) {
+      droppedAnnotations.add(annotation.toSource());
+      continue;
+    }
+
+    final source = _argumentSource(className, constructorName, arguments);
+    if (source != null) sources[className] = source;
+  }
+
+  return TestMetadata(
+    arguments: {
+      for (final MapEntry(key: annotation, value: parameter)
+          in _groupParameters.entries)
+        parameter: ?sources[annotation],
+    },
+    tagNames: tagNames,
+    droppedAnnotations: droppedAnnotations,
+  );
+}
+
+/// The source forwarded for one annotation, or `null` when it has none, as for
+/// a bare `@Timeout` or `@Skip` that names the type rather than a value.
+String? _argumentSource(
+  String className,
+  String? constructorName,
+  ArgumentList? arguments,
+) {
+  final argument = arguments?.arguments.firstOrNull;
+  return switch (className) {
+    'Timeout' when arguments == null && constructorName == null => null,
+    'Timeout' =>
+      'Timeout${constructorName == null ? '' : '.$constructorName'}'
+          '${arguments?.toSource() ?? ''}',
+    'Skip' when arguments == null => null,
+    'Skip' => argument?.toSource() ?? 'true',
+    _ => argument?.toSource(),
+  };
+}
+
+/// Whether every name [arguments] and [constructorName] reference resolves in
+/// the optimized bundle.
+bool _isResolvable(ArgumentList? arguments, String? constructorName) {
+  final referenced = _ReferencedNames();
+  arguments?.accept(referenced);
+  return [
+    ?constructorName,
+    ...referenced.names,
+  ].every(_resolvableNames.contains);
+}
+
+/// Collects every name an expression references.
+///
+/// Named argument labels are tokens rather than identifiers, so `minutes` in
+/// `Duration(minutes: 5)` is not collected, while `Duration` is.
+class _ReferencedNames extends RecursiveAstVisitor<void> {
+  final names = <String>[];
+
+  @override
+  void visitSimpleIdentifier(SimpleIdentifier node) => names.add(node.name);
+
+  @override
+  void visitNamedType(NamedType node) {
+    names.add(node.name.lexeme);
+    super.visitNamedType(node);
+  }
+}
+
+/// Resolves the class and named constructor an annotation refers to.
+///
+/// The syntax is ambiguous, so [prefixes] disambiguates: `@Timeout.none` names
+/// a constructor, while `@test.Skip()` names `Skip` behind the `test` prefix.
+(String, String?) _resolveConstructor(
+  Identifier name,
+  SimpleIdentifier? constructorName,
+  Set<String> prefixes,
+) {
+  if (name is PrefixedIdentifier &&
+      !prefixes.contains(name.prefix.name) &&
+      constructorName == null) {
+    return (name.prefix.name, name.identifier.name);
+  }
+
+  final className = name is PrefixedIdentifier
+      ? name.identifier.name
+      : name.name;
+  return (className, constructorName?.name);
+}

bricks/test_optimizer/hooks/pubspec.yaml

@@ -5,6 +5,7 @@ environment:
   sdk: ^3.13.0
 
 dependencies:
+  analyzer: ^14.0.0
   mason: ^0.1.0
   path: ^1.8.1