Allow developers to run flutter driver web test directly by angjieli · Pull Request #51084 · flutter/flutter

angjieli · 2020-02-20T00:37:27Z

Description

Previously, running flutter drive command against web devices would require developer to run flutter run to start the application first and specify --use-existing-app argument.

This PR is going to support running flutter drive command against web platform without starting the application.

For example, you can run flutter drive --target=test_driver/scroll_perf_web.dart -d web-server --release --browser-name=firefox under examples/flutter_gallery.

Note that it does not support debug mode for now (due to compiler error).

Related Issues

#51083

Tests

I added the following tests:
Updated test flutter_tools/test/commands.shard/hermetic/drive_test.dart.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

…er Drive command.

angjieli · 2020-02-20T00:37:48Z

@nturgut

jonahwilliams · 2020-02-20T00:43:43Z

packages/flutter_tools/lib/src/commands/drive.dart

 import '../base/file_system.dart';
 import '../base/process.dart';
 import '../build_info.dart';
+import '../build_runner/devfs_web.dart';


We cant let this implementation leak into the rest of the tool. There is an interface that flutter run uses, this needs to go through the same indirection

Good to know. Have this fixed by creating a getter on ResidentRunner.

jonahwilliams · 2020-02-20T00:44:05Z

packages/flutter_tools/lib/src/commands/drive.dart

+        // TODO(angjieli): remove this once running against
+        // target under test_driver in debug mode is supported
+        throwToolExit(
+            'Flutter Driver web does not support running in debug mode.\n'


nit: align and indent 2 spaces

jonahwilliams · 2020-02-20T00:44:39Z

packages/flutter_tools/lib/src/commands/drive.dart

+        if (result != 0) {
+          throwToolExit(null, exitCode: result);
+        }
+        final WebDevFS webDevFS = (runner as ResidentWebRunner).device.devFS as WebDevFS;


We should have a better way to surface this URI

jonahwilliams · 2020-02-20T01:17:57Z

packages/flutter_tools/lib/src/resident_runner.dart

  bool get isRunningRelease => debuggingOptions.buildInfo.isRelease;
  bool get supportsServiceProtocol => isRunningDebug || isRunningProfile;

+  Uri get uri => flutterDevices.first.devFS.baseUri;


nit: document as the URI of the first connected device for mobile, and only connected device for web.

Also when it will be null

jonahwilliams · 2020-02-20T01:18:29Z

packages/flutter_tools/lib/src/build_runner/devfs_web.dart


  @override
-  Uri get baseUri => null;
+  Uri get baseUri => _baseUri;


Could you add a simple unit test for the run_hot tests and the resident web runner tests?

jonahwilliams · 2020-02-20T01:19:25Z

packages/flutter_tools/lib/src/build_runner/devfs_web.dart

      entrypoint,
    );
-    return Uri.parse('http://$hostname:$port');
+    _baseUri = Uri.parse('http://$hostname:$port');


This might not be the correct information depending on what you need to connect to. For example, hostname might be something like a corp workstation hostname that needs to be resolved to an actual network address

For our use case, the application and test will be on the same machine so this should be fine.

nturgut · 2020-02-20T18:37:19Z

Thanks a lot @angjieli

/cc @yjbanov

I also added a new repository which has code for downloading the drivers: flutter/web_installers#1

I'll combine these in a smoke test so that we will have flutter web engine end to end tested: #51003

jonahwilliams · 2020-02-20T18:59:03Z

packages/flutter_tools/lib/src/commands/drive.dart

+        ResidentRunner runner;
+        final FlutterProject flutterProject = FlutterProject.current();
+        final FlutterDevice flutterDevice = await FlutterDevice.create(
+            device,


nit: 2 space indent

jonahwilliams · 2020-02-20T19:00:01Z

packages/flutter_tools/lib/src/commands/drive.dart

+        // need to know about analytics.
+        //
+        // Do not add more operations to the future.
+        final Completer<void> appStartedTimeRecorder = Completer<void>.sync();


Should you be awaiting this so that you're sure runner.uri is not null? Also, make it a regular completer and rename to reflect its current purpose

jonahwilliams · 2020-02-20T19:45:51Z

packages/flutter_tools/lib/src/commands/drive.dart

+          urlTunneller: null,
+        );
+
+        // Sync completer so the completing agent attaching to the resident doesn't


This comment isn't relevant to this code anymore

Code is not updated yet..Seems like there are something wrong with Github.

Code updated.

jonahwilliams · 2020-02-20T21:45:25Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

  }
