Refactor 'flutter drive' to get the observatory port from the logs (#7695)

This remove a very brittle aspect of flutter drive, whereby it would
assume a known port instead of explicitly finding out what it was.

Fixes #7692 and hopefully fixes the devicelab tests.

dev/devicelab/lib/tasks/perf_tests.dart

@@ -254,10 +254,8 @@ class MemoryTest {
           testTarget,
           '-d',
           deviceId,
-          '--use-existing-app',
-        ], environment: <String, String> {
-          'VM_SERVICE_URL': 'http://localhost:$observatoryPort'
-        });
+          '--use-existing-app=http://localhost:$observatoryPort',
+        ]);
 
         Map<String, dynamic> endData = await device.getMemoryStats(packageName);
         data['end_total_kb'] = endData['total_kb'];

packages/flutter_driver/lib/src/driver.dart

@@ -116,7 +116,7 @@ class FlutterDriver {
   ///
   /// [dartVmServiceUrl] is the URL to Dart observatory (a.k.a. VM service). If
   /// not specified, the URL specified by the `VM_SERVICE_URL` environment
-  /// variable is used, or 'http://localhost:8183'.
+  /// variable is used. One or the other must be specified.
   ///
   /// [printCommunication] determines whether the command communication between
   /// the test and the app should be printed to stdout.
@@ -127,7 +127,14 @@ class FlutterDriver {
                                          bool printCommunication: false,
                                          bool logCommunicationToFile: true }) async {
     dartVmServiceUrl ??= Platform.environment['VM_SERVICE_URL'];
-    dartVmServiceUrl ??= 'http://localhost:8183';
+
+    if (dartVmServiceUrl == null) {
+      throw new DriverError(
+        'Could not determine URL to connect to application.\n'
+        'Either the VM_SERVICE_URL environment variable should be set, or an explicit\n'
+        'URL should be provided to the FlutterDriver.connect() method.'
+      );
+    }
 
     // Connect to Dart VM servcies
     _log.info('Connecting to Flutter application at $dartVmServiceUrl');

packages/flutter_driver/test/flutter_driver_test.dart

@@ -53,7 +53,7 @@ void main() {
       when(mockIsolate.resume()).thenReturn(new Future<Null>.value());
       when(mockIsolate.onExtensionAdded).thenReturn(new Stream<String>.fromIterable(<String>['ext.flutter.driver']));
 
-      FlutterDriver driver = await FlutterDriver.connect();
+      FlutterDriver driver = await FlutterDriver.connect(dartVmServiceUrl: '');
       expect(driver, isNotNull);
       expectLogContains('Isolate is paused at start');
     });
@@ -62,7 +62,7 @@ void main() {
       when(mockIsolate.pauseEvent).thenReturn(new MockVMPauseBreakpointEvent());
       when(mockIsolate.resume()).thenReturn(new Future<Null>.value());
 
-      FlutterDriver driver = await FlutterDriver.connect();
+      FlutterDriver driver = await FlutterDriver.connect(dartVmServiceUrl: '');
       expect(driver, isNotNull);
       expectLogContains('Isolate is paused mid-flight');
     });
@@ -79,14 +79,14 @@ void main() {
         return new Future<Null>.error(new rpc.RpcException(101, ''));
       });
 
-      FlutterDriver driver = await FlutterDriver.connect();
+      FlutterDriver driver = await FlutterDriver.connect(dartVmServiceUrl: '');
       expect(driver, isNotNull);
       expectLogContains('Attempted to resume an already resumed isolate');
     });
 
     test('connects to unpaused isolate', () async {
       when(mockIsolate.pauseEvent).thenReturn(new MockVMResumeEvent());
-      FlutterDriver driver = await FlutterDriver.connect();
+      FlutterDriver driver = await FlutterDriver.connect(dartVmServiceUrl: '');
       expect(driver, isNotNull);
       expectLogContains('Isolate is not paused. Assuming application is ready.');
     });

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

@@ -55,14 +55,12 @@ class DriveCommand extends RunCommandBase {
         'Ignored if --use-existing-app is specified.'
     );
 
