Driver vm service by dnfield · Pull Request #68654 · flutter/flutter

dnfield · 2020-10-20T21:20:42Z

Migrates package:flutter_driver to use package:vm_service.

This will be needed to eventually completely migrate flutter_driver to null safety. To that end, I've also rewritten the tests to not use mockito.

Part of #37499

Fixes #31813 since package:vm_service is now in charge of that behavior.

dnfield · 2020-10-20T21:21:43Z

packages/flutter_driver/test/flutter_driver_test.dart

    group('tap', () {
      test('requires a target reference', () async {
-        expect(driver.tap(null), throwsDriverError);
+        expect(driver.tap(null), throwsAssertionError);


Several of these changed and I'm not entirely sure why.

I'm not super concerned about it though, since before long we'll be migrating this to NNBD and this test will just go away anyway.

(The test and the entire class of error will change)

jonahwilliams · 2020-10-20T21:23:23Z

dev/integration_tests/flutter_gallery/pubspec.yaml

  url_launcher_linux: 0.0.1+1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
  url_launcher_macos: 0.0.1+8 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
-  url_launcher_platform_interface: 1.0.8 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
+  url_launcher_platform_interface: 1.0.9 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"


Doesn't this version need to be pinned?

I do not know, @mdebbar or @ditman may know what's going on there. I'm assuming by the time the tree is open again that will be resolved.

This PR will fix this issue. I'm working now on getting it green so I can land it.

Closing the loop here: Mouad published the fix for the build issue. Apologies for the breakage!

jonahwilliams

This is great

We don't really test drive on presubmit though, the unit tests are trivial. To make sure this can land without being reverted I would validate several devicelab tests locally.

Separately, you may need to investigate google3

dnfield · 2020-10-20T21:37:36Z

I did run some local driver tests that worked, and a handful of devicelab tests as well.

I need to get this through a FRoB run as well, but posting this is a step towards that.

jcollins-g · 2020-10-21T15:35:33Z

dartdoc crash tracking at dart-lang/dartdoc#2402

bkonyi

package:vm_service_client is dead! Long live package:vm_service!

bkonyi · 2020-10-21T15:42:31Z

packages/flutter_driver/test/flutter_driver_test.dart

+  Future<vms.Timeline> getVMTimeline({int timeOriginMicros, int timeExtentMicros}) async {
+    connectionLog.add('getVMTimeline $timeOriginMicros $timeExtentMicros');
+    final vms.Timeline timeline = timelineResponses[timeOriginMicros ?? 1];
+    assert(timeline != null, 'Missing entry in timelineResponses[$timeOriginMicros]');


Do we run with assertions enabled in tests? If we're not sure, maybe it's safer to just throw here.

Assertions are enabled for tests, yes.

dnfield · 2020-10-21T16:03:41Z

I'm still working through testing this internally, requires a few hoops to get vm_service visibility for flutter_driver in the monorepo.

dnfield · 2020-10-21T17:36:47Z

Internal runs are failing because of unrelated changes related to NNBD. I'll wait until that's cleaned up internally and try to re-run the tests.

jcollins-g · 2020-10-22T17:02:15Z

dartdoc crash tracking at dart-lang/dartdoc#2402

#68791 will fix this for Flutter.

jonahwilliams · 2020-10-22T18:53:03Z

packages/flutter_driver/lib/src/driver/vmservice_driver.dart

      }

-      return isolate.resume().catchError((dynamic e) {
+      return await client.resume(isolate.id).catchError((dynamic e) {


return await is pointless

jonahwilliams · 2020-10-22T18:53:20Z

packages/flutter_driver/lib/src/driver/vmservice_driver.dart

              'Attempted to resume an already resumed isolate. This may happen '
-                  'when we lose a race with another tool (usually a debugger) that '
-                  'is connected to the same isolate.'
+              'when we lose a race with another tool (usually a debugger) that '


nit while you are here: no "we"

jonahwilliams · 2020-10-22T18:53:43Z

packages/flutter_driver/lib/src/driver/vmservice_driver.dart

-        .loadRunnable()
-        .then((VMIsolate isolate) async {
-          if (isolate.extensionRpcs.contains(_flutterExtensionMethodName)) {
+      await client.streamListen(vms.EventStreams.kIsolate);


this will throw an exception if the stream has already been subscribed to

This would be unexpected, since we unsubscribe at the end of this method.

jonahwilliams · 2020-10-22T18:55:27Z

packages/flutter_driver/lib/src/driver/vmservice_driver.dart

-          .sendRequest(_collectAllGarbageMethodName, <String, String>{
-        'isolateId': 'isolates/${_appIsolate.numberAsString}',
-      });
+      await _serviceClient.callMethod(_collectAllGarbageMethodName, isolateId: _appIsolate.id);


I believe this method is deprecated and no longer does anything

We're "not supposed" to use it, but it does work. It's not guaranteed to do anything but it normally works well enough.

We can't remove this - it's used in many of our benchmarks to reduce flakiness.

dnfield · 2020-10-23T00:18:44Z

This will be blocked until a resolution to dart-lang/sdk#43898 is rolled into google.

dnfield · 2020-10-23T23:04:24Z

This patch is now passing internally, with some additional updates at cl/338576267 to cover new dependency changes and some internal usages of API that isn't available anymore (specifically, not using the vm_service_client API that we used to expose through this lib).

dnfield · 2020-10-23T23:08:10Z

@renyou -why doesn't FROB let me apply a G3Fix to this PR? It's approved by bkonyi.

yjbanov · 2020-10-26T15:56:04Z

dev/integration_tests/ui/test_driver/license_check_test.dart

-
-/// Workaround for isolates being paused by driver tests.
-/// https://github.com/flutter/flutter/issues/24703
-class IsolatesWorkaround {


This was actually fixed by a different patch, but became apparent in this one because it's trying to use API the driver no longer exposes :)

yjbanov · 2020-10-26T16:10:52Z

packages/flutter_driver/lib/src/driver/driver.dart

+  /// The `fuchsiaModuleTarget` parameter specifies the pattern for determining
+  /// which mod to control. When running on a Fuchsia device, either this or the
+  /// environment variable `FUCHSIA_MODULE_TARGET` must be set (the environment
+  /// variable is treated as a substring pattern). This field will be ignored if


Should we also say that this parameter is ignored on non-Fuchsia devices?

yjbanov · 2020-10-26T16:13:17Z

packages/flutter_driver/lib/src/driver/driver.dart

-  /// `browser` specifies which FlutterDriver implementation to use. If not
-  /// speicifed or set to false, [VMServiceFlutterDriver] implementation
-  /// will be used. Otherwise, [WebFlutterDriver] implementation will be used.
+  /// The `browser` parameter specifies which FlutterDriver implementation to


I'm failing to find the browser parameter this part is talking about 🤔

Looks like someone removed that and forgot to remove the docs. Removing this block.

yjbanov · 2020-10-26T16:18:24Z

packages/flutter_driver/lib/src/driver/vmservice_driver.dart

-        isolate.pauseEvent is! VMPauseInterruptedEvent &&
-        isolate.pauseEvent is! VMResumeEvent) {
-      isolate = await isolateRef.loadRunnable();
+    if (isolate.pauseEvent.kind == vms.EventKind.kNone) {


dnfield · 2020-10-26T21:15:19Z

Note to self or anyone else looking at this due to a devicelab failure:

I did not run all devicelab tests. We should feel free to aggressively revert this if it's causing devicelab failures. That said, if we need to revert we should ideally do so before it rolls into google3, as that will complicate reverts.

jonahwilliams

LGTM ship it

dnfield · 2020-10-27T00:09:41Z

All tests that CI should be able to catch are passing - will watch devicelab.

This reverts commit 3ecac30.

This reverts commit 3ecac30. Co-authored-by: Dan Field <[email protected]>

This reverts commit 19ce7a8.

This reverts commit 05ab0eb.

…" (#69089) This reverts commit 05ab0eb.

…" (flutter#69077)" (flutter#69089)" This reverts commit e581475.

* One more reland of "Driver vm service (#68654)" (#69074)" (#69077)" (#69089)" This reverts commit e581475. * pub run test

dnfield added 3 commits October 19, 2020 23:34

pubspecs

9ed51fb

migrate driver to vm_service, avoid mockito

b485a42

merge

bd50bb4

dnfield requested review from bkonyi, jonahwilliams and yjbanov October 20, 2020 21:20

flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 20, 2020

google-cla bot added the cla: yes label Oct 20, 2020

dnfield added the t: flutter driver "flutter driver", flutter_drive, or a driver test label Oct 20, 2020

dnfield commented Oct 20, 2020

View reviewed changes

jonahwilliams reviewed Oct 20, 2020

View reviewed changes

Remove obsolete workaround

2f06a30

bkonyi approved these changes Oct 21, 2020

View reviewed changes

jonahwilliams reviewed Oct 22, 2020

View reviewed changes

dnfield added 6 commits October 22, 2020 12:33

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

40e2555

review

07f4886

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

81f7bca

update docs

c9f6641

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

2b546f2

asdf

be0176b

dnfield mentioned this pull request Oct 23, 2020

Provide custom headers for the websocket in vm_service dart-lang/sdk#43898

Closed

dnfield added 2 commits October 23, 2020 09:03

headers back

4abae28

unused import

391c4d4

dnfield mentioned this pull request Oct 23, 2020

retry getting the main isolate #68894

Merged

dnfield added 4 commits October 23, 2020 14:27

retry, test, use toString on isolate number

f8458c7

timeout

5599d3d

toString

fccee36

merge

ebe564d

dnfield added 2 commits October 23, 2020 16:31

ci

9cddfcc

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

75126fd

yjbanov approved these changes Oct 26, 2020

View reviewed changes

dnfield added 2 commits October 26, 2020 09:25

update docs

2120bbb

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

511b7d4

dnfield added the waiting for tree to go green label Oct 26, 2020

merge

3ca2457

jonahwilliams approved these changes Oct 26, 2020

View reviewed changes

dnfield merged commit 3ecac30 into flutter:master Oct 27, 2020

dnfield added a commit that referenced this pull request Oct 27, 2020

Revert "Driver vm service (#68654)"

fe61e61

This reverts commit 3ecac30.

jonahwilliams mentioned this pull request Oct 27, 2020

Revert "Driver vm service" #69074

Merged

jonahwilliams pushed a commit that referenced this pull request Oct 27, 2020

Revert "Driver vm service (#68654)" (#69074)

19ce7a8

This reverts commit 3ecac30. Co-authored-by: Dan Field <[email protected]>

dnfield added a commit that referenced this pull request Oct 27, 2020

Revert "Revert "Driver vm service (#68654)" (#69074)"

389983a

This reverts commit 19ce7a8.

dnfield added a commit that referenced this pull request Oct 27, 2020

Revert "Revert "Driver vm service (#68654)" (#69074)" (#69077)

05ab0eb

This reverts commit 19ce7a8.

dnfield added a commit that referenced this pull request Oct 27, 2020

Revert "Revert "Revert "Driver vm service (#68654)" (#69074)" (#69077)"

18c3339

This reverts commit 05ab0eb.

dnfield added a commit that referenced this pull request Oct 27, 2020

Revert "Revert "Revert "Driver vm service (#68654)" (#69074)" (#69077)…

e581475

…" (#69089) This reverts commit 05ab0eb.

dnfield added a commit to dnfield/flutter that referenced this pull request Oct 27, 2020

One more reland of "Driver vm service (flutter#68654)" (flutter#69074)…

73f20ff

…" (flutter#69077)" (flutter#69089)" This reverts commit e581475.

dnfield mentioned this pull request Oct 27, 2020

reland driver vm_service migration #69126

Merged

dnfield added a commit that referenced this pull request Oct 27, 2020

reland driver vm_service migration (#69126)

36f9af3

* One more reland of "Driver vm service (#68654)" (#69074)" (#69077)" (#69089)" This reverts commit e581475. * pub run test

Conversation

dnfield commented Oct 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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 20, 2020

Uh oh!

jcollins-g commented Oct 21, 2020

Uh oh!

bkonyi left a comment

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!

dnfield commented Oct 21, 2020

Uh oh!

dnfield commented Oct 21, 2020

Uh oh!

jcollins-g commented Oct 22, 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!

dnfield commented Oct 23, 2020

Uh oh!

dnfield commented Oct 23, 2020

Uh oh!

dnfield commented Oct 23, 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!

dnfield commented Oct 26, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 27, 2020

Uh oh!

Reviewers

Assignees