+
+  @override
+  Future<void> exitApp() async {


Sorry, I just noticed these changes here. Shutting down a web app is a bit complicated - does this change modify the behavior of flutter run -d chrome or flutter run -d chrome --release when quiting?

No, it does not. We got the same behavior.

jonahwilliams

LGTM

fluttergithubbot · 2020-02-21T05:11:01Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite deploy_gallery-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite deploy_gallery-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.

nturgut · 2020-02-21T16:56:54Z

packages/flutter_tools/lib/src/commands/drive.dart

+          'Flutter Driver web does not support running in debug mode.\n'
+          '\n'
+          'Use --profile mode for testing application performance.\n'
+          'Use --release mode for testing correctness (with assertions).'


Are we going to address this in the next PRs?

As a team we are also have usecase for the driver tests in the debug mode as well. Thanks!

This is not a driver bug so @jonahwilliams probably knows more about it.
Currently, you will also get an error when running flutter run --target=test_driver/scroll_perf_web.dart --debug.

Actually, this should probably work at ToT

jonahwilliams · 2020-02-21T17:50:53Z

packages/flutter_tools/lib/src/commands/drive.dart


  if (webUri != null) {
    platformArgs['uri'] = webUri.toString();
+    // For web device, startApp will be triggered twice


I think --start-paused only works in debug mode, so this only needs to happen in profile/release mode

… app. Clean flutter run command from cirrus.yml

* smoke test for web * fix comments and remove logs * addressing reviewer comments * fix analyzer issue * running the test on cirrus * cirrus yaml syntax error * pub get for web_drivers * go to the examples directory before running the flutter app * cirrus is not able to find chromedriver. add a sleep to see if timing is the issue. * run chrome driver in the background * After PR #51084, flutter drive command can build and run a web app. Clean flutter run command from cirrus.yml * enable web

angjieli added 3 commits February 19, 2020 15:59

Launch web application when running flutter web driver test via Flutt…

59dd379

…er Drive command.

Merge remote-tracking branch 'upstream/master' into startapp

fc99a72

Remove warning when --use-existing-app is not used.

ca97d20

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 20, 2020

googlebot added the cla: yes label Feb 20, 2020

angjieli requested a review from jonahwilliams February 20, 2020 00:37

jonahwilliams reviewed Feb 20, 2020

View reviewed changes

Do not reference files under build_runner from drive.dart.

0c2e8fc

jonahwilliams reviewed Feb 20, 2020

View reviewed changes

angjieli added 2 commits February 20, 2020 10:45

Add tests for resident_runner and resident_web_runner.

7215f8d

More documentation.

6b8e188

jonahwilliams reviewed Feb 20, 2020

View reviewed changes

Fix indentation and update completer name.

189f84c

jonahwilliams reviewed Feb 20, 2020

View reviewed changes

Remove unnecessay comments.

5d36e81

jonahwilliams reviewed Feb 20, 2020

View reviewed changes

nturgut mentioned this pull request Feb 20, 2020

[web] Smoke tests for web engine #51003

Merged

13 tasks

jonahwilliams approved these changes Feb 21, 2020

View reviewed changes

Add some comments.

3fb5602

jonahwilliams added the waiting for tree to go green label Feb 21, 2020

fluttergithubbot removed the waiting for tree to go green label Feb 21, 2020

Fix space issue.

494f55a

jonahwilliams added the waiting for tree to go green label Feb 21, 2020

nturgut reviewed Feb 21, 2020

View reviewed changes

jonahwilliams removed the waiting for tree to go green label Feb 21, 2020

angjieli added 2 commits February 21, 2020 09:29

Fix issue for chrome.

0e62eaf

Fix documentation.

9ab1485

jonahwilliams reviewed Feb 21, 2020

View reviewed changes

Only set no-launch-chrome in non-debug mode.

764094c

jonahwilliams added the waiting for tree to go green label Feb 22, 2020

fluttergithubbot merged commit 5ee4147 into flutter:master Feb 23, 2020

angjieli mentioned this pull request Feb 24, 2020

Run Flutter web driver test without starting application first #51083

Closed

nturgut pushed a commit to nturgut/flutter that referenced this pull request Feb 24, 2020

After PR flutter#51084, flutter drive command can build and run a web…

2a49d7e

… app. Clean flutter run command from cirrus.yml

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

angjieli commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

angjieli commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturgut commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Feb 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

angjieli commented Feb 20, 2020 •

edited

Loading