Change in dart/sdk[master]: Add --build flag to test.py and test.dart
William Hesse (Gerrit)
Attention is currently required from: Alexander Thomas.
William Hesse would like Alexander Thomas to review this change.
Add --build flag to test.py and test.dart
This flag causes test.py and test.dart to (re)build the necessary
build targets locally to run the selected configurations.
Change-Id: I526de702ac0eb8269c91d0be3673af096c94c1f1
---
M pkg/test_runner/bin/test_runner.dart
A pkg/test_runner/lib/src/build_configurations.dart
M pkg/test_runner/lib/src/configuration.dart
M pkg/test_runner/lib/src/options.dart
M pkg/test_runner/lib/src/test_configurations.dart
5 files changed, 96 insertions(+), 8 deletions(-)
diff --git a/pkg/test_runner/bin/test_runner.dart b/pkg/test_runner/bin/test_runner.dart
index 3f5e048..6c50185 100644
--- a/pkg/test_runner/bin/test_runner.dart
+++ b/pkg/test_runner/bin/test_runner.dart
@@ -23,22 +23,22 @@
/// The default test directory layout is documented in "test_suite.dart", above
/// `factory StandardTestSuite.forDirectory`.
import "package:test_runner/src/options.dart";
+import "package:test_runner/src/build_configurations.dart";
import "package:test_runner/src/test_configurations.dart";
/// Runs all of the tests specified by the given command line [arguments].
-void main(List<String> arguments) {
+void main(List<String> arguments) async {
// Parse the command line arguments to a configuration.
var parser = OptionsParser();
+ List<TestConfiguration> configurations = [];
try {
- var configurations = parser.parse(arguments);
- if (configurations == null || configurations.isEmpty) return;
-
- // Run all of the configured tests.
- // TODO(26372): Ensure that all tasks complete and return a future from this
- // function.
- testConfigurations(configurations);
+ configurations = parser.parse(arguments);
} on OptionParseException catch (exception) {
print(exception.message);
exit(1);
}
+ if (configurations.isEmpty) return;
+ await buildConfigurations(configurations);
+ // Run all of the configured tests.
+ await testConfigurations(configurations);
}
diff --git a/pkg/test_runner/lib/src/build_configurations.dart b/pkg/test_runner/lib/src/build_configurations.dart
new file mode 100644
index 0000000..2d3dc7c
--- /dev/null
+++ b/pkg/test_runner/lib/src/build_configurations.dart
@@ -0,0 +1,79 @@
+// Copyright (c) 2020, 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.
+
+import 'dart:async';
+import 'dart:io';
+
+import 'configuration.dart';
+import 'path.dart';
+import 'utils.dart';
+
+Future buildConfigurations(List<TestConfiguration> configurations) async {
+ var startTime = DateTime.now();
+
+ // Extract global options from first configuration.
+ var firstConf = configurations[0];
+ print(firstConf.configuration);
+ if (!firstConf.build) return;
+ final buildTargets = Set<String>();
+ final modes = Set<Mode>();
+ final architectures = Set<Architecture>();
+ final systems = Set<System>();
+ for (final configuration in configurations) {
+ final inner = configuration.configuration;
+ buildTargets.addAll(_selectBuildTargets(inner));
+ modes.add(inner.mode);
+ architectures.add(inner.architecture);
+ systems.add(inner.system);
+ }
+ if (buildTargets.isEmpty) return;
+ if (systems.length > 1) {
+ print('Unimplemented: building for multiple systems ${systems.join(',')}');
+ }
+
+ final command = [
+ 'tools/build.py',
+ '-m',
+ modes.join(','),
+ '-a',
+ architectures.join(','),
+ if (systems.contains(System.android)) '--os=android',
+ ...buildTargets
+ ];
+ print('Running command: python ${command.join(' ')}');
+ final process = await Process.start('python', command);
+ stdout.nonBlocking.addStream(process.stdout);
+ stderr.nonBlocking.addStream(process.stderr);
+ print('exit code: ${await process.exitCode}');
+}
+
+List<String> _selectBuildTargets(Configuration inner) {
+ final result = <String>[];
+ final compiler = inner.compiler;
+ const targetsForCompilers = {
+ Compiler.dartk: ['runtime', 'create_sdk'], // Is create_sdk needed?
+ Compiler.dartkp: ['runtime', 'dart_precompiled_runtime'],
+ Compiler.appJitk: ['runtime'],
+ Compiler.fasta: ['create_sdk', 'dartdevc_test', 'kernel_platform_files'],
+ Compiler.dartdevk: ['dartdevc_test'],
+ Compiler.dart2js: ['create_sdk'],
+ Compiler.dart2analyzer: ['create_sdk'],
+ Compiler.specParser: <String>[],
+ };
+ result.addAll(targetsForCompilers[compiler]);
+
+ if (compiler == Compiler.dartkp &&
+ [Architecture.arm, Architecture.arm64, Architecture.arm_x64]
+ .contains(inner.architecture)) {
+ result.add('gen_snapshot');
+ }
+
+ if (compiler == Compiler.dartdevk && !inner.useSdk) {
+ result
+ ..remove('dartdevc_test')
+ ..add('dartdevc_test_local');
+ }
+
+ return result;
+}
diff --git a/pkg/test_runner/lib/src/configuration.dart b/pkg/test_runner/lib/src/configuration.dart
index a0bafae..12af614 100644
--- a/pkg/test_runner/lib/src/configuration.dart
+++ b/pkg/test_runner/lib/src/configuration.dart
@@ -27,6 +27,7 @@
{this.configuration,
this.progress,
this.selectors,
+ this.build,
this.testList,
this.repeat,
this.batch,
@@ -82,6 +83,7 @@
final bool batch;
final bool batchDart2JS;
+ final bool build;
final bool copyCoreDumps;
final bool rr;
final bool fastTestsOnly;
diff --git a/pkg/test_runner/lib/src/options.dart b/pkg/test_runner/lib/src/options.dart
index 231dd81..b73f36c 100644
--- a/pkg/test_runner/lib/src/options.dart
+++ b/pkg/test_runner/lib/src/options.dart
@@ -164,6 +164,8 @@
test options, specifying how tests should be run.''',
abbr: 'n',
hide: true),
+ _Option.bool(
+ 'build', 'Build the necessary targets to test this configuration'),
// TODO(sigmund): rename flag once we migrate all dart2js bots to the test
// matrix.
_Option.bool('host_checked', 'Run compiler with assertions enabled.',
@@ -355,6 +357,7 @@
/// For printing out reproducing command lines, we don't want to add these
/// options.
static final _denylistedOptions = {
+ 'build',
'build_directory',
'chrome',
'clean_exit',
@@ -749,6 +752,7 @@
configuration: innerConfiguration,
progress: progress,
selectors: selectors,
+ build: data["build"] as bool,
testList: data["test_list_contents"] as List<String>,
repeat: data["repeat"] as int,
batch: !(data["noBatch"] as bool),
diff --git a/pkg/test_runner/lib/src/test_configurations.dart b/pkg/test_runner/lib/src/test_configurations.dart
index 0b3fb76..999916c 100644
--- a/pkg/test_runner/lib/src/test_configurations.dart
+++ b/pkg/test_runner/lib/src/test_configurations.dart
@@ -18,6 +18,8 @@
import 'test_suite.dart';
import 'utils.dart';
+export 'configuration.dart' show TestConfiguration;
+
/// The directories that contain test suites which follow the conventions
/// required by [StandardTestSuite]'s forDirectory constructor.
///
@@ -53,6 +55,7 @@
Path('utils/tests/peg'),
];
+// TODO(26372): Ensure that the returned future awaits on all started tasks.
Future testConfigurations(List<TestConfiguration> configurations) async {
var startTime = DateTime.now();
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
William Hesse (Gerrit)
Attention is currently required from: Alexander Thomas.
Alexander Thomas (Gerrit)
Attention is currently required from: William Hesse.
Patch set 2:Code-Review +1
3 comments:
Patchset:
LGTM, nice!
File pkg/test_runner/lib/src/build_configurations.dart:
.first
Patch Set #2, Line 55:
'create_sdk'], // Is create_sdk needed?We should just use runtime here. If something really needs create_sdk, we should find out what that something is. Otherwise people may add additional dependencies that will be hard to remove.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Bob Nystrom (Gerrit)
Attention is currently required from: William Hesse.
5 comments:
Patchset:
Bunch of drive-by comments, but this is great! I've wanted this for *years*. :)
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 17:
print(firstConf.configuration);Debug code? Remove.
Patch Set #2, Line 19:
Set<String>()<String>{}
and likewise elsewhere.
Patch Set #2, Line 19:
finalThe rest of the test runner—well, at least 90+% of it—uses "var" for all local variables even ones that aren't re-assigned.
I don't have a strong opinion on which style we use, but I do have a strong opinion that we should use one style and apply it consistently.
Patch Set #2, Line 48:
print('exit code: ${await process.exitCode}');I would omit this if the exit code is zero. I'm generally trying to make the test runner more parsimonious with its output so that users can find the output they actually care about.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Thomas (Gerrit)
Attention is currently required from: William Hesse.
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 41:
if (systems.contains(System.android)) '--os=android',From offline discussion in chat:
We're ignoring the system here unless it's android. That's bad if, e.g. a macOS user tries to run a windows configuration. In the long run we, might want to "warn and help", but for now, fail fast would be better (current behaviour is to attempt to run tests and then fail in weird and surprising ways).
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Sigmund Cherem (Gerrit)
Attention is currently required from: Bob Nystrom, William Hesse.
3 comments:
Patchset:
Bunch of drive-by comments, but this is great! I've wanted this for *years*. […]
+1!
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 60:
Compiler.dart2js: ['create_sdk'],It would be great if we could make this configurable somehow. Could we move some of this data to the test_matrix.json?
Similar to this, for dart2js we want that if `useSdk` is false, we just build the platform file. For example:
`compile_dart2js_platform` (in weak mode)
or `compile_dart2js_nnbd_strong_platform` (in strong mode)
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Thomas (Gerrit)
Attention is currently required from: Sigmund Cherem, Bob Nystrom, William Hesse.
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 60:
Compiler.dart2js: ['create_sdk'],
It would be great if we could make this configurable somehow. […]
Long term, I see it more likely that these configurations would morph into test targets in the build. This is a quick way to get --build working but ideally the test matrix would be more declarative (defines what we want to test rather than how), test.py responsible for running tests, and GN/build.py would be responsible for building.
At that point the landscape would look something like this:
test_matrix.json: "test dart2js on chrome with assertions" (dart2js-assert-chrome)
test.py --build: build.py dart2js-assert-chrome; "run dart2js-assert-chrome" testsHow dart2js-assert-chrome is built would then be defined in a build rule (most likely, it will be an alias for some other rule, probably create_sdk) and we'd have a BUILD.gn file defining all the aliases. The build might also be a better home for "build-like" features in test.py that require dependency tracking, etc. Essentially, test.py transforms Dart files into results.json documents, which is a kind of build step.
At this point, we have no plans to switch the test_matrix to --build (that would blow up the scope and we'd have to ensure we don't lose performance on the bots). This feature is meant to make test.py more accessible but I'd guess some folks will prefer going to build.py directly (some on the VM team even use gn/ninja directly).I'm curious though, why would you prefer it in the test_matrix?
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
William Hesse (Gerrit)
Attention is currently required from: Sigmund Cherem, Bob Nystrom.
8 comments:
File pkg/test_runner/lib/src/build_configurations.dart:
.first
Done
Patch Set #2, Line 17:
print(firstConf.configuration);Debug code? Remove.
Done
Patch Set #2, Line 19:
final
The rest of the test runner—well, at least 90+% of it—uses "var" for all local variables even ones t […]
I would like to move to modern Dart style in this code, and think it is OK to do it at least for new files, remaining consistent in a file. Will probably do it for the entire package when adding sound null-safety.
Patch Set #2, Line 19:
Set<String>()
<String>{} […]
Done
Patch Set #2, Line 41:
if (systems.contains(System.android)) '--os=android',
From offline discussion in chat: […]
Done
Patch Set #2, Line 48:
print('exit code: ${await process.exitCode}');
I would omit this if the exit code is zero. […]
Done. But note that the output from the build command will also appear on stdout. We could change it to only print the output if there is a failure, if that would be better. Or make it dependent on the verbosity flag.
Patch Set #2, Line 55:
'create_sdk'], // Is create_sdk needed?
We should just use runtime here. […]
Verified with tryjobs that create_sdk is not needed on the builder that was requesting it in addition. Removed.
Similar to this, for dart2js we want that if `useSdk` is false, we just build the platform file. […]
When we make build targets for the configurations, we can make one for local testing. Right now, dart2js_bot is just equal to create_sdk, so there is no "smaller" version of create_sdk we can use for local testing (unless "runtime" is sufficient when useSdk is false). I would also want an evaluation of the useSdk == false option and determine that it remains worth supporting .
So we can add this when we make the build targets for dart2js_testing and possibly dart2js_local_testing.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Bob Nystrom (Gerrit)
Attention is currently required from: Sigmund Cherem, William Hesse.
2 comments:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 19:
finalI would like to move to modern Dart style in this code, and think it is OK to do it at least for new […]
There is nothing "non-modern" about using "var" for non-assigned local variables. We don't have any official guidance one way or the other and teams across google3 are split fairly evenly across the two styles.
Personally, I prefer using "var" everywhere, but if you'd like the test runner's style to be "final", then I'm OK with that. I do think we should apply it consistently, and enforce it with by enabling the https://dart-lang.github.io/linter/lints/prefer_final_locals.html lint. (And also ensuring everyone who hacks on the test_runner does so in an editor that supports lints.)
Are you OK with committing to migrate the entire test_runner to that style, either incrementally or all at once? I don't want it to just become less and less consistent over time.
Patch Set #2, Line 48:
print('exit code: ${await process.exitCode}');
We could change it to only print the output if there is a failure, if that would be better.
I would do this unless --verbose is passed, yes.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Sigmund Cherem (Gerrit)
Attention is currently required from: William Hesse.
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 60:
Compiler.dart2js: ['create_sdk'],Long term, I see it more likely that these configurations would morph into test targets in the build […]
I see - having them as test targets in the build makes sense and will likely support our goal too. I don't have a strong preference with the test_matrix.json. My thought was that it already has the steps to build, so it would be an incremental change to write those build dependencies declaratively (as opposed to having a step that calls build.py). That may allow you to declare what to build on different configurations without any configuration-specific logic in the test_runner's code.
More generally, there are two things I'm going after:
- We want to approach the ease of `bazel` that just naming a test target is enough for the system to figure out the minimum dependencies that it needs to build in order to run a test. The API of `test.py --build` seems like a nice fit for this.
- We want to increase our local velocity and want to slice up targets in smaller units, so that we can iterate quickly on changes and not rebuild what we don't need. For example, `create_sdk` is a heavy target that builds a lot of snapshots that are irrelevant to ddc and dart2js. Moreover, we often don't care to have the latest version of the VM and would be fine using one that was built a few CLs earlier.
For now, I think starting by adding a few build targets first is a good first step before we can iron out the second goal.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
William Hesse (Gerrit)
Attention is currently required from: Bob Nystrom.
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 19:
finalThere is nothing "non-modern" about using "var" for non-assigned local variables. […]
I am OK with committing to migrate the entire test_runner to prefer local finals.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
William Hesse (Gerrit)
William Hesse submitted this change.
Add --build flag to test.py and test.dart
This flag causes test.py and test.dart to (re)build the necessary
build targets locally to run the selected configurations.
Change-Id: I526de702ac0eb8269c91d0be3673af096c94c1f1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170822
Reviewed-by: Alexander Thomas <at...@google.com>
---
M pkg/test_runner/bin/test_runner.dart
A pkg/test_runner/lib/src/build_configurations.dart
M pkg/test_runner/lib/src/configuration.dart
M pkg/test_runner/lib/src/options.dart
M pkg/test_runner/lib/src/test_configurations.dart
5 files changed, 109 insertions(+), 8 deletions(-)
diff --git a/pkg/test_runner/bin/test_runner.dart b/pkg/test_runner/bin/test_runner.dart
index 3f5e048..7cb77ae 100644
--- a/pkg/test_runner/bin/test_runner.dart
+++ b/pkg/test_runner/bin/test_runner.dart
@@ -23,22 +23,22 @@
/// The default test directory layout is documented in "test_suite.dart", above
/// `factory StandardTestSuite.forDirectory`.
import "package:test_runner/src/options.dart";
+import "package:test_runner/src/build_configurations.dart";
import "package:test_runner/src/test_configurations.dart";
/// Runs all of the tests specified by the given command line [arguments].
-void main(List<String> arguments) {
+void main(List<String> arguments) async {
// Parse the command line arguments to a configuration.
var parser = OptionsParser();
+ var configurations = <TestConfiguration>[];
try {
- var configurations = parser.parse(arguments);
- if (configurations == null || configurations.isEmpty) return;
-
- // Run all of the configured tests.
- // TODO(26372): Ensure that all tasks complete and return a future from this
- // function.
- testConfigurations(configurations);
+ configurations = parser.parse(arguments);
} on OptionParseException catch (exception) {
print(exception.message);
exit(1);
}
+ if (configurations.isEmpty) return;
+ await buildConfigurations(configurations);
+ // Run all of the configured tests.
+ await testConfigurations(configurations);
}
diff --git a/pkg/test_runner/lib/src/build_configurations.dart b/pkg/test_runner/lib/src/build_configurations.dart
new file mode 100644
index 0000000..6179997
--- /dev/null
+++ b/pkg/test_runner/lib/src/build_configurations.dart
@@ -0,0 +1,92 @@
+// Copyright (c) 2020, 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.
+
+import 'dart:async';
+import 'dart:io';
+
+import 'configuration.dart';
+import 'utils.dart';
+
+Future buildConfigurations(List<TestConfiguration> configurations) async {
+ var startTime = DateTime.now();
+ if (!configurations.first.build) return;
+ final buildTargets = <String>{};
+ final modes = <Mode>{};
+ final architectures = <Architecture>{};
+ final systems = <System>{};
+ for (final configuration in configurations) {
+ final inner = configuration.configuration;
+ buildTargets.addAll(_selectBuildTargets(inner));
+ modes.add(inner.mode);
+ architectures.add(inner.architecture);
+ systems.add(inner.system);
+ }
+ if (buildTargets.isEmpty) return;
+ if (systems.length > 1) {
+ print('Unimplemented: building for multiple systems ${systems.join(',')}');
+ exit(1);
+ }
+ final system = systems.single;
+ final osFlags = <String>[];
+ if (system == System.android) {
+ osFlags.addAll(['--os', 'android']);
+ } else if (system == System.fuchsia) {
+ osFlags.addAll(['--os', 'fuchsia']);
+ } else {
+ final host = System.find(Platform.operatingSystem);
+ if (system != host) {
+ print('Unimplemented: running tests for $system on $host');
+ exit(1);
+ }
+ }
+ final command = [
+ 'tools/build.py',
+ '-m',
+ modes.join(','),
+ '-a',
+ architectures.join(','),
+ ...osFlags,
+ ...buildTargets
+ ];
+ print('Running command: python ${command.join(' ')}');
+ final process = await Process.start('python', command);
+ stdout.nonBlocking.addStream(process.stdout);
+ stderr.nonBlocking.addStream(process.stderr);
+ final exitCode = await process.exitCode;
+ if (exitCode != 0) {
+ print('exit code: $exitCode');
+ }
+ var buildTime = niceTime(DateTime.now().difference(startTime));
+ print('--- Build time: $buildTime ---');
+}
+
+List<String> _selectBuildTargets(Configuration inner) {
+ final result = <String>[];
+ final compiler = inner.compiler;
+ const targetsForCompilers = {
+ Compiler.dartk: ['runtime'],
index be34361..4d3925a 100644
--- a/pkg/test_runner/lib/src/test_configurations.dart
+++ b/pkg/test_runner/lib/src/test_configurations.dart
@@ -18,6 +18,8 @@
import 'test_suite.dart';
import 'utils.dart';
+export 'configuration.dart' show TestConfiguration;
+
/// The directories that contain test suites which follow the conventions
/// required by [StandardTestSuite]'s forDirectory constructor.
///
@@ -52,6 +54,7 @@
Path('utils/tests/peg'),
];
+// TODO(26372): Ensure that the returned future awaits on all started tasks.
Future testConfigurations(List<TestConfiguration> configurations) async {
var startTime = DateTime.now();
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Karl Klose (Gerrit)
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 19:
final
I am OK with committing to migrate the entire test_runner to prefer local finals.
I don't think we should convert the rest of the package to use final, but even if we decide to to it, I would prefer this change to be consistent with the current style.
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Dart CI (Gerrit)
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/d03b7e31f52927de75d4495eeb033eed490af438
Alexander Thomas (Gerrit)
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 60:
Compiler.dart2js: ['create_sdk'],I see - having them as test targets in the build makes sense and will likely support our goal too. […]
Agreed. The second goal needs a fair amount of work because the dependency tracking currently invalidates the snapshots on pretty much every change. And, we're also building DDC/Analyzer/etc.. snapshots even when only running dart2js tests... :(
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Thomas (Gerrit)
Attention is currently required from: William Hesse.
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 55:
'create_sdk'], // Is create_sdk needed?Verified with tryjobs that create_sdk is not needed on the builder that was requesting it in additio […]
Did you also remove it from the tryjobs?
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.
William Hesse (Gerrit)
1 comment:
File pkg/test_runner/lib/src/build_configurations.dart:
Patch Set #2, Line 55:
'create_sdk'], // Is create_sdk needed?
Did you also remove it from the tryjobs?
Removed from the dartk CI builders in https://dart-review.googlesource.com/c/sdk/+/210000
To view, visit change 170822. To unsubscribe, or for help writing mail filters, visit settings.