[modular_test] Remove uses of .packages altogether.

Embed in the modules.yaml any extra paths needed for package dependencies
instead.

This is one step to help towards #48275

Change-Id: I22ef02b2b2327a0c798f2fea73d59c758a8bb0bb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240651
Reviewed-by: Joshua Litt <[email protected]>
Commit-Queue: Sigmund Cherem <[email protected]>

Author: sigmundch

pkg/modular_test/lib/src/loader.dart

@@ -10,23 +10,13 @@
 ///   * individual .dart files, each file is considered a module. A
 ///   `main.dart` file is required as the entry point of the test.
 ///   * subfolders: each considered a module with multiple files
-///   * (optional) a .packages file:
-///       * if this is not specified, the test will use [defaultPackagesInput]
-///       instead.
-///       * if specified, it will be extended with the definitions in
-///       [defaultPackagesInput]. The list of packages provided is expected to
-///       be disjoint with those in [defaultPackagesInput].
 ///   * a modules.yaml file: a specification of dependencies between modules.
 ///     The format is described in `test_specification_parser.dart`.
 import 'dart:io';
-import 'dart:convert';
-import 'dart:typed_data';
 import 'suite.dart';
 import 'test_specification_parser.dart';
 import 'find_sdk_root.dart';
 
-import 'package:package_config/src/packages_file.dart' as packages_file;
-
 /// Returns the [ModularTest] associated with a folder under [uri].
 ///
 /// After scanning the contents of the folder, this method creates a
@@ -38,13 +28,12 @@ Future<ModularTest> loadTest(Uri uri) async {
   var folder = Directory.fromUri(uri);
   var testUri = folder.uri; // normalized in case the trailing '/' was missing.
   Uri root = await findRoot();
-  Map<String, Uri> defaultPackages =
-      _parseDotPackagesBytesToLibMap(_defaultPackagesInput, root);
+  final defaultTestSpecification = parseTestSpecification(_defaultPackagesSpec);
+  Set<String> defaultPackages = defaultTestSpecification.packages.keys.toSet();
   Module sdkModule = await _createSdkModule(root);
   Map<String, Module> modules = {'sdk': sdkModule};
   String specString;
   Module mainModule;
-  Map<String, Uri> packages = {};
   var entries = folder.listSync(recursive: false).toList()
     // Sort to avoid dependency on file system order.
     ..sort(_compareFileSystemEntity);
@@ -59,7 +48,7 @@ Future<ModularTest> loadTest(Uri uri) async {
               "'$moduleName' which conflicts with the sdk module "
               "that is provided by default.");
         }
-        if (defaultPackages.containsKey(moduleName)) {
+        if (defaultPackages.contains(moduleName)) {
           return _invalidTest("The file '$fileName' defines a module called "
               "'$moduleName' which conflicts with a package by the same name "
               "that is provided by default.");
@@ -75,9 +64,6 @@ Future<ModularTest> loadTest(Uri uri) async {
             packageBase: Uri.parse('.'));
         if (isMain) mainModule = module;
         modules[moduleName] = module;
-      } else if (fileName == '.packages') {
-        List<int> packagesBytes = await entry.readAsBytes();
-        packages = _parseDotPackagesBytesToLibMap(packagesBytes, entryUri);
       } else if (fileName == 'modules.yaml') {
         specString = await entry.readAsString();
       }
@@ -90,7 +76,7 @@ Future<ModularTest> loadTest(Uri uri) async {
             "which conflicts with the sdk module "
             "that is provided by default.");
       }