-    argParser.addFlag(
+    argParser.addOption(
       'use-existing-app',
-      negatable: true,
-      defaultsTo: false,
       help:
-        'Will not start a new Flutter application but connect to an '
-        'already running instance. This will also cause the driver to keep '
-        'the application running after tests are done.'
+        'Connect to an already running instance via the given observatory URL.\n'
+        'If this option is given, the application will not be automatically started\n'
+        'or stopped.'
     );
   }
 
@@ -101,7 +99,8 @@ class DriveCommand extends RunCommandBase {
     if (await fs.type(testFile) != FileSystemEntityType.FILE)
       throwToolExit('Test file not found: $testFile');
 
-    if (!argResults['use-existing-app']) {
+    String observatoryUri;
+    if (argResults['use-existing-app'] == null) {
       printStatus('Starting application: ${argResults["target"]}');
 
       if (getBuildMode() == BuildMode.release) {
@@ -114,30 +113,27 @@ class DriveCommand extends RunCommandBase {
         );
       }
 
-      int result = await appStarter(this);
-      if (result != 0)
-        throwToolExit('Application failed to start ($result). Will not run test. Quitting.', exitCode: result);
+      LaunchResult result = await appStarter(this);
+      if (result == null)
+        throwToolExit('Application failed to start. Will not run test. Quitting.', exitCode: 1);
+      observatoryUri = result.observatoryUri.toString();
     } else {
       printStatus('Will connect to already running application instance.');
+      observatoryUri = argResults['use-existing-app'];
     }
 
     Cache.releaseLockEarly();
 
     try {
-      await testRunner(<String>[testFile]);
+      await testRunner(<String>[testFile], observatoryUri);
     } catch (error, stackTrace) {
       if (error is ToolExit)
         rethrow;
       throwToolExit('CAUGHT EXCEPTION: $error\n$stackTrace');
     } finally {
-      if (!argResults['keep-app-running'] && !argResults['use-existing-app']) {
+      if (!argResults['keep-app-running'] && argResults['use-existing-app'] == null) {
         printStatus('Stopping application instance.');
-        try {
         await appStopper(this);
-        } catch(error, stackTrace) {
-          // TODO(yjbanov): remove this guard when this bug is fixed: https://github.com/dart-lang/sdk/issues/25862
-          printTrace('Could not stop application: $error\n$stackTrace');
-        }
       } else {
         printStatus('Leaving the application running.');
       }
@@ -253,18 +249,18 @@ Future<Device> findTargetDevice() async {
 }
 
 /// Starts the application on the device given command configuration.
-typedef Future<int> AppStarter(DriveCommand command);
+typedef Future<LaunchResult> AppStarter(DriveCommand command);
 
-AppStarter appStarter = startApp;
+AppStarter appStarter = _startApp;
 void restoreAppStarter() {
-  appStarter = startApp;
+  appStarter = _startApp;
 }
 
-Future<int> startApp(DriveCommand command) async {
+Future<LaunchResult> _startApp(DriveCommand command) async {
   String mainPath = findMainDartFile(command.targetFile);
   if (await fs.type(mainPath) != FileSystemEntityType.FILE) {
     printError('Tried to run $mainPath, but that file does not exist.');
-    return 1;
+    return null;
   }
 
   // TODO(devoncarew): We should remove the need to special case here.
@@ -316,19 +312,20 @@ Future<int> startApp(DriveCommand command) async {
 
   if (!result.started) {
     await command._deviceLogSubscription.cancel();
+    return null;
   }
 
-  return result.started ? 0 : 2;
+  return result;
 }
 
 /// Runs driver tests.
-typedef Future<Null> TestRunner(List<String> testArgs);
-TestRunner testRunner = runTests;
+typedef Future<Null> TestRunner(List<String> testArgs, String observatoryUri);
+TestRunner testRunner = _runTests;
 void restoreTestRunner() {
-  testRunner = runTests;
+  testRunner = _runTests;
 }
 
-Future<Null> runTests(List<String> testArgs) async {
+Future<Null> _runTests(List<String> testArgs, String observatoryUri) async {
   printTrace('Running driver tests.');
 
   PackageMap.globalPackagesPath = path.normalize(path.absolute(PackageMap.globalPackagesPath));
@@ -336,23 +333,26 @@ Future<Null> runTests(List<String> testArgs) async {
     ..add('--packages=${PackageMap.globalPackagesPath}')
     ..add('-rexpanded');
   String dartVmPath = path.join(dartSdkPath, 'bin', 'dart');
-  int result = await runCommandAndStreamOutput(<String>[dartVmPath]..addAll(args));
+  int result = await runCommandAndStreamOutput(
+    <String>[dartVmPath]..addAll(args),
+    environment: <String, String>{ 'VM_SERVICE_URL': observatoryUri }
+  );
   if (result != 0)
     throwToolExit('Driver tests failed: $result', exitCode: result);
 }
 
 
 /// Stops the application.
-typedef Future<int> AppStopper(DriveCommand command);
-AppStopper appStopper = stopApp;
+typedef Future<bool> AppStopper(DriveCommand command);
+AppStopper appStopper = _stopApp;
 void restoreAppStopper() {
-  appStopper = stopApp;
+  appStopper = _stopApp;
 }
 
-Future<int> stopApp(DriveCommand command) async {
+Future<bool> _stopApp(DriveCommand command) async {
   printTrace('Stopping application.');
   ApplicationPackage package = command.applicationPackages.getPackageForPlatform(command.device.platform);
   bool stopped = await command.device.stopApp(package);
   await command._deviceLogSubscription?.cancel();
-  return stopped ? 0 : 1;
+  return stopped;
 }

packages/flutter_tools/test/drive_test.dart

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:async';
-
 import 'package:file/memory.dart';
 import 'package:flutter_tools/src/android/android_device.dart';
 import 'package:flutter_tools/src/base/common.dart';
@@ -50,7 +48,7 @@ void main() {
       appStarter = (DriveCommand command) {
         throw 'Unexpected call to appStarter';
       };
-      testRunner = (List<String> testArgs) {
+      testRunner = (List<String> testArgs, String observatoryUri) {
         throw 'Unexpected call to testRunner';
       };
       appStopper = (DriveCommand command) {
@@ -86,7 +84,7 @@ void main() {
 
     testUsingContext('returns 1 when app fails to run', () async {
       withMockDevice();
-      appStarter = expectAsync((DriveCommand command) async => 1);
+      appStarter = expectAsync((DriveCommand command) async => null);
 
       String testApp = '/some/app/test_driver/e2e.dart';
       String testFile = '/some/app/test_driver/e2e_test.dart';
@@ -104,7 +102,7 @@ void main() {
         fail('Expect exception');
       } on ToolExit catch (e) {
         expect(e.exitCode, 1);
-        expect(e.message, contains('Application failed to start (1). Will not run test. Quitting.'));
+        expect(e.message, contains('Application failed to start. Will not run test. Quitting.'));
       }
     }, overrides: <Type, Generator>{
       FileSystem: () => memoryFileSystem,
@@ -155,15 +153,15 @@ void main() {
       String testApp = '/some/app/test/e2e.dart';
       String testFile = '/some/app/test_driver/e2e_test.dart';
 
-      appStarter = expectAsync((DriveCommand command) {
-        return new Future<int>.value(0);
+      appStarter = expectAsync((DriveCommand command) async {
+        return new LaunchResult.succeeded();
       });
-      testRunner = expectAsync((List<String> testArgs) {
+      testRunner = expectAsync((List<String> testArgs, String observatoryUri) async {
         expect(testArgs, <String>[testFile]);
-        return new Future<int>.value(0);
+        return null;
       });
-      appStopper = expectAsync((DriveCommand command) {
-        return new Future<int>.value(0);
+      appStopper = expectAsync((DriveCommand command) async {
+        return true;
       });
 
       MemoryFileSystem memFs = memoryFileSystem;
@@ -186,14 +184,14 @@ void main() {
       String testApp = '/some/app/test/e2e.dart';
       String testFile = '/some/app/test_driver/e2e_test.dart';
 
-      appStarter = expectAsync((DriveCommand command) {
-        return new Future<int>.value(0);
+      appStarter = expectAsync((DriveCommand command) async {
+        return new LaunchResult.succeeded();
       });
-      testRunner = (List<String> testArgs) {
+      testRunner = (List<String> testArgs, String observatoryUri) async {
         throwToolExit(null, exitCode: 123);
       };
-      appStopper = expectAsync((DriveCommand command) {
-        return new Future<int>.value(0);
+      appStopper = expectAsync((DriveCommand command) async {
+        return true;
       });
 
       MemoryFileSystem memFs = memoryFileSystem;