Add language version comments to bootstrap files (#1240) (#1267)

* Add language version comments to bootstrap files (#1240)

- Adds a nullable `languageVersionComment` field to `Metadata`
- Adds a utility to get a language comment that matches the root packages version
- Adds explicit language version comments to all bootstrap files ensuring the versions matches that of the test (note this assumes tests live in the root package)
- Clean up the handling of serving bootstrap dart files in the browser platform which had diverged and was serving the wrong content.

Author: jakemac53

pkgs/test/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 1.15.0
+
+* Update bootstrapping logic to ensure the bootstrap library has
+  the same language version as the test.
+
 ## 1.14.7
 
 * Support the latest `package:coverage`.

pkgs/test/lib/src/runner/browser/platform.dart

@@ -164,23 +164,6 @@ class BrowserPlatform extends PlatformPlugin
   shelf.Response _wrapperHandler(shelf.Request request) {
     var path = p.fromUri(request.url);
 
-    if (path.endsWith('.browser_test.dart')) {
-      var testPath = p.basename(p.withoutExtension(p.withoutExtension(path)));
-      return shelf.Response.ok('''
-        import "package:stream_channel/stream_channel.dart";
-
-        import "package:test_core/src/runner/plugin/remote_platform_helpers.dart";
-        import "package:test/src/runner/browser/post_message_channel.dart";
-
-        import "$testPath" as test;
-
-        void main() {
-          var channel = serializeSuite(() => test.main, hidePrints: false);
-          postMessageChannel().pipe(channel);
-        }
-      ''', headers: {'Content-Type': 'application/dart'});
-    }
-
     if (path.endsWith('.html')) {
       var test = p.withoutExtension(path) + '.dart';
       var scriptBase = htmlEscape.convert(p.basename(test));
@@ -379,18 +362,27 @@ class BrowserPlatform extends PlatformPlugin
     return _compileFutures.putIfAbsent(dartPath, () async {
       var dir = Directory(_compiledDir).createTempSync('test_').path;
       var jsPath = p.join(dir, p.basename(dartPath) + '.browser_test.dart.js');
-
-      await _compilers.compile('''
+      var bootstrapContent = '''
+        ${suiteConfig.metadata.languageVersionComment ?? await rootPackageLanguageVersionComment}
         import "package:test/src/bootstrap/browser.dart";
 
         import "${p.toUri(p.absolute(dartPath))}" as test;
 
         void main() {
           internalBootstrapBrowserTest(() => test.main);
         }
-      ''', jsPath, suiteConfig);
+      ''';
+
+      await _compilers.compile(bootstrapContent, jsPath, suiteConfig);
       if (_closed) return;
 
+      var bootstrapUrl = p.toUri(p.relative(dartPath, from: _root)).path +
+          '.browser_test.dart';
+      _jsHandler.add(bootstrapUrl, (request) {
+        return shelf.Response.ok(bootstrapContent,
+            headers: {'Content-Type': 'application/dart'});
+      });
+
       var jsUrl = p.toUri(p.relative(dartPath, from: _root)).path +
           '.browser_test.dart.js';
       _jsHandler.add(jsUrl, (request) {

pkgs/test/lib/src/runner/node/platform.dart

@@ -153,6 +153,7 @@ class NodePlatform extends PlatformPlugin
     var dir = Directory(_compiledDir).createTempSync('test_').path;
     var jsPath = p.join(dir, p.basename(testPath) + '.node_test.dart.js');
     await _compilers.compile('''
+        ${suiteConfig.metadata.languageVersionComment ?? await rootPackageLanguageVersionComment}
         import "package:test/src/bootstrap/node.dart";
 
         import "${p.toUri(p.absolute(testPath))}" as test;

pkgs/test/pubspec.yaml

@@ -1,5 +1,5 @@
 name: test
-version: 1.14.7
+version: 1.15.0
 description: A full featured library for writing and running Dart tests.
 homepage: https://github.com/dart-lang/test/blob/master/pkgs/test
 
@@ -33,8 +33,8 @@ dependencies:
   webkit_inspection_protocol: ">=0.5.0 <0.8.0"
   yaml: ^2.0.0
   # Use an exact version until the test_api and test_core package are stable.
-  test_api: 0.2.16
-  test_core: 0.3.7
+  test_api: 0.2.17
+  test_core: 0.3.8
 
 dev_dependencies:
   fake_async: ^1.0.0

pkgs/test/test/io.dart

@@ -69,10 +69,13 @@ Future<TestProcess> runTest(Iterable<String> args,
     String fileReporter,
     int concurrency,
     Map<String, String> environment,
-    bool forwardStdio = false}) async {
+    bool forwardStdio = false,
+    String packageConfig,
+    Iterable<String> vmArgs}) async {
   concurrency ??= 1;
 
   var allArgs = [
+    ...?vmArgs,
     p.absolute(p.join(await packageDir, 'bin/test.dart')),
     '--concurrency=$concurrency',
     if (reporter != null) '--reporter=$reporter',
@@ -86,18 +89,23 @@ Future<TestProcess> runTest(Iterable<String> args,
   return await runDart(allArgs,
       environment: environment,
       description: 'dart bin/test.dart',
-      forwardStdio: forwardStdio);
+      forwardStdio: forwardStdio,
+      packageConfig: packageConfig);
 }
 
 /// Runs Dart.
+///
+/// If [packageConfig] is provided then that is passed for the `--packages`
+/// arg, otherwise the current isolate config is passed.
 Future<TestProcess> runDart(Iterable<String> args,
     {Map<String, String> environment,
     String description,
-    bool forwardStdio = false}) async {
+    bool forwardStdio = false,
+    String packageConfig}) async {
   var allArgs = <String>[
     ...Platform.executableArguments.where((arg) =>
         !arg.startsWith('--package-root=') && !arg.startsWith('--packages=')),
-    '--packages=${await Isolate.packageConfig}',
+    '--packages=${packageConfig ?? await Isolate.packageConfig}',
     ...args
   ];
 

pkgs/test/test/runner/runner_test.dart

@@ -4,9 +4,14 @@
 
 @TestOn('vm')
 
+import 'dart:convert';
 import 'dart:io';
+import 'dart:isolate';
 import 'dart:math' as math;
 
+import 'package:package_config/package_config.dart';
+import 'package:path/path.dart' as p;
+
 import 'package:test/test.dart';
 import 'package:test_core/src/util/exit_codes.dart' as exit_codes;
 import 'package:test_descriptor/test_descriptor.dart' as d;
@@ -696,4 +701,82 @@ void main(List<String> args) async {
       await test.shouldExit(0);
     });
   });
+
+  group('nnbd', () {
+    final _testContents = '''
+import 'package:test/test.dart';
+import 'opted_in.dart';
+
+void main() {
+  test("success", () {
+    foo = true;
+    expect(foo, true);
+  });
+}''';
+
+    setUp(() async {
+      await d.file('opted_in.dart', '''
+// @dart=2.9
+bool? foo;''').create();
+    });
+
+    test('nnbd can be enabled in deps', () async {
+      await d.file('test.dart', '''
+// @dart=2.8
+$_testContents''').create();
+      var test = await runTest(['test.dart'],
+          packageConfig: (await Isolate.packageConfig).path,
+          vmArgs: ['--enable-experiment=non-nullable']);
+
+      expect(test.stdout, emitsThrough(contains('+1: All tests passed!')));
+      await test.shouldExit(0);
+    });
+
+    test('sound null safety is enabled if the entrypoint opts in', () async {
+      await d.file('test.dart', '''
+// @dart=2.9
+$_testContents''').create();
+      var test = await runTest(['test.dart'],
+          packageConfig: (await Isolate.packageConfig).path,
+          vmArgs: ['--enable-experiment=non-nullable']);
+
+      expect(
+          test.stdout,
+          containsInOrder([
+            'Unable to spawn isolate:',
+            'Error: A library can\'t opt out of non-nullable by default, when in nnbd-strong mode.'
+          ]));
+      await test.shouldExit(1);
+    });
+
+    test('sound null safety is enabled if the package is opted in', () async {
+      var currentPackageConfig =
+          await loadPackageConfigUri(await Isolate.packageConfig);
+      var newPackageConfig = PackageConfig([
+        ...currentPackageConfig.packages,
+        Package('example', Uri.file('${d.sandbox}/'),
+            languageVersion: LanguageVersion(2, 9),
+            // TODO: https://github.com/dart-lang/package_config/issues/81
+            packageUriRoot: Uri.file('${d.sandbox}/')),
+      ]);
+
+      await d.file('test.dart', _testContents).create();
+      await d
+          .file('package_config.json',
+              jsonEncode(PackageConfig.toJson(newPackageConfig)))
+          .create();
+
+      var test = await runTest(['test.dart'],
+          packageConfig: p.join(d.sandbox, 'package_config.json'),
+          vmArgs: ['--enable-experiment=non-nullable']);
+
+      expect(
+          test.stdout,
+          containsInOrder([
+            'Unable to spawn isolate:',
+            'Error: A library can\'t opt out of non-nullable by default, when in nnbd-strong mode.'
+          ]));
+      await test.shouldExit(1);
+    });
+  });
 }

pkgs/test_api/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.2.17
+
+* Add `languageVersionComment` on the `MetaData` class. This should only be
+  presen for test suites.
+
 ## 0.2.16
 
 * Deprecate `LiveTestController.liveTest`, the `LiveTestController` instance now

pkgs/test_api/lib/src/backend/metadata.dart

@@ -68,6 +68,11 @@ class Metadata {
   /// resolved when the metadata is constructed.
   final Map<BooleanSelector, Metadata> forTag;
 
+  /// The language version comment, if one is present.
+  ///
+  /// Only available for test suites and not individual tests.
+  final String languageVersionComment;
+
   /// Parses a user-provided map into the value for [onPlatform].
   static Map<PlatformSelector, Metadata> _parseOnPlatform(
       Map<String, dynamic> onPlatform) {
@@ -147,7 +152,8 @@ class Metadata {
       String skipReason,
       Iterable<String> tags,
       Map<PlatformSelector, Metadata> onPlatform,
-      Map<BooleanSelector, Metadata> forTag}) {
+      Map<BooleanSelector, Metadata> forTag,
+      String languageVersionComment}) {
     // Returns metadata without forTag resolved at all.
     Metadata _unresolved() => Metadata._(
         testOn: testOn,
@@ -159,7 +165,8 @@ class Metadata {
         skipReason: skipReason,
         tags: tags,
         onPlatform: onPlatform,
-        forTag: forTag);
+        forTag: forTag,
+        languageVersionComment: languageVersionComment);
 
     // If there's no tag-specific metadata, or if none of it applies, just
     // return the metadata as-is.
@@ -193,7 +200,8 @@ class Metadata {
       int retry,
       Iterable<String> tags,
       Map<PlatformSelector, Metadata> onPlatform,
-      Map<BooleanSelector, Metadata> forTag})
+      Map<BooleanSelector, Metadata> forTag,
+      this.languageVersionComment})
       : testOn = testOn ?? PlatformSelector.all,
         timeout = timeout ?? const Timeout.factor(1),
         _skip = skip,
@@ -220,7 +228,8 @@ class Metadata {
       bool chainStackTraces,
       int retry,
       Map<String, dynamic> onPlatform,
-      tags})
+      tags,
+      this.languageVersionComment})
       : testOn = testOn == null
             ? PlatformSelector.all
             : PlatformSelector.parse(testOn),
@@ -261,7 +270,8 @@ class Metadata {
         },
         forTag = (serialized['forTag'] as Map).map((key, nested) => MapEntry(
             BooleanSelector.parse(key as String),
-            Metadata.deserialize(nested)));
+            Metadata.deserialize(nested))),
+        languageVersionComment = serialized['languageVersionComment'] as String;
 
   /// Deserializes timeout from the format returned by [_serializeTimeout].
   static Timeout _deserializeTimeout(serialized) {
@@ -314,7 +324,9 @@ class Metadata {
       onPlatform: mergeMaps(onPlatform, other.onPlatform,
           value: (metadata1, metadata2) => metadata1.merge(metadata2)),
       forTag: mergeMaps(forTag, other.forTag,
-          value: (metadata1, metadata2) => metadata1.merge(metadata2)));
+          value: (metadata1, metadata2) => metadata1.merge(metadata2)),
+      languageVersionComment:
+          other.languageVersionComment ?? languageVersionComment);
 
   /// Returns a copy of [this] with the given fields changed.
   Metadata change(
@@ -327,7 +339,8 @@ class Metadata {
       String skipReason,
       Map<PlatformSelector, Metadata> onPlatform,
       Set<String> tags,
-      Map<BooleanSelector, Metadata> forTag}) {
+      Map<BooleanSelector, Metadata> forTag,
+      String languageVersionComment}) {
     testOn ??= this.testOn;
     timeout ??= this.timeout;
     skip ??= _skip;
@@ -338,6 +351,7 @@ class Metadata {
     onPlatform ??= this.onPlatform;
     tags ??= this.tags;
     forTag ??= this.forTag;
+    languageVersionComment ??= this.languageVersionComment;
     return Metadata(
         testOn: testOn,
         timeout: timeout,
@@ -348,7 +362,8 @@ class Metadata {
         onPlatform: onPlatform,
         tags: tags,
         forTag: forTag,
-        retry: retry);
+        retry: retry,
+        languageVersionComment: languageVersionComment);
   }
 
   /// Returns a copy of [this] with all platform-specific metadata from
@@ -384,7 +399,8 @@ class Metadata {
       'tags': tags.toList(),
       'onPlatform': serializedOnPlatform,
       'forTag': forTag.map((selector, metadata) =>
-          MapEntry(selector.toString(), metadata.serialize()))
+          MapEntry(selector.toString(), metadata.serialize())),
+      'languageVersionComment': languageVersionComment,
     };
   }
 

pkgs/test_api/pubspec.yaml

@@ -1,5 +1,5 @@
 name: test_api
-version: 0.2.16
+version: 0.2.17
 description: A library for writing Dart tests.
 homepage: https://github.com/dart-lang/test/blob/master/pkgs/test_api
 

pkgs/test_core/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 0.3.8
+
+* Update vm bootstrapping logic to ensure the bootstrap library has the same
+  language version as the test.
+* Populate `languageVersionComment` in the `Metadata` returned from
+  `parseMetadata`.
+
 ## 0.3.7
 
 * Support the latest `package:coverage`.