[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 https://github.com/dart-lang/sdk/issues/48275
Change-Id: I22ef02b2b2327a0c798f2fea73d59c758a8bb0bb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240651
Reviewed-by: Joshua Litt <joshualitt@google.com>
Commit-Queue: Sigmund Cherem <sigmund@google.com>
diff --git a/pkg/modular_test/lib/src/loader.dart b/pkg/modular_test/lib/src/loader.dart
index 825e679..b67f011 100644
--- a/pkg/modular_test/lib/src/loader.dart
+++ b/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 @@
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 @@
"'$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 @@
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 @@
"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 @@
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 _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 @@
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 @@
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 @@
}
}
}
-
-/// 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;
-}
diff --git a/pkg/modular_test/lib/src/test_specification_parser.dart b/pkg/modular_test/lib/src/test_specification_parser.dart
index d0eed3d..85272b0 100644
--- a/pkg/modular_test/lib/src/test_specification_parser.dart
+++ b/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 @@
_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 @@
/// (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) {
diff --git a/pkg/modular_test/test/loader/dag_with_packages/.packages b/pkg/modular_test/test/loader/dag_with_packages/.packages
deleted file mode 100644
index 194aba9..0000000
--- a/pkg/modular_test/test/loader/dag_with_packages/.packages
+++ /dev/null
@@ -1,4 +0,0 @@
-a:a/
-b:b/
-d:d/
-c:c/
diff --git a/pkg/modular_test/test/loader/dag_with_packages/modules.yaml b/pkg/modular_test/test/loader/dag_with_packages/modules.yaml
index 0a775d2..fe1a957 100644
--- a/pkg/modular_test/test/loader/dag_with_packages/modules.yaml
+++ b/pkg/modular_test/test/loader/dag_with_packages/modules.yaml
@@ -2,3 +2,8 @@
a: [b, c]
b: d
main: [a, b]
+packages:
+ a: a/
+ b: b/
+ d: d/
+ c: c/
diff --git a/pkg/modular_test/test/loader/invalid_packages_error/.packages b/pkg/modular_test/test/loader/invalid_packages_error/.packages
deleted file mode 100644
index d8892ce..0000000
--- a/pkg/modular_test/test/loader/invalid_packages_error/.packages
+++ /dev/null
@@ -1 +0,0 @@
-expect:.
diff --git a/pkg/modular_test/test/loader/invalid_packages_error/expectation.txt b/pkg/modular_test/test/loader/invalid_packages_error/expectation.txt
index 6931c77..b4e0bcf 100644
--- a/pkg/modular_test/test/loader/invalid_packages_error/expectation.txt
+++ b/pkg/modular_test/test/loader/invalid_packages_error/expectation.txt
@@ -1,3 +1,3 @@
# This expectation file is generated by loader_test.dart
-Invalid test: .packages file defines an conflicting entry for package 'expect'.
\ No newline at end of file
+Invalid test: .packages file defines a conflicting entry for package 'expect'.
\ No newline at end of file
diff --git a/pkg/modular_test/test/loader/invalid_packages_error/modules.yaml b/pkg/modular_test/test/loader/invalid_packages_error/modules.yaml
index a5864d3..8daf1249 100644
--- a/pkg/modular_test/test/loader/invalid_packages_error/modules.yaml
+++ b/pkg/modular_test/test/loader/invalid_packages_error/modules.yaml
@@ -1,3 +1,5 @@
dependencies:
main:
- expect
+packages:
+ expect: .
diff --git a/pkg/modular_test/test/loader/valid_packages/.packages b/pkg/modular_test/test/loader/valid_packages/.packages
deleted file mode 100644
index d8e718b..0000000
--- a/pkg/modular_test/test/loader/valid_packages/.packages
+++ /dev/null
@@ -1 +0,0 @@
-js:../../../../js/lib
diff --git a/pkg/modular_test/test/loader/valid_packages/modules.yaml b/pkg/modular_test/test/loader/valid_packages/modules.yaml
index 2ad9483..9a777a1 100644
--- a/pkg/modular_test/test/loader/valid_packages/modules.yaml
+++ b/pkg/modular_test/test/loader/valid_packages/modules.yaml
@@ -2,3 +2,5 @@
main:
- js
- expect
+packages:
+ js: ../../../../js/lib
diff --git a/tests/modular/js_interop/.packages b/tests/modular/js_interop/.packages
deleted file mode 100644
index fce126f..0000000
--- a/tests/modular/js_interop/.packages
+++ /dev/null
@@ -1 +0,0 @@
-js:../../../pkg/js/lib
diff --git a/tests/modular/js_interop/modules.yaml b/tests/modular/js_interop/modules.yaml
index 021eddc..79290b9 100644
--- a/tests/modular/js_interop/modules.yaml
+++ b/tests/modular/js_interop/modules.yaml
@@ -6,3 +6,5 @@
dependencies:
main: log
log: js
+packages:
+ js: ../../../pkg/js/lib
diff --git a/tests/modular/package_imports/.packages b/tests/modular/package_imports/.packages
deleted file mode 100644
index 6c6f5a4..0000000
--- a/tests/modular/package_imports/.packages
+++ /dev/null
@@ -1,6 +0,0 @@
-# Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
-# for details. All rights reserved. Use of this source code is governed by a
-# BSD-style license that can be found in the LICENSE file.
-f0:.
-f1:f1
-a:a
diff --git a/tests/modular/package_imports/modules.yaml b/tests/modular/package_imports/modules.yaml
index a8731e9..e91a134 100644
--- a/tests/modular/package_imports/modules.yaml
+++ b/tests/modular/package_imports/modules.yaml
@@ -11,3 +11,8 @@
f3: a
a: f1
f1: f0
+
+packages:
+ f0: .
+ f1: f1
+ a: a
diff --git a/tests/modular/static_interop_erasure/.packages b/tests/modular/static_interop_erasure/.packages
deleted file mode 100644
index fce126f..0000000
--- a/tests/modular/static_interop_erasure/.packages
+++ /dev/null
@@ -1 +0,0 @@
-js:../../../pkg/js/lib
diff --git a/tests/modular/static_interop_erasure/modules.yaml b/tests/modular/static_interop_erasure/modules.yaml
index 6d1a988..e356ba9 100644
--- a/tests/modular/static_interop_erasure/modules.yaml
+++ b/tests/modular/static_interop_erasure/modules.yaml
@@ -7,3 +7,5 @@
dependencies:
main: static_interop
static_interop: js
+packages:
+ js: ../../../pkg/js/lib
