From d2d810a8e8a87a584bfcfa90f63d484e895847bf Mon Sep 17 00:00:00 2001 From: Sigmund Cherem Date: Sat, 9 Apr 2022 00:35:13 +0000 Subject: [PATCH] [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 Commit-Queue: Sigmund Cherem --- pkg/modular_test/lib/src/loader.dart | 84 ++++++------------- .../lib/src/test_specification_parser.dart | 51 +++++++++-- .../test/loader/dag_with_packages/.packages | 4 - .../loader/dag_with_packages/modules.yaml | 5 ++ .../loader/invalid_packages_error/.packages | 1 - .../invalid_packages_error/expectation.txt | 2 +- .../invalid_packages_error/modules.yaml | 2 + .../test/loader/valid_packages/.packages | 1 - .../test/loader/valid_packages/modules.yaml | 2 + tests/modular/js_interop/.packages | 1 - tests/modular/js_interop/modules.yaml | 2 + tests/modular/package_imports/.packages | 6 -- tests/modular/package_imports/modules.yaml | 5 ++ .../modular/static_interop_erasure/.packages | 1 - .../static_interop_erasure/modules.yaml | 2 + 15 files changed, 88 insertions(+), 81 deletions(-) delete mode 100644 pkg/modular_test/test/loader/dag_with_packages/.packages delete mode 100644 pkg/modular_test/test/loader/invalid_packages_error/.packages delete mode 100644 pkg/modular_test/test/loader/valid_packages/.packages delete mode 100644 tests/modular/js_interop/.packages delete mode 100644 tests/modular/package_imports/.packages delete mode 100644 tests/modular/static_interop_erasure/.packages diff --git a/pkg/modular_test/lib/src/loader.dart b/pkg/modular_test/lib/src/loader.dart index 825e6796747..b67f011aeae 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 @@ Future 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 defaultPackages = - _parseDotPackagesBytesToLibMap(_defaultPackagesInput, root); + final defaultTestSpecification = parseTestSpecification(_defaultPackagesSpec); + Set defaultPackages = defaultTestSpecification.packages.keys.toSet(); Module sdkModule = await _createSdkModule(root); Map modules = {'sdk': sdkModule}; String specString; Module mainModule; - Map packages = {}; var entries = folder.listSync(recursive: false).toList() // Sort to avoid dependency on file system order. ..sort(_compareFileSystemEntity); @@ -59,7 +48,7 @@ Future 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 loadTest(Uri uri) async { packageBase: Uri.parse('.')); if (isMain) mainModule = module; modules[moduleName] = module; - } else if (fileName == '.packages') { - List packagesBytes = await entry.readAsBytes(); - packages = _parseDotPackagesBytesToLibMap(packagesBytes, entryUri); } else if (fileName == 'modules.yaml') { specString = await entry.readAsString(); } @@ -90,7 +76,7 @@ Future 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 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 modules, Module sdkModule) { } } -void _addDefaultPackageEntries( - Map packages, Map 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 _addModulePerPackage( - Map packages, Map modules) async { +Future _addModulePerPackage(Map packages, Uri configRoot, + Map 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 modules, Module main) { toRemove.forEach(modules.remove); } -/// Default entries for a .packages file with paths relative to the SDK root. -List _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 _parseDotPackagesBytesToLibMap(Uint8List bytes, Uri baseUri) { - var map = {}; - 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 d0eed3db0e7..85272b026ef 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 @@ TestSpecification parseTestSpecification(String contents) { _invalidSpecification( "flags: '$flags' expected to be string or list of strings"); } - return new TestSpecification(normalizedFlags, normalizedMap); + + Map normalizedPackages = {}; + final packages = spec['packages']; + if (packages != null) { + if (packages is Map) { + normalizedPackages.addAll(packages.cast()); + } 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> 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 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 194aba97e77..00000000000 --- 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 0a775d22285..fe1a957d8a8 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 @@ dependencies: 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 d8892cee636..00000000000 --- 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 6931c77ea7e..b4e0bcf68a1 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 a5864d392d7..8daf1249355 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 d8e718b3529..00000000000 --- 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 2ad9483d37a..9a777a146da 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 @@ dependencies: 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 fce126f821d..00000000000 --- 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 021eddc698c..79290b9d511 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 6c6f5a47c01..00000000000 --- 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 a8731e9a4f6..e91a1347110 100644 --- a/tests/modular/package_imports/modules.yaml +++ b/tests/modular/package_imports/modules.yaml @@ -11,3 +11,8 @@ dependencies: 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 fce126f821d..00000000000 --- 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 6d1a98874a2..e356ba971e5 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