-      if (defaultPackages.containsKey(moduleName)) {
+      if (defaultPackages.contains(moduleName)) {
         return _invalidTest("The folder '$moduleName' defines a module "
             "which conflicts with a package by the same name "
             "that is provided by default.");
@@ -110,12 +96,17 @@ Future<ModularTest> loadTest(Uri uri) async {
     return _invalidTest("main module is missing");
   }
 
-  _addDefaultPackageEntries(packages, defaultPackages);
-  await _addModulePerPackage(packages, modules);
   TestSpecification spec = parseTestSpecification(specString);
+  for (final name in defaultPackages) {
+    if (spec.packages.containsKey(name)) {
+      _invalidTest(
+          ".packages file defines a conflicting entry for package '$name'.");
+    }
+  }
+  await _addModulePerPackage(defaultTestSpecification.packages, root, modules);
+  await _addModulePerPackage(spec.packages, testUri, modules);
   _attachDependencies(spec.dependencies, modules);
-  _attachDependencies(
-      parseTestSpecification(_defaultPackagesSpec).dependencies, modules);
+  _attachDependencies(defaultTestSpecification.dependencies, modules);
   _addSdkDependencies(modules, sdkModule);
   _detectCyclesAndRemoveUnreachable(modules, mainModule);
   var sortedModules = modules.values.toList()
@@ -170,33 +161,21 @@ void _addSdkDependencies(Map<String, Module> modules, Module sdkModule) {
   }
 }
 
-void _addDefaultPackageEntries(
-    Map<String, Uri> packages, Map<String, Uri> defaultPackages) {
-  for (var name in defaultPackages.keys) {
-    var existing = packages[name];
-    if (existing != null && existing != defaultPackages[name]) {
-      _invalidTest(
-          ".packages file defines an conflicting entry for package '$name'.");
-    }
-    packages[name] = defaultPackages[name];
-  }
-}
-
 /// Create a module for each package dependency.
-Future<void> _addModulePerPackage(
-    Map<String, Uri> packages, Map<String, Module> modules) async {
+Future<void> _addModulePerPackage(Map<String, String> packages, Uri configRoot,
+    Map<String, Module> modules) async {
   for (var packageName in packages.keys) {
     var module = modules[packageName];
     if (module != null) {
       module.isPackage = true;
     } else {
-      var packageLibUri = packages[packageName];
-      var rootUri = Directory.fromUri(packageLibUri).parent.uri;
+      var packageLibUri = configRoot.resolve(packages[packageName]);
+      var packageRootUri = Directory.fromUri(packageLibUri).parent.uri;
       var sources = await _listModuleSources(packageLibUri);
       // TODO(sigmund): validate that we don't use a different alias for a
       // module that is part of the test (package name and module name should
       // match).
-      modules[packageName] = Module(packageName, [], rootUri, sources,
+      modules[packageName] = Module(packageName, [], packageRootUri, sources,
           isPackage: true, packageBase: Uri.parse('lib/'), isShared: true);
     }
   }
@@ -249,15 +228,6 @@ _detectCyclesAndRemoveUnreachable(Map<String, Module> modules, Module main) {
   toRemove.forEach(modules.remove);
 }
 
-/// Default entries for a .packages file with paths relative to the SDK root.
-List<int> _defaultPackagesInput = utf8.encode('''
-expect:pkg/expect/lib
-smith:pkg/smith/lib
-async_helper:pkg/async_helper/lib
-meta:pkg/meta/lib
-collection:third_party/pkg/collection/lib
-''');
-
 /// Specifies the dependencies of all packages in [_defaultPackagesInput]. This
 /// string needs to be updated if dependencies between those packages changes
 /// (which is rare).
@@ -270,6 +240,12 @@ dependencies:
   meta: []
   async_helper: []
   collection: []
+packages:
+  expect: pkg/expect/lib
+  smith: pkg/smith/lib
+  async_helper: pkg/async_helper/lib
+  meta: pkg/meta/lib
+  collection: third_party/pkg/collection/lib
 ''';
 
 /// Report an conflict error.
@@ -316,15 +292,3 @@ int _compareFileSystemEntity(FileSystemEntity a, FileSystemEntity b) {
     }
   }
 }
-
-/// Parse [bytes] representing a `.packages` file into the map of package names
-/// to URIs of their `lib` locations.
-Map<String, Uri> _parseDotPackagesBytesToLibMap(Uint8List bytes, Uri baseUri) {
-  var map = <String, Uri>{};
-  var packageConfig =
-      packages_file.parse(bytes, baseUri, (error) => throw error);
-  for (var package in packageConfig.packages) {
-    map[package.name] = package.packageUriRoot;
-  }
-  return map;
-}

pkg/modular_test/lib/src/test_specification_parser.dart

@@ -10,9 +10,16 @@
 ///      main: [b, expect]
 ///    flags:
 ///      - constant-update-2018
+///    packages:
+///      c: .
+///      a: a
 ///
-/// Where the dependencies section describe how modules depend on one another,
-/// and the flags section show what flags are needed to run that specific test.
+///
+/// Where:
+///   - the dependencies section describe how modules depend on one another,
+///   - the flags section show what flags are needed to run that specific test,
+///   - the packages section is used to create a package structure on top of the
+///     declared modules.
 ///
 /// When defining dependencies:
 ///   - Each name corresponds to a module.
@@ -21,9 +28,24 @@
 ///   - If a module has a single dependency, it can be written as a single
 ///     value.
 ///
+/// When defining packages:
+///   - The name corresponds to a package name, this doesn't need to match
+///     the name of the module. That said, it's common for some modules
+///     and packages to share their name (especially for the default set of
+///     packages included by the framework, like package:expect).
+///   - The value is a path to the folder containing the libraries of that
+///     package.
+///
+/// The packages entry is optional.  If this is not specified, the test will
+/// still have a default set of packages, like package:expect and package:meta.
+/// If the packages entry is specified, it will be extended with the definitions
+/// of the default set of packages as well. Thus, the list of packages provided
+/// is expected to be disjoint with those in the default set. The default set is
+/// defined directly in the code of `loader.dart`.
+///
 /// The logic in this library mostly treats these names as strings, separately
-/// `loader.dart` is responsible for validating and attaching this dependency
-/// information to a set of module definitions.
+/// `loader.dart` is responsible for validating, attaching dependency
+/// information to a set of module definitions, and resolving package paths.
 ///
 /// The framework is agnostic of what the flags are, but at this time we only
 /// use the name of experimental language features. These are then used to
@@ -76,7 +98,18 @@ TestSpecification parseTestSpecification(String contents) {
     _invalidSpecification(
         "flags: '$flags' expected to be string or list of strings");
   }
-  return new TestSpecification(normalizedFlags, normalizedMap);
+
+  Map<String, String> normalizedPackages = {};
+  final packages = spec['packages'];
+  if (packages != null) {
+    if (packages is Map) {
+      normalizedPackages.addAll(packages.cast<String, String>());
+    } else {
+      _invalidSpecification("packages is not a map");
+    }
+  }
+  return new TestSpecification(
+      normalizedFlags, normalizedMap, normalizedPackages);
 }
 
 /// Data specifying details about a modular test including dependencies and
@@ -96,7 +129,13 @@ class TestSpecification {
   /// (for instance, the module of `package:expect` or the sdk itself).
   final Map<String, List<String>> dependencies;
 
-  TestSpecification(this.flags, this.dependencies);
+  /// Map of package name to a relative path.
+  ///
+  /// The paths in this map are meant to be resolved relative to the location
+  /// where this test specification was defined.
+  final Map<String, String> packages;
+
+  TestSpecification(this.flags, this.dependencies, this.packages);
 }
 
 _invalidSpecification(String message) {

pkg/modular_test/test/loader/dag_with_packages/.packages

@@ -2,3 +2,8 @@ dependencies:
   a: [b, c]
   b: d
   main: [a, b]
+packages:
+  a: a/
+  b: b/
+  d: d/
+  c: c/

pkg/modular_test/test/loader/dag_with_packages/modules.yaml

@@ -1,3 +1,3 @@
 # This expectation file is generated by loader_test.dart
 
-Invalid test: .packages file defines an conflicting entry for package 'expect'.
+Invalid test: .packages file defines a conflicting entry for package 'expect'.

pkg/modular_test/test/loader/invalid_packages_error/.packages

@@ -1,3 +1,5 @@
 dependencies:
   main:
     - expect
+packages:
+  expect: .

pkg/modular_test/test/loader/invalid_packages_error/expectation.txt

@@ -2,3 +2,5 @@ dependencies:
   main:
     - js
     - expect
+packages:
+  js: ../../../../js/lib

pkg/modular_test/test/loader/invalid_packages_error/modules.yaml

@@ -6,3 +6,5 @@
 dependencies:
   main: log
   log: js
+packages:
+  js: ../../../pkg/js/lib

pkg/modular_test/test/loader/valid_packages/.packages

@@ -11,3 +11,8 @@ dependencies:
   f3: a
   a: f1
   f1: f0
+
+packages:
+  f0: .
+  f1: f1
+  a: a

pkg/modular_test/test/loader/valid_packages/modules.yaml

@@ -7,3 +7,5 @@
 dependencies:
   main: static_interop
   static_interop: js
+packages:
+  js: ../../../pkg/js/lib