Skip to content

Add --preview-dart-2 to 'flutter test' #14135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 19, 2018
Merged

Add --preview-dart-2 to 'flutter test' #14135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/flutter_tools/lib/src/commands/test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ class TestCommand extends FlutterCommand {
negatable: false,
help: 'Handle machine structured JSON command input\n'
'and provide output and progress in machine friendly format.');
argParser.addFlag('preview-dart-2',
hide: !verboseHelp,
help: 'Preview Dart 2.0 functionality.');
}

@override
Expand Down Expand Up @@ -206,6 +209,7 @@ class TestCommand extends FlutterCommand {
startPaused: startPaused,
ipv6: argResults['ipv6'],
machine: machine,
previewDart2: argResults['preview-dart-2'],
);

if (collector != null) {
Expand Down
13 changes: 7 additions & 6 deletions packages/flutter_tools/lib/src/compile.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ Future<String> compile(
bool aot : false,
bool strongMode : false,
List<String> extraFrontEndOptions,
String incrementalCompilerByteStorePath}) async {
String incrementalCompilerByteStorePath,
String packagesPath}) async {
final String frontendServer = artifacts.getArtifactPath(
Artifact.frontendServerSnapshotForEngineDartSdk
);
Expand All @@ -86,12 +87,12 @@ Future<String> compile(
command.add('--strong');
}
if (incrementalCompilerByteStorePath != null) {
command.addAll(<String>[
'--incremental',
'--byte-store',
incrementalCompilerByteStorePath]);
fs.directory(incrementalCompilerByteStorePath).createSync(recursive: true);
command.add('--incremental');
}
if (packagesPath != null) {
command.addAll(<String>['--packages', packagesPath]);
}

if (extraFrontEndOptions != null)
command.addAll(extraFrontEndOptions);
command.add(mainPath);
Expand Down
28 changes: 26 additions & 2 deletions packages/flutter_tools/lib/src/test/flutter_platform.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import 'dart:async';
import 'dart:convert';

import 'package:flutter_tools/src/artifacts.dart';
import 'package:flutter_tools/src/compile.dart';
import 'package:meta/meta.dart';
import 'package:stream_channel/stream_channel.dart';

Expand Down Expand Up @@ -56,6 +58,7 @@ void installHook({
bool enableObservatory: false,
bool machine: false,
bool startPaused: false,
bool previewDart2: false,
int observatoryPort,
InternetAddressType serverType: InternetAddressType.IP_V4,
}) {
Expand All @@ -71,6 +74,7 @@ void installHook({
startPaused: startPaused,
explicitObservatoryPort: observatoryPort,
host: _kHosts[serverType],
previewDart2: previewDart2,
),
);
}
Expand All @@ -88,6 +92,7 @@ class _FlutterPlatform extends PlatformPlugin {
this.startPaused,
this.explicitObservatoryPort,
this.host,
this.previewDart2,
}) : assert(shellPath != null);

final String shellPath;
Expand All @@ -97,6 +102,7 @@ class _FlutterPlatform extends PlatformPlugin {
final bool startPaused;
final int explicitObservatoryPort;
final InternetAddress host;
final bool previewDart2;

// Each time loadChannel() is called, we spin up a local WebSocket server,
// then spin up the engine in a subprocess. We pass the engine a Dart file
Expand Down Expand Up @@ -191,14 +197,28 @@ class _FlutterPlatform extends PlatformPlugin {
));

// Start the engine subprocess.
printTrace('test $ourTestCount: starting shell process');
printTrace('test $ourTestCount: starting shell process${previewDart2? " in preview-dart-2 mode":""}');

final String mainDart = previewDart2
? await compile(
sdkRoot: artifacts.getArtifactPath(Artifact.flutterPatchedSdkPath),
incrementalCompilerByteStorePath: '' /* not null is enough */,
mainPath: listenerFile.path,
Copy link
Contributor

@a-siva a-siva Jan 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also add parameter strongMode: true

Copy link
Member Author

@aam aam Jan 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test fails in strongMode, we won't be able to confirm the flow until flutter and frontend is strong mode clean. Given that I would prefer to add strong mode separately in follow up PR.

Copy link
Contributor

@a-siva a-siva Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok as discussed off line you will land this PR, follow it up with a PR to make it work on linux too and another PR to add the strong mode option.
It is important to get the strong mode option in as we want to get to the errors quickly.

packagesPath: PackageMap.globalPackagesPath
)
: listenerFile.path;

// bundlePath needs to point to a folder with `platform.dill` file.
final String bundlePath = previewDart2 ? artifacts.getArtifactPath(Artifact.flutterPatchedSdkPath) : null;

final Process process = await _startProcess(
Copy link
Contributor

@a-siva a-siva Jan 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --strong option needs to be passed to the engine being invoked also as we will want to also test the runtime strong mode checks.

shellPath,
listenerFile.path,
mainDart,
packages: PackageMap.globalPackagesPath,
enableObservatory: enableObservatory,
startPaused: startPaused,
observatoryPort: explicitObservatoryPort,
bundlePath: bundlePath,
);
subprocessActive = true;
finalizers.add(() async {
Expand Down Expand Up @@ -466,6 +486,7 @@ void main() {
String executable,
String testPath, {
String packages,
String bundlePath,
bool enableObservatory: false,
bool startPaused: false,
int observatoryPort,
Expand All @@ -492,6 +513,9 @@ void main() {
}
if (host.type == InternetAddressType.IP_V6)
command.add('--ipv6');
if (bundlePath != null) {
command.add('--flutter-assets-dir=$bundlePath');
Copy link
Contributor

@mehmetf mehmetf Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aam Do you recall why this was needed?

It is a bit odd that we are passing the SDK path as the asset dir.

Copy link
Member Author

@aam aam Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that option is the way how bundle path(that should have platform.dill) is passed to flutter tester(per accompanying flutter/engine#4564).

Copy link
Contributor

@mehmetf mehmetf Oct 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies all assets to be used by tester needs to be put in the sdk path. That's quite odd. In platform packaging the dill files are not found in the asset directory. Would it make sense to separate these concerns and change the way the dill files are located in the tester? [probably a new flag]?

Copy link
Member Author

@aam aam Oct 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it does make sense.

Copy link
Contributor

@mehmetf mehmetf Oct 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvolkert FYI I will attempt this if you don't have any objection. The request is to change tester to locate dill files w/o needing the asset flag so we can safely remove this particular line of code in flutter_platform.dart. When we add proper asset support [a separate param to installHook?], we can add it back. If you disagree with the methodology, let me know.

Copy link
Contributor

@tvolkert tvolkert Oct 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection. I agree that a new flag to separate these things for flutter_tester would make sense.

}
command.addAll(<String>[
'--enable-dart-profiling',
'--non-interactive',
Expand Down
2 changes: 2 additions & 0 deletions packages/flutter_tools/lib/src/test/runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Future<int> runTests(
bool startPaused: false,
bool ipv6: false,
bool machine: false,
bool previewDart2: false,
TestWatcher watcher,
}) async {
// Compute the command-line arguments for package:test.
Expand Down Expand Up @@ -75,6 +76,7 @@ Future<int> runTests(
machine: machine,
startPaused: startPaused,
serverType: serverType,
previewDart2: previewDart2,
);

// Make the global packages path absolute.
Expand Down