From edadc9c88149b52bb090dd2cc12de4e3fafb7640 Mon Sep 17 00:00:00 2001 From: Sigmund Cherem Date: Fri, 24 May 2019 01:27:13 +0000 Subject: [PATCH] Add support for caching results of shared modules. This is important as we will soon add support for compiling the sdk as a module and we would like to only compile it once when running a suite of tests. + also enable caching in the dart2js pipeline test. Change-Id: Ic9043f868123164f3ab425ba73f7428416b05fc0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103485 Commit-Queue: Sigmund Cherem Reviewed-by: Johnni Winther --- pkg/modular_test/lib/src/io_pipeline.dart | 130 +++++++++++++----- pkg/modular_test/lib/src/loader.dart | 21 +-- pkg/modular_test/lib/src/memory_pipeline.dart | 44 +++++- pkg/modular_test/lib/src/pipeline.dart | 9 +- pkg/modular_test/lib/src/suite.dart | 66 ++++++++- ...er.dart => test_specification_parser.dart} | 52 +++++-- pkg/modular_test/test/io_pipeline_test.dart | 34 +++-- .../test/loader/dag/expectation.txt | 5 + .../test/loader/dag_with_packages/.packages | 2 + .../loader/dag_with_packages/expectation.txt | 9 +- .../expectation.txt | 3 + pkg/modular_test/test/loader/loader_test.dart | 42 +++--- .../loader/no_dependencies/expectation.txt | 1 + .../test/loader/tree/expectation.txt | 5 + .../loader/valid_packages/expectation.txt | 4 + .../test/memory_pipeline_test.dart | 21 ++- pkg/modular_test/test/pipeline_common.dart | 112 ++++++++++++++- ...st.dart => specification_parser_test.dart} | 34 +++-- ..._test.dart => validate_pipeline_test.dart} | 4 +- .../test/validate_suite_test.dart | 61 ++++++++ pkg/pkg.status | 2 +- .../dart2js/modular/modular_test.dart | 40 ++++-- 22 files changed, 578 insertions(+), 123 deletions(-) rename pkg/modular_test/lib/src/{dependency_parser.dart => test_specification_parser.dart} (55%) rename pkg/modular_test/test/{dependency_parser_test.dart => specification_parser_test.dart} (63%) rename pkg/modular_test/test/{validate_test.dart => validate_pipeline_test.dart} (95%) create mode 100644 pkg/modular_test/test/validate_suite_test.dart diff --git a/pkg/modular_test/lib/src/io_pipeline.dart b/pkg/modular_test/lib/src/io_pipeline.dart index ed77f2d8383..910eb3dfaeb 100644 --- a/pkg/modular_test/lib/src/io_pipeline.dart +++ b/pkg/modular_test/lib/src/io_pipeline.dart @@ -26,49 +26,103 @@ abstract class IOModularStep extends ModularStep { } class IOPipeline extends Pipeline { - /// A folder per step. The key is the data id produced by a specific step. + /// Folder that holds the results of each step during the run of the pipeline. /// - /// This contains internal state used during the run of the pipeline, but is - /// expected to be null before and after the pipeline is executed. - Map _tmpFolders; - Map get tmpFoldersForTesting => _tmpFolders; - bool saveFoldersForTesting; + /// This value is usually null before and after the pipeline runs, but will be + /// non-null in two cases: + /// + /// * for testing purposes when using [saveIntermediateResultsForTesting]. + /// + /// * to share results across pipeline runs when using [cacheSharedModules]. + /// + /// When using [cacheSharedModules] the pipeline will only reuse data for + /// modules that are known to be shared (e.g. shared packages and sdk + /// libraries), and not modules that are test specific. File names will be + /// specific enough so that we can keep separate the artifacts created from + /// running tools under different configurations (with different flags). + Uri _resultsFolderUri; + Uri get resultFolderUriForTesting => _resultsFolderUri; - IOPipeline(List steps, {this.saveFoldersForTesting: false}) - : super(steps); + /// A unique number to denote the current modular test configuration. + /// + /// When using [cacheSharedModules], a test can resuse the output of a + /// previous run of this pipeline if that output was generated with the same + /// configuration. + int _currentConfiguration; + + final ConfigurationRegistry _registry; + + /// Whether to keep alive the temporary folder used to store intermediate + /// results in order to inspect it later in test. + final bool saveIntermediateResultsForTesting; + + IOPipeline(List steps, + {this.saveIntermediateResultsForTesting: false, + bool cacheSharedModules: false}) + : _registry = cacheSharedModules ? new ConfigurationRegistry() : null, + super(steps, cacheSharedModules); @override Future run(ModularTest test) async { - assert(_tmpFolders == null); - _tmpFolders = {}; + var resultsDir = null; + if (_resultsFolderUri == null) { + resultsDir = await Directory.systemTemp.createTemp('modular_test_res-'); + _resultsFolderUri = resultsDir.uri; + } + if (cacheSharedModules) { + _currentConfiguration = _registry.computeConfigurationId(test); + } await super.run(test); - if (!saveFoldersForTesting) { - for (var folder in _tmpFolders.values) { - await Directory.fromUri(folder).delete(recursive: true); - } - _tmpFolders = null; + if (resultsDir != null && + !saveIntermediateResultsForTesting && + !cacheSharedModules) { + await resultsDir.delete(recursive: true); + _resultsFolderUri = null; + } + if (!saveIntermediateResultsForTesting) { + _currentConfiguration = null; + } + } + + /// Delete folders that were kept around either because of + /// [saveIntermediateResultsForTesting] or because of [cacheSharedModules]. + Future cleanup() async { + if (saveIntermediateResultsForTesting || cacheSharedModules) { + await Directory.fromUri(_resultsFolderUri).delete(recursive: true); } } @override Future runStep(IOModularStep step, Module module, Map> visibleData) async { - // Since data ids are unique throughout the pipeline, we use the first - // result data id as a hint for the name of the temporary folder of a step. - var stepFolder; - for (var dataId in step.resultData) { - stepFolder ??= - await Directory.systemTemp.createTemp('modular_test_${dataId}-'); - _tmpFolders[dataId] ??= - (await Directory.systemTemp.createTemp('modular_test_${dataId}_res-')) - .uri; + if (cacheSharedModules && module.isShared) { + // If all expected outputs are already available, skip the step. + bool allCachedResultsFound = true; + for (var dataId in step.resultData) { + var cachedFile = File.fromUri(_resultsFolderUri + .resolve(_toFileName(module, dataId, configSpecific: true))); + if (!await cachedFile.exists()) { + allCachedResultsFound = false; + break; + } + } + if (allCachedResultsFound) { + step.notifyCached(module); + return; + } } + + // Each step is executed in a separate folder. To make it easier to debug + // issues, we include one of the step data ids in the name of the folder. + var stepId = step.resultData.first; + var stepFolder = + await Directory.systemTemp.createTemp('modular_test_${stepId}-'); for (var module in visibleData.keys) { for (var dataId in visibleData[module]) { - var filename = "${module.name}.${dataId.name}"; - var assetUri = _tmpFolders[dataId].resolve(filename); - await File.fromUri(assetUri) - .copy(stepFolder.uri.resolve(filename).toFilePath()); + var assetUri = _resultsFolderUri + .resolve(_toFileName(module, dataId, configSpecific: true)); + await File.fromUri(assetUri).copy( + stepFolder.uri.resolve(_toFileName(module, dataId)).toFilePath()); } } if (step.needsSources) { @@ -81,19 +135,31 @@ class IOPipeline extends Pipeline { } await step.execute(module, stepFolder.uri, - (Module m, DataId id) => Uri.parse("${m.name}.${id.name}")); + (Module m, DataId id) => Uri.parse(_toFileName(m, id))); for (var dataId in step.resultData) { var outputFile = - File.fromUri(stepFolder.uri.resolve("${module.name}.${dataId.name}")); + File.fromUri(stepFolder.uri.resolve(_toFileName(module, dataId))); if (!await outputFile.exists()) { throw StateError( "Step '${step.runtimeType}' didn't produce an output file"); } - await outputFile.copy(_tmpFolders[dataId] - .resolve("${module.name}.${dataId.name}") + await outputFile.copy(_resultsFolderUri + .resolve(_toFileName(module, dataId, configSpecific: true)) .toFilePath()); } await stepFolder.delete(recursive: true); } + + String _toFileName(Module module, DataId dataId, + {bool configSpecific: false}) { + var prefix = + cacheSharedModules && configSpecific && _currentConfiguration != null + ? _currentConfiguration + : ''; + return "$prefix${module.name}.${dataId.name}"; + } + + String configSpecificResultFileNameForTesting(Module module, DataId dataId) => + _toFileName(module, dataId, configSpecific: true); } diff --git a/pkg/modular_test/lib/src/loader.dart b/pkg/modular_test/lib/src/loader.dart index ce00e49f127..61b8969f2d4 100644 --- a/pkg/modular_test/lib/src/loader.dart +++ b/pkg/modular_test/lib/src/loader.dart @@ -17,11 +17,11 @@ /// [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 `dependencies_parser.dart`. +/// The format is described in `test_specification_parser.dart`. import 'dart:io'; import 'dart:convert'; import 'suite.dart'; -import 'dependency_parser.dart'; +import 'test_specification_parser.dart'; import 'find_sdk_root.dart'; import 'package:package_config/packages_file.dart' as package_config; @@ -40,7 +40,7 @@ Future loadTest(Uri uri) async { Map defaultPackages = package_config.parse(_defaultPackagesInput, root); Map modules = {}; - String spec; + String specString; Module mainModule; Map packages = {}; await for (var entry in folder.list(recursive: false)) { @@ -69,7 +69,7 @@ Future loadTest(Uri uri) async { List packagesBytes = await entry.readAsBytes(); packages = package_config.parse(packagesBytes, entryUri); } else if (fileName == 'modules.yaml') { - spec = await entry.readAsString(); + specString = await entry.readAsString(); } } else { assert(entry is Directory); @@ -88,7 +88,7 @@ Future loadTest(Uri uri) async { packageBase: Uri.parse('$moduleName/')); } } - if (spec == null) { + if (specString == null) { return _invalidTest("modules.yaml file is missing"); } if (mainModule == null) { @@ -97,12 +97,15 @@ Future loadTest(Uri uri) async { _addDefaultPackageEntries(packages, defaultPackages); await _addModulePerPackage(packages, modules); - _attachDependencies(parseDependencyMap(spec), modules); - _attachDependencies(parseDependencyMap(_defaultPackagesSpec), modules); + TestSpecification spec = parseTestSpecification(specString); + _attachDependencies(spec.dependencies, modules); + _attachDependencies( + parseTestSpecification(_defaultPackagesSpec).dependencies, modules); _detectCyclesAndRemoveUnreachable(modules, mainModule); var sortedModules = modules.values.toList() ..sort((a, b) => a.name.compareTo(b.name)); - return new ModularTest(sortedModules, mainModule); + var sortedFlags = spec.flags.toList()..sort(); + return new ModularTest(sortedModules, mainModule, sortedFlags); } /// Returns all source files recursively found in a folder as relative URIs. @@ -169,7 +172,7 @@ Future _addModulePerPackage( // module that is part of the test (package name and module name should // match). modules[packageName] = Module(packageName, [], rootUri, sources, - isPackage: true, packageBase: Uri.parse('lib/')); + isPackage: true, packageBase: Uri.parse('lib/'), isShared: true); } } } diff --git a/pkg/modular_test/lib/src/memory_pipeline.dart b/pkg/modular_test/lib/src/memory_pipeline.dart index 9ad5c43f452..a107af4cd7f 100644 --- a/pkg/modular_test/lib/src/memory_pipeline.dart +++ b/pkg/modular_test/lib/src/memory_pipeline.dart @@ -27,20 +27,60 @@ class MemoryPipeline extends Pipeline { /// A copy of [_result] at the time the pipeline last finished running. Map> resultsForTesting; - MemoryPipeline(this._sources, List steps) : super(steps); + final ConfigurationRegistry _registry; + + /// Cache of results when [cacheSharedModules] is true + final List>> _resultCache; + + MemoryPipeline(this._sources, List steps, + {bool cacheSharedModules: false}) + : _registry = cacheSharedModules ? new ConfigurationRegistry() : null, + _resultCache = cacheSharedModules ? [] : null, + super(steps, cacheSharedModules); @override Future run(ModularTest test) async { - assert(_results == null); _results = {}; + Map> cache = null; + if (cacheSharedModules) { + int id = _registry.computeConfigurationId(test); + if (id < _resultCache.length) { + cache = _resultCache[id]; + } else { + assert(id == _resultCache.length); + _resultCache.add(cache = {}); + } + _results.addAll(cache); + } await super.run(test); resultsForTesting = _results; + if (cacheSharedModules) { + for (var module in _results.keys) { + if (module.isShared) { + cache[module] = _results[module]; + } + } + } _results = null; } @override Future runStep(MemoryModularStep step, Module module, Map> visibleData) async { + if (cacheSharedModules && module.isShared) { + bool allCachedResultsFound = true; + for (var dataId in step.resultData) { + if (_results[module] == null || _results[module][dataId] == null) { + allCachedResultsFound = false; + break; + } + } + if (allCachedResultsFound) { + step.notifyCached(module); + return; + } + } + Map> inputData = {}; visibleData.forEach((module, dataIdSet) { inputData[module] = {}; diff --git a/pkg/modular_test/lib/src/pipeline.dart b/pkg/modular_test/lib/src/pipeline.dart index 64e08a7d98c..029e5b25c52 100644 --- a/pkg/modular_test/lib/src/pipeline.dart +++ b/pkg/modular_test/lib/src/pipeline.dart @@ -48,6 +48,9 @@ class ModularStep { this.moduleDataNeeded: const [], this.resultData, this.onlyOnMain: false}); + + /// Notifies that the step was not executed, but cached instead. + void notifyCached(Module module) {} } /// An object to uniquely identify modular data produced by a modular step. @@ -63,9 +66,13 @@ class DataId { } abstract class Pipeline { + /// Whether to cache the result of shared modules (e.g. shard packages and sdk + /// libraries) when multiple tests are run by this pipeline. + final bool cacheSharedModules; + final List steps; - Pipeline(this.steps) { + Pipeline(this.steps, this.cacheSharedModules) { _validate(); } diff --git a/pkg/modular_test/lib/src/suite.dart b/pkg/modular_test/lib/src/suite.dart index 506349b192e..88aca4c8d14 100644 --- a/pkg/modular_test/lib/src/suite.dart +++ b/pkg/modular_test/lib/src/suite.dart @@ -13,8 +13,23 @@ class ModularTest { /// The module containing the main entry method. final Module mainModule; - ModularTest(this.modules, this.mainModule) - : assert(mainModule != null && modules.length > 0); + /// Flags provided to tools that compile and execute the test. + final List flags; + + ModularTest(this.modules, this.mainModule, this.flags) { + if (mainModule == null) { + throw ArgumentError("main module was null"); + } + if (flags == null) { + throw ArgumentError("flags was null"); + } + if (modules == null || modules.length == 0) { + throw ArgumentError("modules cannot be null or empty"); + } + for (var module in modules) { + module._validateDependencies(); + } + } String debugString() => modules.map((m) => m.debugString()).join('\n'); } @@ -50,16 +65,36 @@ class Module { /// Whether this is the main entry module of a test. bool isMain; + /// Whether this module is test specific or shared across tests. Usually this + /// will be true only for the SDK and shared packages like `package:expect`. + bool isShared; + Module(this.name, this.dependencies, this.rootUri, this.sources, {this.mainSource, this.isPackage: false, this.isMain: false, - this.packageBase}) { + this.packageBase, + this.isShared: false}) { if (!_validModuleName.hasMatch(name)) { throw ArgumentError("invalid module name: $name"); } } + void _validateDependencies() { + if (!isPackage && !isShared) return; + for (var dependency in dependencies) { + if (isPackage && !dependency.isPackage) { + throw InvalidModularTestError("invalid dependency: $name is a package " + "but it depends on ${dependency.name}, which is not."); + } + if (isShared && !dependency.isShared) { + throw InvalidModularTestError( + "invalid dependency: $name is a shared module " + "but it depends on ${dependency.name}, which is not."); + } + } + } + @override String toString() => '[module $name]'; @@ -87,3 +122,28 @@ Set computeTransitiveDependencies(Module module) { module.dependencies.forEach(helper); return deps; } + +/// A registry that can map a test configuration to a simple id. +/// +/// This is used to help determine whether two tests are run with the same set +/// of flags (the same configuration), and thus pipelines could reuse the +/// results of shared modules from the first test when running the second test. +class ConfigurationRegistry { + Map _configurationId = {}; + + /// Compute an id to identify the configuration of a modular test. + /// + /// A configuration is defined in terms of the set of flags provided to a + /// test. If two test provided to this registry share the same set of flags, + /// the resulting ids are the same. Similarly, if the flags are different, + /// their ids will be different as well. + int computeConfigurationId(ModularTest test) { + return _configurationId[test.flags.join(' ')] ??= _configurationId.length; + } +} + +class InvalidModularTestError extends Error { + final String message; + InvalidModularTestError(this.message); + String toString() => "Invalid modular test: $message"; +} diff --git a/pkg/modular_test/lib/src/dependency_parser.dart b/pkg/modular_test/lib/src/test_specification_parser.dart similarity index 55% rename from pkg/modular_test/lib/src/dependency_parser.dart rename to pkg/modular_test/lib/src/test_specification_parser.dart index 51f2460120e..92f81eceeb4 100644 --- a/pkg/modular_test/lib/src/dependency_parser.dart +++ b/pkg/modular_test/lib/src/test_specification_parser.dart @@ -2,14 +2,19 @@ // 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. -/// This library defines how to read module dependencies from a Yaml -/// specification. We expect to find specifications written in this format: +/// This library defines how to read a test specification from a Yaml +/// file. We expect specifications written in this format: /// /// dependencies: /// b: a /// main: [b, expect] +/// flags: +/// - "--enable-experiment=constant-update-2018" /// -/// Where: +/// 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. +/// +/// When defining dependencies: /// - Each name corresponds to a module. /// - Module names correlate to either a file, a folder, or a package. /// - A map entry contains all the dependencies of a module, if any. @@ -22,12 +27,8 @@ import 'package:yaml/yaml.dart'; /// Parses [contents] containing a module dependencies specification written in -/// yaml, and returns a normalized dependency map. -/// -/// Note: some values in the map may not have a corresponding key. That may be -/// the case for modules that have no dependencies and modules that are not -/// specified in [contents] (e.g. modules that are supported by default). -Map> parseDependencyMap(String contents) { +/// yaml, and returns a [TestSpecification]. +TestSpecification parseTestSpecification(String contents) { var spec = loadYaml(contents); if (spec is! YamlMap) { return _invalidSpecification("spec is not a map"); @@ -60,7 +61,38 @@ Map> parseDependencyMap(String contents) { "entry: '$value' is not a string or a list of strings"); } }); - return normalizedMap; + + List normalizedFlags = []; + dynamic flags = spec['flags']; + if (flags is String) { + normalizedFlags.add(flags); + } else if (flags is List) { + normalizedFlags.addAll(flags.cast()); + } else if (flags != null) { + _invalidSpecification( + "flags: '$flags' expected to be string or list of strings"); + } + return new TestSpecification(normalizedFlags, normalizedMap); +} + +/// Data specifying details about a modular test including dependencies and +/// flags that are necessary in order to properly run a test. +/// +class TestSpecification { + /// Set of flags necessary to properly run a test. + /// + /// Usually this contains flags enabling language experiments. + final List flags; + + /// Dependencies of the modules that are expected to exist on the test. + /// + /// Note: some values in the map may not have a corresponding key. That may be + /// the case for modules that have no dependencies and modules that are not + /// specified explicitly because they are added automatically by the framework + /// (for instance, the module of `package:expect` or the sdk itself). + final Map> dependencies; + + TestSpecification(this.flags, this.dependencies); } _invalidSpecification(String message) { diff --git a/pkg/modular_test/test/io_pipeline_test.dart b/pkg/modular_test/test/io_pipeline_test.dart index dd3d01faeef..0ae59ae2a37 100644 --- a/pkg/modular_test/test/io_pipeline_test.dart +++ b/pkg/modular_test/test/io_pipeline_test.dart @@ -29,14 +29,17 @@ class IOPipelineTestStrategy implements PipelineTestStrategy { @override Future> createPipeline( - Map sources, List steps) async { + Map sources, List steps, + {bool cacheSharedModules: false}) async { await Directory.fromUri(testRootUri).create(); for (var uri in sources.keys) { var file = new File.fromUri(uri); await file.create(recursive: true); await file.writeAsStringSync(sources[uri]); } - return new IOPipeline(steps, saveFoldersForTesting: true); + return new IOPipeline(steps, + saveIntermediateResultsForTesting: true, + cacheSharedModules: cacheSharedModules); } @override @@ -83,17 +86,15 @@ class IOPipelineTestStrategy implements PipelineTestStrategy { @override String getResult(covariant IOPipeline pipeline, Module m, DataId dataId) { - var folderUri = pipeline.tmpFoldersForTesting[dataId]; - var file = File.fromUri(folderUri.resolve("${m.name}.${dataId.name}")); + var folderUri = pipeline.resultFolderUriForTesting; + var file = File.fromUri(folderUri + .resolve(pipeline.configSpecificResultFileNameForTesting(m, dataId))); return file.existsSync() ? file.readAsStringSync() : null; } @override - Future cleanup(Pipeline pipeline) async { - var folders = (pipeline as IOPipeline).tmpFoldersForTesting.values; - for (var folder in folders) { - await Directory.fromUri(folder).delete(recursive: true); - } + Future cleanup(covariant IOPipeline pipeline) async { + pipeline.cleanup(); await Directory.fromUri(testRootUri).delete(recursive: true); } } @@ -122,6 +123,9 @@ class SourceOnlyStep implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, resultId))) .writeAsString(action(sources)); } + + @override + void notifyCached(Module module) {} } class ModuleDataStep implements IOModularStep { @@ -146,6 +150,9 @@ class ModuleDataStep implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, resultId))) .writeAsString(result); } + + @override + void notifyCached(Module module) {} } class TwoOutputStep implements IOModularStep { @@ -176,6 +183,9 @@ class TwoOutputStep implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, result2Id))) .writeAsString(result2); } + + @override + void notifyCached(Module module) {} } class LinkStep implements IOModularStep { @@ -205,6 +215,9 @@ class LinkStep implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, resultId))) .writeAsString(action(inputData, depsData)); } + + @override + void notifyCached(Module module) {} } class MainOnlyStep implements IOModularStep { @@ -234,6 +247,9 @@ class MainOnlyStep implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, resultId))) .writeAsString(action(inputData, depsData)); } + + @override + void notifyCached(Module module) {} } Future _readHelper(Module module, Uri root, DataId dataId, diff --git a/pkg/modular_test/test/loader/dag/expectation.txt b/pkg/modular_test/test/loader/dag/expectation.txt index 885c41fb91d..188ee70d339 100644 --- a/pkg/modular_test/test/loader/dag/expectation.txt +++ b/pkg/modular_test/test/loader/dag/expectation.txt @@ -2,21 +2,25 @@ a is package? no + is shared? no dependencies: b, c a.dart b is package? no + is shared? no dependencies: d b.dart c is package? no + is shared? no (no dependencies) c.dart d is package? no + is shared? no (no dependencies) d/d.dart d/e.dart @@ -24,5 +28,6 @@ d main **main module** is package? no + is shared? no dependencies: a, b main.dart diff --git a/pkg/modular_test/test/loader/dag_with_packages/.packages b/pkg/modular_test/test/loader/dag_with_packages/.packages index 5f1a72118ac..194aba97e77 100644 --- a/pkg/modular_test/test/loader/dag_with_packages/.packages +++ b/pkg/modular_test/test/loader/dag_with_packages/.packages @@ -1,2 +1,4 @@ a:a/ +b:b/ +d:d/ c:c/ diff --git a/pkg/modular_test/test/loader/dag_with_packages/expectation.txt b/pkg/modular_test/test/loader/dag_with_packages/expectation.txt index df35ba523df..ddb9f1e5986 100644 --- a/pkg/modular_test/test/loader/dag_with_packages/expectation.txt +++ b/pkg/modular_test/test/loader/dag_with_packages/expectation.txt @@ -2,21 +2,25 @@ a is package? yes + is shared? no dependencies: b, c a.dart b - is package? no + is package? yes + is shared? no dependencies: d b.dart c is package? yes + is shared? no (no dependencies) c.dart d - is package? no + is package? yes + is shared? no (no dependencies) d/d.dart d/e.dart @@ -24,5 +28,6 @@ d main **main module** is package? no + is shared? no dependencies: a, b main.dart diff --git a/pkg/modular_test/test/loader/default_package_dependency_error/expectation.txt b/pkg/modular_test/test/loader/default_package_dependency_error/expectation.txt index cf449f37033..29f45c1be9d 100644 --- a/pkg/modular_test/test/loader/default_package_dependency_error/expectation.txt +++ b/pkg/modular_test/test/loader/default_package_dependency_error/expectation.txt @@ -2,6 +2,7 @@ expect is package? yes + is shared? yes dependencies: meta lib/expect.dart lib/matchers_lite.dart @@ -10,11 +11,13 @@ expect main **main module** is package? no + is shared? no dependencies: expect main.dart meta is package? yes + is shared? yes (no dependencies) lib/dart2js.dart lib/meta.dart diff --git a/pkg/modular_test/test/loader/loader_test.dart b/pkg/modular_test/test/loader/loader_test.dart index 4f5f59ce893..6faf7168058 100644 --- a/pkg/modular_test/test/loader/loader_test.dart +++ b/pkg/modular_test/test/loader/loader_test.dart @@ -4,34 +4,27 @@ /// Tests that the logic to load, parse, and validate modular tests. import 'dart:io'; -import 'package:async_helper/async_helper.dart'; -import 'package:expect/expect.dart'; + +import 'package:test/test.dart'; import 'package:modular_test/src/loader.dart'; import 'package:modular_test/src/suite.dart'; import 'package:args/args.dart'; -main(List args) { +main(List args) async { var options = _Options.parse(args); - asyncTest(() async { - var baseUri = Platform.script.resolve('./'); - var baseDir = Directory.fromUri(baseUri); - await for (var entry in baseDir.list(recursive: false)) { - if (entry is Directory) { - await _runTest(entry.uri, baseUri, options); - } + var baseUri = Platform.script.resolve('./'); + var baseDir = Directory.fromUri(baseUri); + await for (var entry in baseDir.list(recursive: false)) { + if (entry is Directory) { + var dirName = entry.uri.path.substring(baseDir.path.length); + test(dirName, () => _runTest(entry.uri, dirName, options), + skip: options.filter != null && !dirName.contains(options.filter)); } - }); + } } -Future _runTest(Uri uri, Uri baseDir, _Options options) async { - var dirName = uri.path.substring(baseDir.path.length); - if (options.filter != null && !dirName.contains(options.filter)) { - if (options.showSkipped) print("skipped: $dirName"); - return; - } - - print("testing: $dirName"); +Future _runTest(Uri uri, String dirName, _Options options) async { String result; String header = "# This expectation file is generated by loader_test.dart\n\n"; @@ -44,7 +37,8 @@ Future _runTest(Uri uri, Uri baseDir, _Options options) async { var file = File.fromUri(uri.resolve('expectation.txt')); if (!options.updateExpectations) { - Expect.isTrue(await file.exists(), "expectation.txt file is missing"); + expect(await file.exists(), isTrue, + reason: "expectation.txt file is missing"); var expectation = await file.readAsString(); if (expectation != result) { print("expectation.txt doesn't match the result of the test. " @@ -52,8 +46,7 @@ Future _runTest(Uri uri, Uri baseDir, _Options options) async { " ${Platform.executable} ${Platform.script} " "--update --show-update --filter $dirName"); } - Expect.equals(expectation, result); - print(" expectation matches result."); + expect(expectation, result); } else if (await file.exists() && (await file.readAsString() == result)) { print(" expectation matches result and was up to date."); } else { @@ -76,10 +69,11 @@ String _dumpAsText(ModularTest test) { } buffer.write(module.name); if (module.isMain) { - Expect.equals(test.mainModule, module); + expect(test.mainModule, module); buffer.write('\n **main module**'); } buffer.write('\n is package? ${module.isPackage ? 'yes' : 'no'}'); + buffer.write('\n is shared? ${module.isShared ? 'yes' : 'no'}'); if (module.dependencies.isEmpty) { buffer.write('\n (no dependencies)'); } else { @@ -103,7 +97,6 @@ String _dumpAsText(ModularTest test) { class _Options { bool updateExpectations = false; bool showResultOnUpdate = false; - bool showSkipped = false; String filter = null; static _Options parse(List args) { @@ -124,7 +117,6 @@ class _Options { return _Options() ..updateExpectations = argResults['update'] ..showResultOnUpdate = argResults['show-update'] - ..showSkipped = argResults['show-skipped'] ..filter = argResults['filter']; } } diff --git a/pkg/modular_test/test/loader/no_dependencies/expectation.txt b/pkg/modular_test/test/loader/no_dependencies/expectation.txt index f19449be967..21b81651e0b 100644 --- a/pkg/modular_test/test/loader/no_dependencies/expectation.txt +++ b/pkg/modular_test/test/loader/no_dependencies/expectation.txt @@ -3,5 +3,6 @@ main **main module** is package? no + is shared? no (no dependencies) main.dart diff --git a/pkg/modular_test/test/loader/tree/expectation.txt b/pkg/modular_test/test/loader/tree/expectation.txt index f438e2238fa..285b4d78a93 100644 --- a/pkg/modular_test/test/loader/tree/expectation.txt +++ b/pkg/modular_test/test/loader/tree/expectation.txt @@ -2,21 +2,25 @@ a is package? no + is shared? no dependencies: b, c a.dart b is package? no + is shared? no dependencies: d b.dart c is package? no + is shared? no (no dependencies) c.dart d is package? no + is shared? no (no dependencies) d/d.dart d/e.dart @@ -24,5 +28,6 @@ d main **main module** is package? no + is shared? no dependencies: a main.dart diff --git a/pkg/modular_test/test/loader/valid_packages/expectation.txt b/pkg/modular_test/test/loader/valid_packages/expectation.txt index dab41634988..be86423e0f5 100644 --- a/pkg/modular_test/test/loader/valid_packages/expectation.txt +++ b/pkg/modular_test/test/loader/valid_packages/expectation.txt @@ -2,6 +2,7 @@ expect is package? yes + is shared? yes dependencies: meta lib/expect.dart lib/matchers_lite.dart @@ -9,6 +10,7 @@ expect js is package? yes + is shared? yes (no dependencies) lib/js.dart lib/js_util.dart @@ -17,11 +19,13 @@ js main **main module** is package? no + is shared? no dependencies: js, expect main.dart meta is package? yes + is shared? yes (no dependencies) lib/dart2js.dart lib/meta.dart diff --git a/pkg/modular_test/test/memory_pipeline_test.dart b/pkg/modular_test/test/memory_pipeline_test.dart index ba406e124f2..124bda71c62 100644 --- a/pkg/modular_test/test/memory_pipeline_test.dart +++ b/pkg/modular_test/test/memory_pipeline_test.dart @@ -22,8 +22,10 @@ class MemoryPipelineTestStrategy @override FutureOr> createPipeline( - Map sources, List steps) { - return new MemoryPipeline(sources, steps); + Map sources, List steps, + {bool cacheSharedModules: false}) { + return new MemoryPipeline(sources, steps, + cacheSharedModules: cacheSharedModules); } @override @@ -95,6 +97,9 @@ class SourceOnlyStep implements MemoryModularStep { } return Future.value({resultId: action(sources)}); } + + @override + void notifyCached(Module module) {} } class ModuleDataStep implements MemoryModularStep { @@ -117,6 +122,9 @@ class ModuleDataStep implements MemoryModularStep { return Future.value({resultId: "data for $module was null"}); return Future.value({resultId: action(inputData)}); } + + @override + void notifyCached(Module module) {} } class TwoOutputStep implements MemoryModularStep { @@ -145,6 +153,9 @@ class TwoOutputStep implements MemoryModularStep { return Future.value( {result1Id: action1(inputData), result2Id: action2(inputData)}); } + + @override + void notifyCached(Module module) {} } class LinkStep implements MemoryModularStep { @@ -170,6 +181,9 @@ class LinkStep implements MemoryModularStep { var inputData = dataProvider(module, inputId) as String; return Future.value({resultId: action(inputData, depsData)}); } + + @override + void notifyCached(Module module) {} } class MainOnlyStep implements MemoryModularStep { @@ -195,4 +209,7 @@ class MainOnlyStep implements MemoryModularStep { var inputData = dataProvider(module, inputId) as String; return Future.value({resultId: action(inputData, depsData)}); } + + @override + void notifyCached(Module module) {} } diff --git a/pkg/modular_test/test/pipeline_common.dart b/pkg/modular_test/test/pipeline_common.dart index b09e611fd02..7bfbf7794e7 100644 --- a/pkg/modular_test/test/pipeline_common.dart +++ b/pkg/modular_test/test/pipeline_common.dart @@ -28,7 +28,8 @@ abstract class PipelineTestStrategy { /// Creates a pipeline with the given sources and steps. Steps will be created /// by other methods in this strategy to ensure they are compatible with to /// the pipeline created here. - FutureOr> createPipeline(Map sources, List steps); + FutureOr> createPipeline(Map sources, List steps, + {bool cacheSharedModules: false}); /// Create a step that applies [action] on all input files of the module, and /// emits a result with the given [id] @@ -98,15 +99,16 @@ runPipelineTest(PipelineTestStrategy testStrategy) { }; var m1 = Module("a", const [], testStrategy.testRootUri, - [Uri.parse("a1.dart"), Uri.parse("a2.dart")]); + [Uri.parse("a1.dart"), Uri.parse("a2.dart")], + isShared: true); var m2 = Module("b", [m1], testStrategy.testRootUri, [Uri.parse("b/b1.dart"), Uri.parse("b/b2.dart")]); var m3 = Module("c", [m2], testStrategy.testRootUri, [Uri.parse("c.dart")], isMain: true); - var singleModuleInput = ModularTest([m1], m1); - var twoModuleInput = ModularTest([m1, m2], m2); - var threeModuleInput = ModularTest([m1, m2, m3], m3); + var singleModuleInput = ModularTest([m1], m1, []); + var twoModuleInput = ModularTest([m1, m2], m2, []); + var threeModuleInput = ModularTest([m1, m2, m3], m3, []); test('can read source data if requested', () async { var concatStep = @@ -310,6 +312,106 @@ runPipelineTest(PipelineTestStrategy testStrategy) { expect(testStrategy.getResult(pipeline, m3, _joinId), "null\nnull\nc c0\n"); await testStrategy.cleanup(pipeline); }); + + test('no reuse of existing results if not caching', () async { + int i = 1; + const counterId = const DataId("counter"); + const linkId = const DataId("link"); + // This step is not idempotent, we do this purposely to test whether caching + // is taking place. + var counterStep = testStrategy.createSourceOnlyStep( + action: (_) => '${i++}', resultId: counterId); + var linkStep = testStrategy.createLinkStep( + action: (String m, List deps) => "${deps.join(',')},$m", + inputId: counterId, + depId: counterId, + resultId: linkId, + requestDependenciesData: true); + var pipeline = await testStrategy.createPipeline( + sources, [counterStep, linkStep], + cacheSharedModules: false); + await pipeline.run(twoModuleInput); + expect(testStrategy.getResult(pipeline, m1, counterId), "1"); + expect(testStrategy.getResult(pipeline, m2, counterId), "2"); + expect(testStrategy.getResult(pipeline, m2, linkId), "1,2"); + + await pipeline.run(threeModuleInput); + expect(testStrategy.getResult(pipeline, m1, counterId), "3"); + expect(testStrategy.getResult(pipeline, m2, counterId), "4"); + expect(testStrategy.getResult(pipeline, m2, linkId), "3,4"); + expect(testStrategy.getResult(pipeline, m3, counterId), "5"); + expect(testStrategy.getResult(pipeline, m3, linkId), "4,5"); + + await testStrategy.cleanup(pipeline); + }); + + test('caching reuses existing results for the same configuration', () async { + int i = 1; + const counterId = const DataId("counter"); + const linkId = const DataId("link"); + var counterStep = testStrategy.createSourceOnlyStep( + action: (_) => '${i++}', resultId: counterId); + var linkStep = testStrategy.createLinkStep( + action: (String m, List deps) => "${deps.join(',')},$m", + inputId: counterId, + depId: counterId, + resultId: linkId, + requestDependenciesData: true); + var pipeline = await testStrategy.createPipeline( + sources, [counterStep, linkStep], + cacheSharedModules: true); + await pipeline.run(twoModuleInput); + expect(testStrategy.getResult(pipeline, m1, counterId), "1"); + expect(testStrategy.getResult(pipeline, m2, counterId), "2"); + expect(testStrategy.getResult(pipeline, m2, linkId), "1,2"); + + await pipeline.run(threeModuleInput); + expect(testStrategy.getResult(pipeline, m1, counterId), "1"); // cached! + expect(testStrategy.getResult(pipeline, m2, counterId), "3"); + expect(testStrategy.getResult(pipeline, m2, linkId), "1,3"); + expect(testStrategy.getResult(pipeline, m3, counterId), "4"); + expect(testStrategy.getResult(pipeline, m3, linkId), "3,4"); + + await testStrategy.cleanup(pipeline); + }); + + test('no reuse of existing results on different configurations', () async { + int i = 1; + const counterId = const DataId("counter"); + const linkId = const DataId("link"); + // This step is not idempotent, we do this purposely to test whether caching + // is taking place. + var counterStep = testStrategy.createSourceOnlyStep( + action: (_) => '${i++}', resultId: counterId); + var linkStep = testStrategy.createLinkStep( + action: (String m, List deps) => "${deps.join(',')},$m", + inputId: counterId, + depId: counterId, + resultId: linkId, + requestDependenciesData: true); + var pipeline = await testStrategy.createPipeline( + sources, [counterStep, linkStep], + cacheSharedModules: true); + var input1 = ModularTest([m1, m2], m2, []); + var input2 = ModularTest([m1, m2], m2, ['--foo']); + var input3 = ModularTest([m1, m2], m2, ['--foo']); + await pipeline.run(input1); + expect(testStrategy.getResult(pipeline, m1, counterId), "1"); + expect(testStrategy.getResult(pipeline, m2, counterId), "2"); + expect(testStrategy.getResult(pipeline, m2, linkId), "1,2"); + + await pipeline.run(input2); + expect(testStrategy.getResult(pipeline, m1, counterId), "3"); // no cache! + expect(testStrategy.getResult(pipeline, m2, counterId), "4"); + expect(testStrategy.getResult(pipeline, m2, linkId), "3,4"); + + await pipeline.run(input3); + expect(testStrategy.getResult(pipeline, m1, counterId), "3"); // same config + expect(testStrategy.getResult(pipeline, m2, counterId), "5"); + expect(testStrategy.getResult(pipeline, m2, linkId), "3,5"); + + await testStrategy.cleanup(pipeline); + }); } DataId _concatId = const DataId("concat"); diff --git a/pkg/modular_test/test/dependency_parser_test.dart b/pkg/modular_test/test/specification_parser_test.dart similarity index 63% rename from pkg/modular_test/test/dependency_parser_test.dart rename to pkg/modular_test/test/specification_parser_test.dart index f5362822020..6940d5e93ec 100644 --- a/pkg/modular_test/test/dependency_parser_test.dart +++ b/pkg/modular_test/test/specification_parser_test.dart @@ -3,46 +3,46 @@ // BSD-style license that can be found in the LICENSE file. import 'package:test/test.dart'; -import 'package:modular_test/src/dependency_parser.dart'; +import 'package:modular_test/src/test_specification_parser.dart'; main() { test('require dependencies section', () { - expect(() => parseDependencyMap(""), + expect(() => parseTestSpecification(""), throwsA(TypeMatcher())); }); test('dependencies is a map', () { - expect(() => parseDependencyMap("dependencies: []"), + expect(() => parseTestSpecification("dependencies: []"), throwsA(TypeMatcher())); }); test('dependencies can be a string or list of strings', () { - parseDependencyMap(''' + parseTestSpecification(''' dependencies: a: b '''); - parseDependencyMap(''' + parseTestSpecification(''' dependencies: a: [b, c] '''); - expect(() => parseDependencyMap(''' + expect(() => parseTestSpecification(''' dependencies: a: 1 '''), throwsA(TypeMatcher())); - expect(() => parseDependencyMap(''' + expect(() => parseTestSpecification(''' dependencies: a: true '''), throwsA(TypeMatcher())); - expect(() => parseDependencyMap(''' + expect(() => parseTestSpecification(''' dependencies: a: [false] '''), throwsA(TypeMatcher())); - expect(() => parseDependencyMap(''' + expect(() => parseTestSpecification(''' dependencies: a: c: d @@ -51,14 +51,26 @@ main() { test('result map is normalized', () { expect( - parseDependencyMap(''' + parseTestSpecification(''' dependencies: a: [b, c] b: d - '''), + ''').dependencies, equals({ 'a': ['b', 'c'], 'b': ['d'], })); }); + + test('flags are normalized', () { + expect(parseTestSpecification(''' + dependencies: {} + flags: "a" + ''').flags, equals(["a"])); + + expect(parseTestSpecification(''' + dependencies: {} + flags: ["a"] + ''').flags, equals(["a"])); + }); } diff --git a/pkg/modular_test/test/validate_test.dart b/pkg/modular_test/test/validate_pipeline_test.dart similarity index 95% rename from pkg/modular_test/test/validate_test.dart rename to pkg/modular_test/test/validate_pipeline_test.dart index 8394dfeaed6..76663336445 100644 --- a/pkg/modular_test/test/validate_test.dart +++ b/pkg/modular_test/test/validate_pipeline_test.dart @@ -27,7 +27,7 @@ main() { ]); }); - test('circular dependency is not allowed', () { + test('circular step dependency is not allowed', () { var id1 = DataId("data_a"); expect( () => validateSteps([ @@ -78,7 +78,7 @@ validateSteps(List steps) { /// An implementation of [Pipeline] that simply validates the steps, but doesn't /// do anything else. class _NoopPipeline extends Pipeline { - _NoopPipeline(List steps) : super(steps); + _NoopPipeline(List steps) : super(steps, false); @override Future runStep(ModularStep step, Module module, diff --git a/pkg/modular_test/test/validate_suite_test.dart b/pkg/modular_test/test/validate_suite_test.dart new file mode 100644 index 00000000000..e0fad1e03e2 --- /dev/null +++ b/pkg/modular_test/test/validate_suite_test.dart @@ -0,0 +1,61 @@ +// 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. + +/// Unit test for validation of modular steps in a pipeline. +import 'package:test/test.dart'; +import 'package:modular_test/src/suite.dart'; + +main() { + test('module test is not empty', () { + expect( + () => ModularTest([], null, []), throwsA(TypeMatcher())); + + var m = Module("a", [], Uri.parse("app:/"), []); + expect(() => ModularTest([], m, []), throwsA(TypeMatcher())); + }); + + test('module test must have a main module', () { + var m = Module("a", [], Uri.parse("app:/"), []); + expect(() => ModularTest([m], null, []), + throwsA(TypeMatcher())); + }); + + test('package must depend on package', () { + var m1a = Module("a", const [], Uri.parse("app:/"), + [Uri.parse("a1.dart"), Uri.parse("a2.dart")], + isPackage: false); + var m1b = Module("a", const [], Uri.parse("app:/"), + [Uri.parse("a1.dart"), Uri.parse("a2.dart")], + isPackage: true); + + var m2a = Module("b", [m1a], Uri.parse("app:/"), + [Uri.parse("b/b1.dart"), Uri.parse("b/b2.dart")], + isPackage: true); + var m2b = Module("b", [m1b], Uri.parse("app:/"), + [Uri.parse("b/b1.dart"), Uri.parse("b/b2.dart")], + isPackage: true); + expect(() => ModularTest([m1a, m2a], m2a, []), + throwsA(TypeMatcher())); + expect(ModularTest([m1b, m2b], m2b, []), isNotNull); + }); + + test('shared module must depend on shared modules', () { + var m1a = Module("a", const [], Uri.parse("app:/"), + [Uri.parse("a1.dart"), Uri.parse("a2.dart")], + isShared: false); + var m1b = Module("a", const [], Uri.parse("app:/"), + [Uri.parse("a1.dart"), Uri.parse("a2.dart")], + isShared: true); + + var m2a = Module("b", [m1a], Uri.parse("app:/"), + [Uri.parse("b/b1.dart"), Uri.parse("b/b2.dart")], + isShared: true); + var m2b = Module("b", [m1b], Uri.parse("app:/"), + [Uri.parse("b/b1.dart"), Uri.parse("b/b2.dart")], + isShared: true); + expect(() => ModularTest([m1a, m2a], m2a, []), + throwsA(TypeMatcher())); + expect(ModularTest([m1b, m2b], m2b, []), isNotNull); + }); +} diff --git a/pkg/pkg.status b/pkg/pkg.status index 881f0931d77..607d1b1dfd1 100644 --- a/pkg/pkg.status +++ b/pkg/pkg.status @@ -178,10 +178,10 @@ analyzer/tool/*: Skip # Issue 26813 [ $compiler != dart2analyzer && $runtime != vm ] dev_compiler/test/*: Skip -modular_test/test/dependency_parser_test: SkipByDesign modular_test/test/find_sdk_root1_test: SkipByDesign modular_test/test/io_pipeline_test: SkipByDesign modular_test/test/loader/loader_test: SkipByDesign +modular_test/test/specification_parser_test: SkipByDesign modular_test/test/src/find_sdk_root2_test: SkipByDesign [ $compiler == dart2js && $runtime == chrome && $system == macos ] diff --git a/tests/compiler/dart2js/modular/modular_test.dart b/tests/compiler/dart2js/modular/modular_test.dart index 80aa85755a1..2e37025d545 100644 --- a/tests/compiler/dart2js/modular/modular_test.dart +++ b/tests/compiler/dart2js/modular/modular_test.dart @@ -23,15 +23,23 @@ main(List args) { asyncTest(() async { var baseUri = Platform.script.resolve('data/'); var baseDir = Directory.fromUri(baseUri); + var pipeline = new IOPipeline([ + SourceToDillStep(), + GlobalAnalysisStep(), + Dart2jsBackendStep(), + RunD8(), + ], cacheSharedModules: true); await for (var entry in baseDir.list(recursive: false)) { if (entry is Directory) { - await _runTest(entry.uri, baseUri); + await _runTest(pipeline, entry.uri, baseUri); } } + + await pipeline.cleanup(); }); } -Future _runTest(Uri uri, Uri baseDir) async { +Future _runTest(IOPipeline pipeline, Uri uri, Uri baseDir) async { var dirName = uri.path.substring(baseDir.path.length); if (_options.filter != null && !dirName.contains(_options.filter)) { if (_options.showSkipped) print("skipped: $dirName"); @@ -41,13 +49,6 @@ Future _runTest(Uri uri, Uri baseDir) async { print("testing: $dirName"); ModularTest test = await loadTest(uri); if (_options.verbose) print(test.debugString()); - var pipeline = new IOPipeline([ - SourceToDillStep(), - GlobalAnalysisStep(), - Dart2jsBackendStep(), - RunD8(), - ]); - await pipeline.run(test); } @@ -153,6 +154,11 @@ class SourceToDillStep implements IOModularStep { Platform.resolvedExecutable, workerArgs, root.toFilePath()); _checkExitCode(result, this, module); } + + @override + void notifyCached(Module module) { + if (_options.verbose) print("cached step: source-to-dill on $module"); + } } // Step that invokes the dart2js global analysis on the main module by providing @@ -194,6 +200,12 @@ class GlobalAnalysisStep implements IOModularStep { _checkExitCode(result, this, module); } + + @override + void notifyCached(Module module) { + if (_options.verbose) + print("cached step: dart2js global analysis on $module"); + } } // Step that invokes the dart2js backend on the main module given the results of @@ -231,6 +243,11 @@ class Dart2jsBackendStep implements IOModularStep { _checkExitCode(result, this, module); } + + @override + void notifyCached(Module module) { + if (_options.verbose) print("cached step: dart2js backend on $module"); + } } /// Step that runs the output of dart2js in d8 and saves the output. @@ -269,6 +286,11 @@ class RunD8 implements IOModularStep { await File.fromUri(root.resolveUri(toUri(module, txtId))) .writeAsString(result.stdout); } + + @override + void notifyCached(Module module) { + if (_options.verbose) print("cached step: d8 on $module"); + } } void _checkExitCode(ProcessResult result, IOModularStep step, Module module) {