Remove randomness from port assignment during coverage collection. (#7548)

Also, defer to test package for throttling (this will require a test
package update as well).

Also, add a lot more instrumentation to --verbose mode for tests, and
fix minor trivial things here and there, and add error handling in
more places.

Also, refactor how coverage works to be simpler and not use statics.

bin/flutter

@@ -36,13 +36,16 @@ DART_SDK_PATH="$FLUTTER_ROOT/bin/cache/dart-sdk"
 
 DART="$DART_SDK_PATH/bin/dart"
 
+# To debug the tool, you can uncomment the following line to enable checked mode and set an observatory port:
+# FLUTTER_TOOL_ARGS="--observe=65432 --checked"
+
 (
   if hash flock 2>/dev/null; then
     flock 3 # ensures that we don't simultaneously update Dart in multiple parallel instances
     # some platforms (e.g. Mac) don't have flock or any reliable alternative
   fi
   REVISION=`(cd "$FLUTTER_ROOT"; git rev-parse HEAD)`
-  if [ ! -f "$SNAPSHOT_PATH" ] || [ ! -f "$STAMP_PATH" ] || [ `cat "$STAMP_PATH"` != "$REVISION" ] || [ "$FLUTTER_TOOLS_DIR/pubspec.yaml" -nt "$FLUTTER_TOOLS_DIR/pubspec.lock" ]; then
+  if [ ! -f "$SNAPSHOT_PATH" ] || [ ! -s "$STAMP_PATH" ] || [ `cat "$STAMP_PATH"` != "$REVISION" ] || [ "$FLUTTER_TOOLS_DIR/pubspec.yaml" -nt "$FLUTTER_TOOLS_DIR/pubspec.lock" ]; then
     mkdir -p "$FLUTTER_ROOT/bin/cache"
     touch "$FLUTTER_ROOT/bin/cache/.dartignore"
     "$FLUTTER_ROOT/bin/internal/update_dart_sdk.sh"
@@ -56,7 +59,7 @@ DART="$DART_SDK_PATH/bin/dart"
 ) 3< "$PROG_NAME"
 
 set +e
-"$DART" "$SNAPSHOT_PATH" "$@"
+"$DART" $FLUTTER_TOOL_ARGS "$SNAPSHOT_PATH" "$@"
 
 # The VM exits with code 253 if the snapshot version is out-of-date.
 # If it is, we need to snapshot it again.
@@ -67,4 +70,4 @@ fi
 
 set -e
 "$DART" --snapshot="$SNAPSHOT_PATH" --packages="$FLUTTER_TOOLS_DIR/.packages" "$SCRIPT_PATH"
-"$DART" "$SNAPSHOT_PATH" "$@"
+"$DART" $FLUTTER_TOOL_ARGS "$SNAPSHOT_PATH" "$@"

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

@@ -32,7 +32,7 @@ class TestCommand extends FlutterCommand {
     argParser.addFlag('merge-coverage',
       defaultsTo: false,
       negatable: false,
-      help: 'Whether to merge converage data with "coverage/lcov.base.info". '
+      help: 'Whether to merge converage data with "coverage/lcov.base.info".\n'
             'Implies collecting coverage data. (Requires lcov)'
     );
     argParser.addOption('coverage-path',
@@ -93,6 +93,7 @@ class TestCommand extends FlutterCommand {
       timeout: new Duration(seconds: 30),
     );
     status.stop();
+    printTrace('coverage information collection complete');
     if (coverageData == null)
       return false;
 
@@ -146,40 +147,42 @@ class TestCommand extends FlutterCommand {
 
   @override
   Future<Null> runCommand() async {
-    List<String> testArgs = argResults.rest.map((String testPath) => path.absolute(testPath)).toList();
+    List<String> testArgs = <String>[];
 
     commandValidator();
 
-    Directory testDir;
+    if (!terminal.supportsColor)
+      testArgs.addAll(<String>['--no-color', '-rexpanded']);
+
+    CoverageCollector collector;
+    if (argResults['coverage'] || argResults['merge-coverage']) {
+      collector = new CoverageCollector();
+      testArgs.add('--concurrency=1');
+    }
+
+    testArgs.add('--');
 
-    if (testArgs.isEmpty) {
+    Directory testDir;
+    List<String> files = argResults.rest.map((String testPath) => path.absolute(testPath)).toList();
+    if (files.isEmpty) {
       testDir = _currentPackageTestDir;
       if (!testDir.existsSync())
         throwToolExit("Test directory '${testDir.path}' not found.");
-
       testArgs.addAll(_findTests(testDir));
+    } else {
+      testArgs.addAll(files);
     }
 
-    testArgs.insert(0, '--');
-    if (!terminal.supportsColor)
-      testArgs.insertAll(0, <String>['--no-color', '-rexpanded']);
-
-    if (argResults['coverage'])
-      testArgs.insert(0, '--concurrency=1');
-
     final String shellPath = tools.getHostToolPath(HostTool.SkyShell) ?? Platform.environment['SKY_SHELL'];
     if (!fs.isFileSync(shellPath))
       throwToolExit('Cannot find Flutter shell at $shellPath');
-    loader.installHook(shellPath: shellPath);
+    loader.installHook(shellPath: shellPath, collector: collector);
 
     Cache.releaseLockEarly();
 
-    CoverageCollector collector = CoverageCollector.instance;
-    collector.enabled = argResults['coverage'] || argResults['merge-coverage'];
-
     int result = await _runTests(testArgs, testDir);
 
-    if (collector.enabled) {
+    if (collector != null) {
       if (!await _collectCoverageData(collector, mergeCoverageData: argResults['merge-coverage']))
         throwToolExit(null);
     }

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

@@ -14,43 +14,6 @@ import '../globals.dart';
 
 /// A class that's used to collect coverage data during tests.
 class CoverageCollector {
-  CoverageCollector._();
-
-  /// The singleton instance of the coverage collector.
-  static final CoverageCollector instance = new CoverageCollector._();
-
-  /// By default, coverage collection is not enabled. Set [enabled] to true
-  /// to turn on coverage collection.
-  bool enabled = false;
-  int observatoryPort;
-
-  /// Adds a coverage collection tasks to the pending queue. The task will not
-  /// begin collecting coverage data until [CoverageCollectionTask.start] is
-  /// called.
-  ///
-  /// When a process is spawned to accumulate code coverage data, this method
-  /// should be called before the process terminates so that this collector
-  /// knows to wait for the coverage data in [finalizeCoverage].
-  ///
-  /// If this collector is not [enabled], the task will still be added to the
-  /// pending queue. Only when the task is started will the enabled state of
-  /// the collector be consulted.
-  CoverageCollectionTask addTask({
-    String host,
-    int port,
-    Process processToKill,
-  }) {
-    final CoverageCollectionTask task = new CoverageCollectionTask._(
-      this,
-      host,
-      port,
-      processToKill,
-    );
-    _tasks.add(task._future);
-    return task;
-  }
-
-  List<Future<Null>> _tasks = <Future<Null>>[];
   Map<String, dynamic> _globalHitmap;
 
   void _addHitmap(Map<String, dynamic> hitmap) {
@@ -60,36 +23,43 @@ class CoverageCollector {
       mergeHitmaps(hitmap, _globalHitmap);
   }
 
-  /// Returns a future that completes once all tasks have finished.
-  /// This will not start any tasks that were not already started.
+  /// Collects coverage for the given [Process] using the given `port`.
   ///
-  /// If [timeout] is specified, the future will timeout (with a
-  /// [TimeoutException]) after the specified duration.
-  Future<Null> finishPendingTasks({ Duration timeout }) {
-    Future<dynamic> future = Future.wait(_tasks, eagerError: true);
-    if (timeout != null)
-      future = future.timeout(timeout);
-    return future;
+  /// This should be called when the code whose coverage data is being collected
+  /// has been run to completion so that all coverage data has been recorded.
+  ///
+  /// The returned [Future] completes when the coverage is collected.
+  Future<Null> collectCoverage(Process process, InternetAddress host, int port) async {
+    assert(process != null);
+    assert(port != null);
+
+    int pid = process.pid;
+    int exitCode;
+    process.exitCode.then((int code) {
+      exitCode = code;
+    });
+
+    printTrace('pid $pid (port $port): collecting coverage data...');
+    final Map<dynamic, dynamic> data = await collect(host.address, port, false, false);
+    printTrace('pid $pid (port $port): ${ exitCode != null ? "process terminated prematurely with exit code $exitCode; aborting" : "collected coverage data; merging..." }');
+    if (exitCode != null)
+      throw new Exception('Failed to collect coverage, process terminated prematurely.');
+    _addHitmap(createHitmap(data['coverage']));
+    printTrace('pid $pid (port $port): done merging coverage data into global coverage map.');
   }
 
   /// Returns a future that will complete with the formatted coverage data
   /// (using [formatter]) once all coverage data has been collected.
   ///
-  /// This will not start any collection tasks. It us up to the caller of
-  /// [addTask] to maintain a reference to the [CoverageCollectionTask] and
-  /// call `start` on the task once the code in question has run. Failure to do
-  /// so will cause this method to wait indefinitely for the task.
+  /// This will not start any collection tasks. It us up to the caller of to
+  /// call [collectCoverage] for each process first.
   ///
   /// If [timeout] is specified, the future will timeout (with a
   /// [TimeoutException]) after the specified duration.
-  ///
-  /// This must only be called if this collector is [enabled].
   Future<String> finalizeCoverage({
     Formatter formatter,
     Duration timeout,
   }) async {
-    assert(enabled);
-    await finishPendingTasks(timeout: timeout);
     printTrace('formating coverage data');
     if (_globalHitmap == null)
       return null;
@@ -99,68 +69,8 @@ class CoverageCollector {
       List<String> reportOn = <String>[path.join(packagePath, 'lib')];
       formatter = new LcovFormatter(resolver, reportOn: reportOn, basePath: packagePath);
     }
-    return await formatter.format(_globalHitmap);
-  }
-}
-
-/// A class that represents a pending task of coverage data collection.
-/// Instances of this class are obtained when a process starts running code
-/// by calling [CoverageCollector.addTask]. Then, when the code has run to
-/// completion (all the coverage data has been recorded), the task is started
-/// to actually collect the coverage data.
-class CoverageCollectionTask {
-  final Completer<Null> _completer = new Completer<Null>();
-  final CoverageCollector _collector;
-  final String _host;
-  final int _port;
-  final Process _processToKill;
-
-  CoverageCollectionTask._(
-    this._collector,
-    this._host,
-    this._port,
-    this._processToKill,
-  );
-
-  bool _started = false;
-
-  Future<Null> get _future => _completer.future;
-
-  /// Starts the task of collecting coverage.
-  ///
-  /// This should be called when the code whose coverage data is being collected
-  /// has been run to completion so that all coverage data has been recorded.
-  /// Failure to do so will cause [CoverageCollector.finalizeCoverage] to wait
-  /// indefinitely for the task to complete.
-  ///
-  /// Each task may only be started once.
-  void start() {
-    assert(!_started);
-    _started = true;
-
-    if (!_collector.enabled) {
-      _processToKill.kill();
-      _completer.complete();
-      return;
-    }
-
-    int pid = _processToKill.pid;
-    printTrace('collecting coverage data from pid $pid on port $_port');
-    collect(_host, _port, false, false).then(
-      (Map<dynamic, dynamic> data) {
-        printTrace('done collecting coverage data from pid $pid');
-        _processToKill.kill();
-        try {
-          _collector._addHitmap(createHitmap(data['coverage']));
-          printTrace('done merging data from pid $pid into global coverage map');
-          _completer.complete();
-        } catch (error, stackTrace) {
-          _completer.completeError(error, stackTrace);
-        }
-      },
-      onError: (dynamic error, StackTrace stackTrace) {
-        _completer.completeError(error, stackTrace);
-      },
-    );
+    String result = await formatter.format(_globalHitmap);
+    _globalHitmap = null;
+    return result;
   }
 }

packages/flutter_tools/lib/src/usage.dart

@@ -104,11 +104,11 @@ class Usage {
 
   /// Returns when the last analytics event has been sent, or after a fixed
   /// (short) delay, whichever is less.
-  Future<Null> ensureAnalyticsSent() {
+  Future<Null> ensureAnalyticsSent() async {
     // TODO(devoncarew): This may delay tool exit and could cause some analytics
     // events to not be reported. Perhaps we could send the analytics pings
     // out-of-process from flutter_tools?
-    return _analytics.waitForLastPing(timeout: new Duration(milliseconds: 250));
+    await _analytics.waitForLastPing(timeout: new Duration(milliseconds: 250));
   }
 
   void printUsage() {