Check exit code for test subprocess (#7269)

dev/automated_tests/test_smoke_test/README.md

@@ -1,2 +1,2 @@
-This directory is used by ///flutter/travis/test.sh to verify that
+This directory is used by //flutter/dev/bots/test.sh to verify that
 `flutter test` actually correctly fails when a test fails.

dev/automated_tests/test_smoke_test/crash1_test.dart

@@ -0,0 +1,16 @@
+// Copyright 2016 The Chromium Authors. 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:io' as system;
+
+import 'package:flutter_test/flutter_test.dart';
+
+// this is a test to make sure our tests consider engine crashes to be failures
+// see //flutter/dev/bots/test.sh
+
+void main() {
+  test('test smoke test -- this test should fail', () async {
+    system.Process.killPid(system.pid, system.ProcessSignal.SIGSEGV);
+  });
+}

dev/automated_tests/test_smoke_test/crash2_test.dart

@@ -0,0 +1,12 @@
+// Copyright 2016 The Chromium Authors. 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:io' as system;
+
+// this is a test to make sure our tests consider engine crashes to be failures
+// see //flutter/dev/bots/test.sh
+
+void main() {
+  system.Process.killPid(system.pid, system.ProcessSignal.SIGSEGV);
+}

dev/automated_tests/test_smoke_test/fail_test.dart

@@ -5,7 +5,7 @@
 import 'package:flutter_test/flutter_test.dart';
 
 // this is a test to make sure our tests actually catch failures
-// see ///flutter/travis/test.sh
+// see //flutter/dev/bots/test.sh
 
 void main() {
   test('test smoke test -- this test SHOULD FAIL', () async {

dev/automated_tests/test_smoke_test/missing_import_test.broken_dart

@@ -0,0 +1,10 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// this is a test to make sure our tests consider syntax errors to be failures
+// see //flutter/dev/bots/test.sh
+
+void main() {
+  fail(); // inspired by https://github.com/flutter/flutter/issues/2698
+}

dev/automated_tests/test_smoke_test/pass_test.dart

@@ -5,7 +5,7 @@
 import 'package:flutter_test/flutter_test.dart';
 
 // this is a test to make sure our tests actually catch failures
-// see ///flutter/travis/test.sh
+// see //flutter/dev/bots/test.sh
 
 void main() {
   test('test smoke test -- this test should pass', () async {

dev/automated_tests/test_smoke_test/syntax_error_test.broken_dart

@@ -0,0 +1,9 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// this is a test to make sure our tests consider syntax errors to be failures
+// see //flutter/dev/bots/test.sh
+
+The challenge: demand satisfaction
+If they apologize, no need for further action.

dev/bots/test.sh

@@ -22,6 +22,10 @@ flutter analyze --flutter-repo
 # verify that the tests actually return failure on failure and success on success
 (cd dev/automated_tests; ! flutter test test_smoke_test/fail_test.dart > /dev/null)
 (cd dev/automated_tests; flutter test test_smoke_test/pass_test.dart > /dev/null)
+(cd dev/automated_tests; ! flutter test test_smoke_test/crash1_test.dart > /dev/null)
+(cd dev/automated_tests; ! flutter test test_smoke_test/crash2_test.dart > /dev/null)
+(cd dev/automated_tests; ! flutter test test_smoke_test/syntax_error_test.broken_dart > /dev/null)
+(cd dev/automated_tests; ! flutter test test_smoke_test/missing_import_test.broken_dart > /dev/null)
 (cd packages/flutter_driver; ! flutter drive --use-existing-app -t test_driver/failure.dart >/dev/null 2>&1)
 
 COVERAGE_FLAG=

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

@@ -6,7 +6,7 @@ import 'dart:async';
 import 'dart:io';
 
 import 'package:path/path.dart' as path;
-import 'package:test/src/executable.dart' as executable; // ignore: implementation_imports
+import 'package:test/src/executable.dart' as test; // ignore: implementation_imports
 
 import '../base/common.dart';
 import '../base/logger.dart';
@@ -77,9 +77,9 @@ class TestCommand extends FlutterCommand {
         Directory.current = testDirectory;
       }
       printTrace('running test package with arguments: $testArgs');
-      await executable.main(testArgs);
+      await test.main(testArgs);
+      // test.main() sets dart:io's exitCode global.
       printTrace('test package returned with exit code $exitCode');
-
       return exitCode;
     } finally {
       Directory.current = currentDirectory;
@@ -164,10 +164,10 @@ class TestCommand extends FlutterCommand {
     if (argResults['coverage'])
       testArgs.insert(0, '--concurrency=1');
 
-    loader.installHook();
-    loader.shellPath = tools.getHostToolPath(HostTool.SkyShell);
-    if (!FileSystemEntity.isFileSync(loader.shellPath))
-      throwToolExit('Cannot find Flutter shell at ${loader.shellPath}');
+    final String shellPath = tools.getHostToolPath(HostTool.SkyShell) ?? Platform.environment['SKY_SHELL'];
+    if (!FileSystemEntity.isFileSync(shellPath))
+      throwToolExit('Cannot find Flutter shell at $shellPath');
+    loader.installHook(shellPath: shellPath);
 
     Cache.releaseLockEarly();
 

packages/flutter_tools/lib/src/test/coverage_collector.dart

@@ -20,14 +20,14 @@ class CoverageCollector {
   void collectCoverage({
     String host,
     int port,
-    Process processToKill
+    Process processToKill,
   }) {
     if (enabled) {
       assert(_jobs != null);
       _jobs.add(_startJob(
         host: host,
         port: port,
-        processToKill: processToKill
+        processToKill: processToKill,
       ));
     } else {
       processToKill.kill();
@@ -37,7 +37,7 @@ class CoverageCollector {
   Future<Null> _startJob({
     String host,
     int port,
-    Process processToKill
+    Process processToKill,
   }) async {
     int pid = processToKill.pid;
     printTrace('collecting coverage data from pid $pid on port $port');

packages/flutter_tools/lib/src/test/flutter_platform.dart

@@ -7,7 +7,6 @@ import 'dart:convert';
 import 'dart:io';
 import 'dart:math' as math;
 
-import 'package:async/async.dart';
 import 'package:path/path.dart' as path;
 import 'package:stream_channel/stream_channel.dart';
 
@@ -20,113 +19,198 @@ import '../dart/package_map.dart';
 import '../globals.dart';
 import 'coverage_collector.dart';
 
-final String _kSkyShell = Platform.environment['SKY_SHELL'];
+const Duration _kTestStartupTimeout = const Duration(seconds: 5);
 final InternetAddress _kHost = InternetAddress.LOOPBACK_IP_V4;
-const String _kRunnerPath = '/runner';
-const String _kShutdownPath = '/shutdown';
 
-String shellPath;
-
-List<String> fontDirectories = <String>[cache.getCacheArtifacts().path];
-
-void installHook() {
-  hack.registerPlatformPlugin(<TestPlatform>[TestPlatform.vm], () => new FlutterPlatform());
+void installHook({ String shellPath }) {
+  hack.registerPlatformPlugin(<TestPlatform>[TestPlatform.vm], () => new FlutterPlatform(shellPath: shellPath));
 }
 
-class _ServerInfo {
-  final String url;
-  final String shutdownUrl;
-  final Future<WebSocket> socket;
-  final HttpServer server;
-
-  _ServerInfo(this.server, this.url, this.shutdownUrl, this.socket);
-}
-
-Future<_ServerInfo> _startServer() async {
-  HttpServer server = await HttpServer.bind(_kHost, 0);
-  Completer<WebSocket> socket = new Completer<WebSocket>();
-  server.listen((HttpRequest request) {
-    if (request.uri.path == _kRunnerPath)
-      socket.complete(WebSocketTransformer.upgrade(request));
-    else if (!socket.isCompleted && request.uri.path == _kShutdownPath)
-      socket.completeError('Failed to start test');
-  });
-  return new _ServerInfo(server, 'ws://${_kHost.address}:${server.port}$_kRunnerPath',
-      'ws://${_kHost.address}:${server.port}$_kShutdownPath', socket.future);
-}
-
-Future<Process> _startProcess(String mainPath, { String packages, int observatoryPort }) {
-  assert(shellPath != null || _kSkyShell != null); // Please provide the path to the shell in the SKY_SHELL environment variable.
-  String executable = shellPath ?? _kSkyShell;
-  List<String> arguments = <String>[];
-  if (observatoryPort != null) {
-    arguments.add('--observatory-port=$observatoryPort');
-  } else {
-    arguments.add('--disable-observatory');
-  }
-  arguments.addAll(<String>[
-    '--enable-dart-profiling',
-    '--non-interactive',
-    '--enable-checked-mode',
-    '--packages=$packages',
-    mainPath
-  ]);
-  printTrace('$executable ${arguments.join(' ')}');
-  Map<String, String> environment = <String, String>{
-    'FLUTTER_TEST': 'true',
-    'FONTCONFIG_FILE': _fontConfigFile.path,
-  };
-  return processManager.start(executable, arguments, environment: environment);
-}
-
-void _attachStandardStreams(Process process) {
-  for (Stream<List<int>> stream in
-      <Stream<List<int>>>[process.stderr, process.stdout]) {
-    stream.transform(UTF8.decoder)
-      .transform(const LineSplitter())
-      .listen((String line) {
-        if (line != null)
-          print('Shell: $line');
-      });
-  }
-}
-
-File _cachedFontConfig;
-
-/// Returns a Fontconfig config file that limits font fallback to directories
-/// specified in [fontDirectories].
-File get _fontConfigFile {
-  if (_cachedFontConfig != null) return _cachedFontConfig;
-
-  Directory fontsDir = Directory.systemTemp.createTempSync('flutter_fonts');
-
-  StringBuffer sb = new StringBuffer();
-  sb.writeln('<fontconfig>');
-  for (String fontDir in fontDirectories) {
-    sb.writeln('  <dir>$fontDir</dir>');
-  }
-  sb.writeln('  <cachedir>/var/cache/fontconfig</cachedir>');
-  sb.writeln('</fontconfig>');
-
-  _cachedFontConfig = new File('${fontsDir.path}/fonts.conf');
-  _cachedFontConfig.createSync();
-  _cachedFontConfig.writeAsStringSync(sb.toString());
-  return _cachedFontConfig;
-}
+enum _InitialResult { crashed, timedOut, connected }
+enum _TestResult { crashed, harnessBailed, completed }
+typedef Future<Null> _Finalizer();
 
 class FlutterPlatform extends PlatformPlugin {
-  @override
-  StreamChannel<dynamic> loadChannel(String mainPath, TestPlatform platform) {
-    return StreamChannelCompleter.fromFuture(_startTest(mainPath));
+  FlutterPlatform({ this.shellPath }) {
+    assert(shellPath != null);
   }
 
-  Future<StreamChannel<dynamic>> _startTest(String mainPath) async {
-    _ServerInfo info = await _startServer();
-    Directory tempDir = Directory.systemTemp.createTempSync(
-        'dart_test_listener');
-    File listenerFile = new File('${tempDir.path}/listener.dart');
-    listenerFile.createSync();
-    listenerFile.writeAsStringSync('''
+  final String shellPath;
+
+  // 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
+  // that connects to our WebSocket server, then we proxy JSON messages from
+  // the test harness to the engine and back again. If at any time the engine
+  // crashes, we inject an error into that stream. When the process closes,
+  // we clean everything up.
+
+  @override
+  StreamChannel<dynamic> loadChannel(String testPath, TestPlatform platform) {
+    final StreamChannelController<dynamic> controller = new StreamChannelController<dynamic>(allowForeignErrors: false);
+    _startTest(testPath, controller.local);
+    return controller.foreign;
+  }
+
+  Future<Null> _startTest(String testPath, StreamChannel<dynamic> controller) async {
+    printTrace('starting test: $testPath');
+
+    final List<_Finalizer> finalizers = <_Finalizer>[];
+    bool subprocessActive = false;
+    bool controllerSinkClosed = false;
+    try {
+      controller.sink.done.then((_) { controllerSinkClosed = true; });
+
+      // Prepare our WebSocket server to talk to the engine subproces.
+      HttpServer server = await HttpServer.bind(_kHost, 0);
+      finalizers.add(() async { await server.close(force: true); });
+      Completer<WebSocket> webSocket = new Completer<WebSocket>();
+      server.listen((HttpRequest request) {
+        webSocket.complete(WebSocketTransformer.upgrade(request));
+      });
+
+      // Prepare a temporary directory to store the Dart file that will talk to us.
+      Directory temporaryDirectory = Directory.systemTemp.createTempSync('dart_test_listener');
+      finalizers.add(() async { temporaryDirectory.deleteSync(recursive: true); });
+
+      // Prepare the Dart file that will talk to us and start the test.
+      File listenerFile = new File('${temporaryDirectory.path}/listener.dart');
+      listenerFile.createSync();
+      listenerFile.writeAsStringSync(_generateTestMain(
+        testUrl: path.toUri(path.absolute(testPath)).toString(),
+        encodedWebsocketUrl: Uri.encodeComponent("ws://${_kHost.address}:${server.port}"),
+      ));
+
+      // If we are collecting coverage data, then set that up now.
+      int observatoryPort;
+      if (CoverageCollector.instance.enabled) {
+        // TODO(ianh): the random number on the next line is a landmine that will eventually
+        // cause a hard-to-find bug...
+        observatoryPort = CoverageCollector.instance.observatoryPort ?? new math.Random().nextInt(30000) + 2000;
+        await CoverageCollector.instance.finishPendingJobs();
+      }
+
+      // Start the engine subprocess.
+      Process process = await _startProcess(
+        shellPath,
+        listenerFile.path,
+        packages: PackageMap.globalPackagesPath,
+        observatoryPort: observatoryPort,
+      );
+      subprocessActive = true;
+      finalizers.add(() async {
+        if (subprocessActive)
+          process.kill();
+        int exitCode = await process.exitCode;
+        subprocessActive = false;
+        if (!controllerSinkClosed && exitCode != 0) {
+          String message = _getErrorMessage(_getExitCodeMessage(exitCode, 'after tests finished'), testPath, shellPath);
+          controller.sink.addError(new Exception(message));
+        }
+      });
+
+      // Pipe stdout and stderr from the subprocess to our printStatus console.
+      _pipeStandardStreamsToConsole(process);
+
+      // At this point, three things can happen next:
+      // The engine could crash, in which case process.exitCode will complete.
+      // The engine could connect to us, in which case webSocket.future will complete.
+      // The local test harness could get bored of us.
+
+      _InitialResult initialResult = await Future.any(<Future<_InitialResult>>[
+        process.exitCode.then<_InitialResult>((int exitCode) { return _InitialResult.crashed; }),
+        new Future<_InitialResult>.delayed(_kTestStartupTimeout, () { return _InitialResult.timedOut; }),
+        webSocket.future.then<_InitialResult>((WebSocket webSocket) { return _InitialResult.connected; }),
+      ]);
+
+      switch (initialResult) {
+        case _InitialResult.crashed:
+          int exitCode = await process.exitCode;
+          String message = _getErrorMessage(_getExitCodeMessage(exitCode, 'before connecting to test harness'), testPath, shellPath);
+          controller.sink.addError(new Exception(message));
+          controller.sink.close();
+          await controller.sink.done;
+          break;
+        case _InitialResult.timedOut:
+          String message = _getErrorMessage('Test never connected to test harness.', testPath, shellPath);
+          controller.sink.addError(new Exception(message));
+          controller.sink.close();
+          await controller.sink.done;
+          break;
+        case _InitialResult.connected:
+          WebSocket testSocket = await webSocket.future;
+
+          Completer<Null> harnessDone = new Completer<Null>();
+          StreamSubscription<dynamic> harnessToTest = controller.stream.listen(
+            (dynamic event) { testSocket.add(JSON.encode(event)); },
+            onDone: () { harnessDone.complete(); },
+          );
+
+          Completer<Null> testDone = new Completer<Null>();
+          StreamSubscription<dynamic> testToHarness = testSocket.listen(
+            (dynamic event) {
+              assert(event is String); // we shouldn't ever get binary messages
+              controller.sink.add(JSON.decode(event));
+            },
+            onDone: () { testDone.complete(); },
+          );
+
+          _TestResult testResult = await Future.any(<Future<_TestResult>>[
+            process.exitCode.then<_TestResult>((int exitCode) { return _TestResult.crashed; }),
+            testDone.future.then<_TestResult>((Null _) { return _TestResult.completed; }),
+            harnessDone.future.then<_TestResult>((Null _) { return _TestResult.harnessBailed; }),
+          ]);
+
+          harnessToTest.cancel();
+          testToHarness.cancel();
+
+          assert(!controllerSinkClosed);
+          switch (testResult) {
+            case _TestResult.crashed:
+              int exitCode = await process.exitCode;
+              subprocessActive = false;
+              String message = _getErrorMessage(_getExitCodeMessage(exitCode, 'before test harness closed its WebSocket'), testPath, shellPath);
+              controller.sink.addError(new Exception(message));
+              controller.sink.close();
+              await controller.sink.done;
+              break;
+            case _TestResult.completed:
+              break;
+            case _TestResult.harnessBailed:
+              break;
+          }
+          break;
+      }
+
+      CoverageCollector.instance.collectCoverage(
+        host: _kHost.address,
+        port: observatoryPort,
+        processToKill: process, // This kills the subprocess whether coverage is enabled or not.
+      );
+      subprocessActive = false;
+    } catch (e, stack) {
+      if (!controllerSinkClosed) {
+        controller.sink.addError(e, stack);
+      } else {
+        printError('unhandled error during test:\n$e\n$stack');
+      }
+    } finally {
+      for (_Finalizer finalizer in finalizers)
+        await finalizer();
+      if (!controllerSinkClosed) {
+        controller.sink.close();
+        await controller.sink.done;
+      }
+    }
+    assert(!subprocessActive);
+    assert(controllerSinkClosed);
+    printTrace('ending test: $testPath');
+  }
+
+  String _generateTestMain({
+    String testUrl,
+    String encodedWebsocketUrl,
+  }) {
+    return '''
 import 'dart:convert';
 import 'dart:io';
 
@@ -134,10 +218,10 @@ import 'package:stream_channel/stream_channel.dart';
 import 'package:test/src/runner/plugin/remote_platform_helpers.dart';
 import 'package:test/src/runner/vm/catch_isolate_errors.dart';
 
-import '${path.toUri(path.absolute(mainPath))}' as test;
+import '$testUrl' as test;
 
 void main() {
-  String server = Uri.decodeComponent('${Uri.encodeComponent(info.url)}');
+  String server = Uri.decodeComponent('$encodedWebsocketUrl');
   StreamChannel channel = serializeSuite(() {
     catchIsolateErrors();
     return test.main;
@@ -147,65 +231,82 @@ void main() {
     socket.addStream(channel.stream.map(JSON.encode));
   });
 }
-''');
+''';
+  }
 
-    int observatoryPort;
-    if (CoverageCollector.instance.enabled) {
-      observatoryPort = CoverageCollector.instance.observatoryPort ?? new math.Random().nextInt(30000) + 2000;
-      await CoverageCollector.instance.finishPendingJobs();
+  File _cachedFontConfig;
+
+  /// Returns a Fontconfig config file that limits font fallback to the
+  /// artifact cache directory.
+  File get _fontConfigFile {
+    if (_cachedFontConfig != null)
+      return _cachedFontConfig;
+
+    StringBuffer sb = new StringBuffer();
+    sb.writeln('<fontconfig>');
+    sb.writeln('  <dir>${cache.getCacheArtifacts().path}</dir>');
+    sb.writeln('  <cachedir>/var/cache/fontconfig</cachedir>');
+    sb.writeln('</fontconfig>');
+
+    Directory fontsDir = Directory.systemTemp.createTempSync('flutter_fonts');
+    _cachedFontConfig = new File('${fontsDir.path}/fonts.conf');
+    _cachedFontConfig.createSync();
+    _cachedFontConfig.writeAsStringSync(sb.toString());
+    return _cachedFontConfig;
+  }
+
+
+  Future<Process> _startProcess(String executable, String testPath, { String packages, int observatoryPort }) {
+    assert(executable != null); // Please provide the path to the shell in the SKY_SHELL environment variable.
+    List<String> arguments = <String>[];
+    if (observatoryPort != null) {
+      arguments.add('--observatory-port=$observatoryPort');
+    } else {
+      arguments.add('--disable-observatory');
     }
+    arguments.addAll(<String>[
+      '--enable-dart-profiling',
+      '--non-interactive',
+      '--enable-checked-mode',
+      '--packages=$packages',
+      testPath,
+    ]);
+    printTrace('$executable ${arguments.join(' ')}');
+    Map<String, String> environment = <String, String>{
+      'FLUTTER_TEST': 'true',
+      'FONTCONFIG_FILE': _fontConfigFile.path,
+    };
+    return processManager.start(executable, arguments, environment: environment);
+  }
 
-    Process process = await _startProcess(
-      listenerFile.path,
-      packages: PackageMap.globalPackagesPath,
-      observatoryPort: observatoryPort
-    );
-
-    _attachStandardStreams(process);
-
-    void finalize() {
-      if (process != null) {
-        Process processToKill = process;
-        process = null;
-        CoverageCollector.instance.collectCoverage(
-          host: _kHost.address,
-          port: observatoryPort,
-          processToKill: processToKill
-        );
-      }
-      if (tempDir != null) {
-        Directory dirToDelete = tempDir;
-        tempDir = null;
-        dirToDelete.deleteSync(recursive: true);
-      }
+  void _pipeStandardStreamsToConsole(Process process) {
+    for (Stream<List<int>> stream in
+        <Stream<List<int>>>[process.stderr, process.stdout]) {
+      stream.transform(UTF8.decoder)
+        .transform(const LineSplitter())
+        .listen((String line) {
+          if (line != null)
+            printStatus('Shell: $line');
+        });
     }
+  }
 
-    process.exitCode.then((_) {
-      WebSocket.connect(info.shutdownUrl);
-    });
+  String _getErrorMessage(String what, String testPath, String shellPath) {
+    return '$what\nTest: $testPath\nShell: $shellPath\n\n';
+  }
 
-    try {
-      WebSocket socket = await info.socket;
-      StreamChannel<dynamic> channel = new StreamChannel<dynamic>(socket.map(JSON.decode), socket);
-      return channel.transformStream(
-        new StreamTransformer<dynamic, dynamic>.fromHandlers(
-          handleDone: (EventSink<dynamic> sink) {
-            finalize();
-            sink.close();
-          }
-        )
-      ).transformSink(new StreamSinkTransformer<dynamic, String>.fromHandlers(
-        handleData: (dynamic data, StreamSink<String> sink) {
-          sink.add(JSON.encode(data));
-        },
-        handleDone: (EventSink<String> sink) {
-          finalize();
-          sink.close();
-        }
-      ));
-    } catch(e) {
-      finalize();
-      rethrow;
+  String _getExitCodeMessage(int exitCode, String when) {
+    switch (exitCode) {
+      case 0:
+        return 'Shell subprocess ended cleanly $when. Did main() call exit()?';
+      case -0x0f: // ProcessSignal.SIGTERM
+        return 'Shell subprocess crashed with SIGTERM ($exitCode) $when.';
+      case -0x0b: // ProcessSignal.SIGSEGV
+        return 'Shell subprocess crashed with segmentation fault $when.';
+      case -0x06: // ProcessSignal.SIGABRT
+        return 'Shell subprocess crashed with SIGABRT ($exitCode) $when.';
+      default:
+        return 'Shell subprocess crashed with unexpected exit code $exitCode $when.';
     }
   }
 }