Some minor cleanup for flutter_tools (#36569)

2019-07-24 10:58:09 -07:00 · 2019-07-24 10:58:09 -07:00 · e8d7306828
commit e8d7306828
parent 6fe45fb1ac
15 changed files with 65 additions and 55 deletions
--- a/packages/flutter_tools/lib/src/base/io.dart
+++ b/packages/flutter_tools/lib/src/base/io.dart
@ -26,7 +26,7 @@
 /// increase the API surface that we have to test in Flutter tools, and the APIs
 /// in `dart:io` can sometimes be hard to use in tests.
 import 'dart:async';
-import 'dart:io' as io show exit, IOSink, ProcessSignal, stderr, stdin, Stdout, stdout;
+import 'dart:io' as io show exit, IOSink, Process, ProcessSignal, stderr, stdin, Stdout, stdout;

 import 'package:meta/meta.dart';

@ -116,7 +116,12 @@ void restoreExitFunction() {
 /// Listening on signals that don't exist on the current platform is just a
 /// no-op. This is in contrast to [io.ProcessSignal], where listening to
 /// non-existent signals throws an exception.
-class ProcessSignal implements io.ProcessSignal {
+///
+/// This class does NOT implement io.ProcessSignal, because that class uses
+/// private fields. This means it cannot be used with, e.g., [Process.killPid].
+/// Alternative implementations of the relevant methods that take
+/// [ProcessSignal] instances are available on this class (e.g. "send").
+class ProcessSignal {
  @visibleForTesting
  const ProcessSignal(this._delegate);

@ -129,11 +134,23 @@ class ProcessSignal implements io.ProcessSignal {

  final io.ProcessSignal _delegate;

-  @override
  Stream<ProcessSignal> watch() {
    return _delegate.watch().map<ProcessSignal>((io.ProcessSignal signal) => this);
  }

+  /// Sends the signal to the given process (identified by pid).
+  ///
+  /// Returns true if the signal was delivered, false otherwise.
+  ///
+  /// On Windows, this can only be used with [ProcessSignal.SIGTERM], which
+  /// terminates the process.
+  ///
+  /// This is implemented by sending the signal using [Process.killPid].
+  bool send(int pid) {
+    assert(!platform.isWindows || this == ProcessSignal.SIGTERM);
+    return io.Process.killPid(pid, _delegate);
+  }
+
  @override
  String toString() => _delegate.toString();
 }
--- a/packages/flutter_tools/lib/src/commands/attach.dart
+++ b/packages/flutter_tools/lib/src/commands/attach.dart
@ -89,16 +89,13 @@ class AttachCommand extends FlutterCommand {
        'project-root',
        hide: !verboseHelp,
        help: 'Normally used only in run target',
-      )..addFlag('track-widget-creation',
-        hide: !verboseHelp,
-        help: 'Track widget creation locations.',
-        defaultsTo: false,
      )..addFlag('machine',
        hide: !verboseHelp,
        negatable: false,
        help: 'Handle machine structured JSON command input and provide output '
              'and progress in machine friendly format.',
      );
+    usesTrackWidgetCreation(verboseHelp: verboseHelp);
    hotRunnerFactory ??= HotRunnerFactory();
  }

--- a/packages/flutter_tools/lib/src/commands/build.dart
+++ b/packages/flutter_tools/lib/src/commands/build.dart
@ -23,7 +23,7 @@ class BuildCommand extends FlutterCommand {
    addSubcommand(BuildAarCommand(verboseHelp: verboseHelp));
    addSubcommand(BuildApkCommand(verboseHelp: verboseHelp));
    addSubcommand(BuildAppBundleCommand(verboseHelp: verboseHelp));
-    addSubcommand(BuildAotCommand());
+    addSubcommand(BuildAotCommand(verboseHelp: verboseHelp));
    addSubcommand(BuildIOSCommand());
    addSubcommand(BuildBundleCommand(verboseHelp: verboseHelp));
    addSubcommand(BuildWebCommand());
--- a/packages/flutter_tools/lib/src/commands/build_aot.dart
+++ b/packages/flutter_tools/lib/src/commands/build_aot.dart
@ -21,7 +21,7 @@ import '../runner/flutter_command.dart';
 import 'build.dart';

 class BuildAotCommand extends BuildSubCommand with TargetPlatformBasedDevelopmentArtifacts {
-  BuildAotCommand() {
+  BuildAotCommand({bool verboseHelp = false}) {
    usesTargetOption();
    addBuildModeFlags();
    usesPubOption();
@ -37,12 +37,6 @@ class BuildAotCommand extends BuildSubCommand with TargetPlatformBasedDevelopmen
        defaultsTo: false,
        help: 'Report timing information about build steps in machine readable form,',
      )
-      // track-widget-creation is exposed as a flag here but ignored to deal with build
-      // invalidation issues - there are no plans to support it for AOT mode.
-      ..addFlag('track-widget-creation',
-        defaultsTo: false,
-        hide: true,
-      )
      ..addMultiOption('ios-arch',
        splitCommas: true,
        defaultsTo: defaultIOSArchs.map<String>(getNameForIOSArch),
@ -62,6 +56,10 @@ class BuildAotCommand extends BuildSubCommand with TargetPlatformBasedDevelopmen
        help: 'Build the AOT bundle with bitcode. Requires a compatible bitcode engine.',
        hide: true,
      );
+    // --track-widget-creation is exposed as a flag here to deal with build
+    // invalidation issues, but it is ignored -- there are no plans to support
+    // it for AOT mode.
+    usesTrackWidgetCreation(hasEffect: false, verboseHelp: verboseHelp);
  }

  @override
--- a/packages/flutter_tools/lib/src/commands/build_apk.dart
+++ b/packages/flutter_tools/lib/src/commands/build_apk.dart
@ -22,7 +22,6 @@ class BuildApkCommand extends BuildSubCommand {
    usesBuildNameOption();

    argParser
-      ..addFlag('track-widget-creation', negatable: false, hide: !verboseHelp)
      ..addFlag('split-per-abi',
        negatable: false,
        help: 'Whether to split the APKs per ABIs.'
@ -34,6 +33,7 @@ class BuildApkCommand extends BuildSubCommand {
        allowed: <String>['android-arm', 'android-arm64', 'android-x86', 'android-x64'],
        help: 'The target platform for which the app is compiled.',
      );
+    usesTrackWidgetCreation(verboseHelp: verboseHelp);
  }

  @override
--- a/packages/flutter_tools/lib/src/commands/build_bundle.dart
+++ b/packages/flutter_tools/lib/src/commands/build_bundle.dart
@ -41,10 +41,6 @@ class BuildBundleCommand extends BuildSubCommand {
          'windows-x64',
        ],
      )
-      ..addFlag('track-widget-creation',
-        hide: !verboseHelp,
-        help: 'Track widget creation locations. Requires Dart 2.0 functionality.',
-      )
      ..addMultiOption(FlutterOptions.kExtraFrontEndOptions,
        splitCommas: true,
        hide: true,
@ -59,6 +55,7 @@ class BuildBundleCommand extends BuildSubCommand {
              'in the application\'s LICENSE file.',
        defaultsTo: false);
    usesPubOption();
+    usesTrackWidgetCreation(verboseHelp: verboseHelp);

    bundleBuilder ??= BundleBuilder();
  }
--- a/packages/flutter_tools/lib/src/commands/run.dart
+++ b/packages/flutter_tools/lib/src/commands/run.dart
@ -49,6 +49,7 @@ abstract class RunCommandBase extends FlutterCommand with DeviceBasedDevelopment
    usesPortOptions();
    usesIpv6Flag();
    usesPubOption();
+    usesTrackWidgetCreation(verboseHelp: verboseHelp);
    usesIsolateFilterOption(hide: !verboseHelp);
  }

@ -61,7 +62,6 @@ class RunCommand extends RunCommandBase {
  RunCommand({ bool verboseHelp = false }) : super(verboseHelp: verboseHelp) {
    requiresPubspecYaml();
    usesFilesystemOptions(hide: !verboseHelp);
-
    argParser
      ..addFlag('start-paused',
        negatable: false,
@ -130,10 +130,6 @@ class RunCommand extends RunCommandBase {
        hide: !verboseHelp,
        help: 'Specify a pre-built application binary to use when running.',
      )
-      ..addFlag('track-widget-creation',
-        hide: !verboseHelp,
-        help: 'Track widget creation locations.',
-      )
      ..addOption('project-root',
        hide: !verboseHelp,
        help: 'Specify the project root directory.',
--- a/packages/flutter_tools/lib/src/commands/test.dart
+++ b/packages/flutter_tools/lib/src/commands/test.dart
@ -78,12 +78,6 @@ class TestCommand extends FastFlutterCommand {
        help: 'Handle machine structured JSON command input\n'
              'and provide output and progress in machine friendly format.',
      )
-      ..addFlag('track-widget-creation',
-        negatable: false,
-        hide: !verboseHelp,
-        help: 'Track widget creation locations.\n'
-              'This enables testing of features such as the widget inspector.',
-      )
      ..addFlag('update-goldens',
        negatable: false,
        help: 'Whether matchesGoldenFile() calls within your test methods should '
@ -106,6 +100,7 @@ class TestCommand extends FastFlutterCommand {
        defaultsTo: 'tester',
        help: 'The platform to run the unit tests on. Defaults to "tester".'
      );
+    usesTrackWidgetCreation(verboseHelp: verboseHelp);
  }

  @override
--- a/packages/flutter_tools/lib/src/dart/pub.dart
+++ b/packages/flutter_tools/lib/src/dart/pub.dart
@ -119,10 +119,11 @@ Future<void> pubGet({
  }

  if (!dotPackages.existsSync())
-    throwToolExit('$directory: pub did not create .packages file');
+    throwToolExit('$directory: pub did not create .packages file.');

-  if (dotPackages.lastModifiedSync().isBefore(pubSpecYaml.lastModifiedSync()))
-    throwToolExit('$directory: pub did not update .packages file (pubspec.yaml file has a newer timestamp)');
+  if (dotPackages.lastModifiedSync().isBefore(pubSpecYaml.lastModifiedSync())) {
+    throwToolExit('$directory: pub did not update .packages file (pubspec.yaml timestamp: ${pubSpecYaml.lastModifiedSync()}; .packages timestamp: ${dotPackages.lastModifiedSync()}).');
+  }
 }

 typedef MessageFilter = String Function(String message);
--- a/packages/flutter_tools/lib/src/runner/flutter_command.dart
+++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart
@ -288,8 +288,18 @@ abstract class FlutterCommand extends Command<void> {
    argParser.addOption(
      'flavor',
      help: 'Build a custom app flavor as defined by platform-specific build setup.\n'
-        'Supports the use of product flavors in Android Gradle scripts.\n'
-        'Supports the use of custom Xcode schemes.',
+            'Supports the use of product flavors in Android Gradle scripts, and '
+            'the use of custom Xcode schemes.',
+    );
+  }
+
+  void usesTrackWidgetCreation({ bool hasEffect = true, @required bool verboseHelp }) {
+    argParser.addFlag(
+      'track-widget-creation',
+      hide: !hasEffect && !verboseHelp,
+      defaultsTo: false, // this will soon be changed to true
+      help: 'Track widget creation locations. This enables features such as the widget inspector. '
+            'This parameter is only functional in debug mode (i.e. when compiling JIT, not AOT).',
    );
  }

@ -384,7 +394,6 @@ abstract class FlutterCommand extends Command<void> {
        } finally {
          final DateTime endTime = systemClock.now();
          printTrace(userMessages.flutterElapsedTime(name, getElapsedAsMilliseconds(endTime.difference(startTime))));
-          printTrace('"flutter $name" took ${getElapsedAsMilliseconds(endTime.difference(startTime))}.');
          _sendPostUsage(commandPath, commandResult, startTime, endTime);
        }
      },
--- a/packages/flutter_tools/test/integration.shard/README.md
+++ b/packages/flutter_tools/test/integration.shard/README.md
@ -1,11 +1,17 @@
 # Integration tests

-These tests are not hermetic, and use actual Flutter SDK.
-While they don't require actual devices, they run `flutter_tester` to test
+These tests are not hermetic, and use the actual Flutter SDK. While
+they don't require actual devices, they run `flutter_tester` to test
 Dart VM and Flutter integration.

-Use this command to run:
+Use this command to run (from the `flutter_tools` directory):

 ```shell
-../../bin/cache/dart-sdk/bin/pub run test
+../../bin/cache/dart-sdk/bin/pub run test test/integration.shard
 ```
+
+These tests are expensive to run and do not give meaningful coverage
+information for the flutter tool (since they are black-box tests that
+run the tool as a subprocess, rather than being unit tests). For this
+reason, they are in a separate shard when running on continuous
+integration and are not run when calculating coverage.
--- a/packages/flutter_tools/test/integration.shard/test_driver.dart
+++ b/packages/flutter_tools/test/integration.shard/test_driver.dart
@ -152,13 +152,13 @@ abstract class FlutterTestDriver {
    if (_processPid == null)
      return -1;
    _debugPrint('Sending SIGTERM to $_processPid..');
-    Process.killPid(_processPid);
+    ProcessSignal.SIGTERM.send(_processPid);
    return _process.exitCode.timeout(quitTimeout, onTimeout: _killForcefully);
  }

  Future<int> _killForcefully() {
    _debugPrint('Sending SIGKILL to $_processPid..');
-    Process.killPid(_processPid, ProcessSignal.SIGKILL);
+    ProcessSignal.SIGKILL.send(_processPid);
    return _process.exitCode;
  }

@ -655,6 +655,7 @@ class FlutterTestTestDriver extends FlutterTestDriver {
  Future<Map<String, dynamic>> _waitForJson({
    Duration timeout = defaultTimeout,
  }) async {
+    assert(timeout != null);
    return _timeoutWithMessages<Map<String, dynamic>>(
      () => _stdout.stream.map<Map<String, dynamic>>(_parseJsonResponse)
          .firstWhere((Map<String, dynamic> output) => output != null),
--- a/packages/flutter_tools/test/integration.shard/test_utils.dart
+++ b/packages/flutter_tools/test/integration.shard/test_utils.dart
@ -51,6 +51,5 @@ Future<void> getPackages(String folder) async {
  process.stderr.transform(utf8.decoder).listen(errorOutput.write);
  final int exitCode = await process.exitCode;
  if (exitCode != 0)
-    throw Exception(
-        'flutter pub get failed: ${errorOutput.toString()}');
+    throw Exception('flutter pub get failed: $errorOutput');
 }
--- a/packages/flutter_tools/test/src/context.dart
+++ b/packages/flutter_tools/test/src/context.dart
@ -91,7 +91,6 @@ void testUsingContext(
        },
        body: () {
          final String flutterRoot = getFlutterRoot();
-
          return runZoned<Future<dynamic>>(() {
            try {
              return context.run<dynamic>(
@ -105,7 +104,6 @@ void testUsingContext(
                    // tests can override this either in the test or during setup.
                    Cache.flutterRoot ??= flutterRoot;
                  }
-
                  return await testMethod();
                },
              );
--- a/packages/flutter_tools/test/src/testbed.dart
+++ b/packages/flutter_tools/test/src/testbed.dart
@ -25,15 +25,11 @@ import 'context.dart';

 export 'package:flutter_tools/src/base/context.dart' show Generator;

-// A default value should be provided if one of the following criteria is met:
-//    - The vast majority of tests should use this provider. For example,
-//      [BufferLogger], [MemoryFileSystem].
-//    - More TBD.
+// A default value should be provided if the vast majority of tests should use
+// this provider. For example, [BufferLogger], [MemoryFileSystem].
 final Map<Type, Generator> _testbedDefaults = <Type, Generator>{
-  // Keeps tests fast by avoid actual file system.
-  FileSystem: () => MemoryFileSystem(style: platform.isWindows
-      ? FileSystemStyle.windows
-      : FileSystemStyle.posix),
+  // Keeps tests fast by avoiding the actual file system.
+  FileSystem: () => MemoryFileSystem(style: platform.isWindows ? FileSystemStyle.windows : FileSystemStyle.posix),
  Logger: () => BufferLogger(), // Allows reading logs and prevents stdout.
  OperatingSystemUtils: () => FakeOperatingSystemUtils(),
  OutputPreferences: () => OutputPreferences(showColor: false), // configures BufferLogger to avoid color codes.