Require a definition named main in test suites (#2569)

Closes #2567

The error when there is a compilation failure due to a missing `main` is
less direct than it could be and exposes details of wrapping the test
suite in another library. Add an explicit check for a function declared
with that name when parsing metadata before attempting any compiles.

Add a check to `parseMetadata` that throws a `FormatException` if there
is no function declared directly in the library named `main`. This will
prevent some previously working designs such as exposing a main in a
part or with `export`, but we do not expect those designs are used in
practice.

Author: natebosch

pkgs/test/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 * Add `--coverage-package` flag, which filters the coverage report to specific
   packages using RegExps.
+* Require a function definition named `main` directly in a test suite and
+  provide a more direct error message than a failing compiler output.
 
 ## 1.28.0
 

pkgs/test/test/runner/browser/runner_test.dart

@@ -33,7 +33,7 @@ void main() {
 
   group('fails gracefully if', () {
     test('a test file fails to compile', () async {
-      await d.file('test.dart', 'invalid Dart file').create();
+      await d.file('test.dart', 'void main() {invalid Dart}').create();
       var test = await runTest(['-p', 'chrome', 'test.dart']);
 
       expect(

pkgs/test/test/runner/node/runner_test.dart

@@ -78,7 +78,7 @@ void main() {
 
   group('fails gracefully if', () {
     test('a test file fails to compile', () async {
-      await d.file('test.dart', 'invalid Dart file').create();
+      await d.file('test.dart', 'void main() {invalid Dart}').create();
       var test = await runTest(['-p', 'node', 'test.dart']);
 
       expect(

pkgs/test/test/runner/parse_metadata_test.dart

@@ -14,16 +14,16 @@ import 'package:test_core/src/runner/parse_metadata.dart';
 final _path = 'test.dart';
 
 void main() {
-  test('returns empty metadata for an empty file', () {
-    var metadata = parseMetadata(_path, '', {});
+  test('returns empty metadata for a file without annotations', () {
+    var metadata = parseMetadata(_path, 'main(){}', {});
     expect(metadata.testOn, equals(PlatformSelector.all));
     expect(metadata.timeout.scaleFactor, equals(1));
   });
 
   test('ignores irrelevant annotations', () {
     var metadata = parseMetadata(
       _path,
-      '@Fblthp\n@Fblthp.foo\nlibrary foo;',
+      '@Fblthp\n@Fblthp.foo\nlibrary foo;main(){}',
       {},
     );
     expect(metadata.testOn, equals(PlatformSelector.all));
@@ -33,7 +33,8 @@ void main() {
     var metadata = parseMetadata(
       _path,
       "@foo.TestOn('vm')\n"
-      "import 'package:test/test.dart' as foo;",
+      "import 'package:test/test.dart' as foo;\n"
+      'main(){}',
       {},
     );
     expect(
@@ -46,9 +47,20 @@ void main() {
     );
   });
 
+  test('throws for missing definition of `main`', () {
+    expect(
+      () => parseMetadata(_path, 'void notMain() {}', {}),
+      throwsFormatException,
+    );
+  });
+
   group('@TestOn:', () {
     test('parses a valid annotation', () {
-      var metadata = parseMetadata(_path, "@TestOn('vm')\nlibrary foo;", {});
+      var metadata = parseMetadata(
+        _path,
+        "@TestOn('vm')\nlibrary foo;\nmain(){}",
+        {},
+      );
       expect(
         metadata.testOn.evaluate(SuitePlatform(Runtime.vm, compiler: null)),
         isTrue,
@@ -62,7 +74,7 @@ void main() {
     test('ignores a constructor named TestOn', () {
       var metadata = parseMetadata(
         _path,
-        "@foo.TestOn('foo')\nlibrary foo;",
+        "@foo.TestOn('foo')\nlibrary foo;\nmain(){}",
         {},
       );
       expect(metadata.testOn, equals(PlatformSelector.all));
@@ -93,6 +105,7 @@ void main() {
     microseconds: 5))
 
 library foo;
+main(){}
 ''', {});
       expect(
         metadata.timeout.duration,
@@ -118,6 +131,7 @@ library foo;
     microseconds: 5))
 
 library foo;
+main(){}
 ''', {});
       expect(
         metadata.timeout.duration,
@@ -142,6 +156,7 @@ library foo;
     milliseconds: 4,
     microseconds: 5))
 import 'dart:core' as core;
+main(){}
 ''', {});
       expect(
         metadata.timeout.duration,
@@ -162,6 +177,7 @@ import 'dart:core' as core;
 @Timeout.factor(1)
 
 library foo;
+main(){}
 ''', {});
       expect(metadata.timeout.scaleFactor, equals(1));
     });
@@ -170,6 +186,7 @@ library foo;
       var metadata = parseMetadata(_path, '''
 @test.Timeout.factor(1)
 import 'package:test/test.dart' as test;
+main(){}
 ''', {});
       expect(metadata.timeout.scaleFactor, equals(1));
     });
@@ -179,6 +196,7 @@ import 'package:test/test.dart' as test;
 @Timeout.factor(0.5)
 
 library foo;
+main(){}
 ''', {});
       expect(metadata.timeout.scaleFactor, equals(0.5));
     });
@@ -188,14 +206,15 @@ library foo;
 @Timeout.none
 
 library foo;
+main(){}
 ''', {});
       expect(metadata.timeout, same(Timeout.none));
     });
 
     test('ignores a constructor named Timeout', () {
       var metadata = parseMetadata(
         _path,
-        "@foo.Timeout('foo')\nlibrary foo;",
+        "@foo.Timeout('foo')\nlibrary foo;\nmain(){}",
         {},
       );
       expect(metadata.timeout.scaleFactor, equals(1));
@@ -217,19 +236,31 @@ library foo;
 
   group('@Skip:', () {
     test('parses a valid annotation', () {
-      var metadata = parseMetadata(_path, '@Skip()\nlibrary foo;', {});
+      var metadata = parseMetadata(
+        _path,
+        '@Skip()\nlibrary foo;\nmain(){}',
+        {},
+      );
       expect(metadata.skip, isTrue);
       expect(metadata.skipReason, isNull);
     });
 
     test('parses a valid annotation with a reason', () {
-      var metadata = parseMetadata(_path, "@Skip('reason')\nlibrary foo;", {});
+      var metadata = parseMetadata(
+        _path,
+        "@Skip('reason')\nlibrary foo;\nmain(){}",
+        {},
+      );
       expect(metadata.skip, isTrue);
       expect(metadata.skipReason, equals('reason'));
     });
 
     test('ignores a constructor named Skip', () {
-      var metadata = parseMetadata(_path, "@foo.Skip('foo')\nlibrary foo;", {});
+      var metadata = parseMetadata(
+        _path,
+        "@foo.Skip('foo')\nlibrary foo;\nmain(){}",
+        {},
+      );
       expect(metadata.skip, isFalse);
     });
 
@@ -249,12 +280,20 @@ library foo;
 
   group('@Tags:', () {
     test('parses a valid annotation', () {
-      var metadata = parseMetadata(_path, "@Tags(['a'])\nlibrary foo;", {});
+      var metadata = parseMetadata(
+        _path,
+        "@Tags(['a'])\nlibrary foo;\nmain(){}",
+        {},
+      );
       expect(metadata.tags, equals(['a']));
     });
 
     test('ignores a constructor named Tags', () {
-      var metadata = parseMetadata(_path, "@foo.Tags(['a'])\nlibrary foo;", {});
+      var metadata = parseMetadata(
+        _path,
+        "@foo.Tags(['a'])\nlibrary foo;\nmain(){}",
+        {},
+      );
       expect(metadata.tags, isEmpty);
     });
 
@@ -290,7 +329,8 @@ library foo;
   'chrome': Timeout.factor(2),
   'vm': [Skip(), Timeout.factor(3)]
 })
-library foo;''', {});
+library foo;
+main(){}''', {});
 
       var key = metadata.onPlatform.keys.first;
       expect(
@@ -319,6 +359,7 @@ library foo;''', {});
   'vm': [test.Skip(), test.Timeout.factor(3)]
 })
 import 'package:test/test.dart' as test;
+main(){}
 ''', {});
 
       var key = metadata.onPlatform.keys.first;
@@ -344,7 +385,7 @@ import 'package:test/test.dart' as test;
     test('ignores a constructor named OnPlatform', () {
       var metadata = parseMetadata(
         _path,
-        "@foo.OnPlatform('foo')\nlibrary foo;",
+        "@foo.OnPlatform('foo')\nlibrary foo;main(){}",
         {},
       );
       expect(metadata.testOn, equals(PlatformSelector.all));

pkgs/test/test/runner/precompiled_test.dart

@@ -107,7 +107,9 @@ void main() {
         preamble.getPreamble(minified: true) + await jsFile.readAsString(),
       );
 
-      await d.dir('test', [d.file('test.dart', 'invalid dart}')]).create();
+      await d.dir('test', [
+        d.file('test.dart', 'void main() {invalid dart}'),
+      ]).create();
     });
 
     test(
@@ -278,5 +280,5 @@ Future<void> _precompileBrowserTest(String testPath) async {
   ], workingDirectory: d.sandbox);
   await dart2js.shouldExit(0);
 
-  await d.file(testPath, 'invalid dart}').create();
+  await d.file(testPath, 'void main() {invalid dart}').create();
 }

pkgs/test/test/runner/runner_test.dart

@@ -186,15 +186,14 @@ $_usage''');
     });
 
     test('a test file fails to load', () async {
-      await d.file('test.dart', 'invalid Dart file').create();
+      await d.file('test.dart', 'void main(){invalid Dart}').create();
       var test = await runTest(['test.dart']);
 
       expect(
         test.stdout,
         containsInOrder([
           'Failed to load "test.dart":',
-          "test.dart:1:9: Error: Expected ';' after this.",
-          'invalid Dart file',
+          "Error: Expected ';' after this.",
         ]),
       );
 
@@ -222,7 +221,7 @@ $_usage''');
     // This is slightly different from the above test because it's an error
     // that's caught first by the analyzer when it's used to parse the file.
     test('a test file fails to parse', () async {
-      await d.file('test.dart', '@TestOn)').create();
+      await d.file('test.dart', '@TestOn) void main() {}').create();
       var test = await runTest(['test.dart']);
 
       expect(
@@ -239,7 +238,9 @@ $_usage''');
     });
 
     test("an annotation's contents are invalid", () async {
-      await d.file('test.dart', "@TestOn('zim')\nlibrary foo;").create();
+      await d
+          .file('test.dart', "@TestOn('zim')\nlibrary foo;\nvoid main() {}")
+          .create();
       var test = await runTest(['test.dart']);
 
       expect(
@@ -276,12 +277,7 @@ $_usage''');
       expect(test.stdout, emitsThrough(contains('-1: loading test.dart [E]')));
       expect(
         test.stdout,
-        emitsThrough(
-          anyOf([
-            contains("Error: Getter not found: 'main'"),
-            contains("Error: Undefined name 'main'"),
-          ]),
-        ),
+        emitsThrough(contains('Missing definition of `main` method')),
       );
 
       await test.shouldExit(1);
@@ -294,18 +290,7 @@ $_usage''');
       expect(test.stdout, emitsThrough(contains('-1: loading test.dart [E]')));
       expect(
         test.stdout,
-        emitsThrough(
-          anyOf([
-            contains(
-              "A value of type 'int' can't be assigned to a variable of type "
-              "'Function'",
-            ),
-            contains(
-              "A value of type 'int' can't be returned from a function with "
-              "return type 'Function'",
-            ),
-          ]),
-        ),
+        emitsThrough(contains('Missing definition of `main` method')),
       );
 
       await test.shouldExit(1);

pkgs/test_core/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 * Add `--coverage-package` flag, which filters the coverage report to specific
   packages using RegExps.
+* Require a function definition named `main` directly in a test suite and
+  provide a more direct error message than a failing compiler output.
 
 ## 0.6.14
 

pkgs/test_core/lib/src/runner/parse_metadata.dart

@@ -20,7 +20,7 @@ import '../util/dart.dart';
 /// allowed everywhere.
 ///
 /// Throws an [AnalysisError] if parsing fails or a [FormatException] if the
-/// test annotations are incorrect.
+/// test suite is unrunnable due to incorrect annotations or a missing main.
 Metadata parseMetadata(
   String path,
   String contents,
@@ -48,6 +48,12 @@ class _Parser {
   /// The language version override comment if one was present, otherwise null.
   String? _languageVersionComment;
 
+  /// Whether any member of the compilation unit is named 'main'.
+  ///
+  /// When main is missing a call to [parse] will throw a [FormatException] with
+  /// a descriptive message.
+  late final bool _hasMain;
+
   _Parser(this._path, this._contents, this._platformVariables) {
     var result = parseString(
       content: _contents,
@@ -57,6 +63,9 @@ class _Parser {
     var directives = result.unit.directives;
     _annotations = directives.isEmpty ? [] : directives.first.metadata;
     _languageVersionComment = result.unit.languageVersionToken?.value();
+    _hasMain = result.unit.declarations.any(
+      (d) => d is FunctionDeclaration && d.name.lexeme == 'main',
+    );
 
     // We explicitly *don't* just look for "package:test" imports here,
     // because it could be re-exported from another library.
@@ -75,6 +84,9 @@ class _Parser {
 
   /// Parses the metadata.
   Metadata parse() {
+    if (!_hasMain) {
+      throw const FormatException('Missing definition of `main` method.');
+    }
     Timeout? timeout;
     PlatformSelector? testOn;
     Object? /*String|bool*/ skip;