Run command validation on all commands. (#12246)

This makes command validation happen as part of `verifyThenRunCommand()`,
using a newly introduced protected method (`validateCommand()`) rather than
a `commandValidator` property (that subclasses were responsible for manually
invoking).

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

@@ -32,22 +32,13 @@ class BuildCommand extends FlutterCommand {
   @override
   final String description = 'Flutter build commands.';
 
-  @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
-  }
-
   @override
   Future<Null> runCommand() async { }
 }
 
 abstract class BuildSubCommand extends FlutterCommand {
-  @override
-  @mustCallSuper
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
+  BuildSubCommand() {
+    requiresPubspecYaml();
   }
 
   @override
@@ -72,18 +63,16 @@ abstract class BuildSubCommand extends FlutterCommand {
 }
 
 class BuildCleanCommand extends FlutterCommand {
+  BuildCleanCommand() {
+    requiresPubspecYaml();
+  }
+
   @override
   final String name = 'clean';
 
   @override
   final String description = 'Delete the build/ directory.';
 
-  @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
-  }
-
   @override
   Future<Null> runCommand() async {
     final Directory buildDir = fs.directory(getBuildDirectory());

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

@@ -38,6 +38,8 @@ import 'run.dart';
 /// exit code.
 class DriveCommand extends RunCommandBase {
   DriveCommand() {
+    requiresPubspecYaml();
+
     argParser.addFlag(
       'keep-app-running',
       defaultsTo: null,
@@ -89,12 +91,6 @@ class DriveCommand extends RunCommandBase {
   // ignore: cancel_subscriptions
   StreamSubscription<String> _deviceLogSubscription;
 
-  @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
-  }
-
   @override
   Future<Null> runCommand() async {
     final String testFile = _getTestFile();

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

@@ -12,6 +12,10 @@ import '../globals.dart';
 import '../runner/flutter_command.dart';
 
 class InstallCommand extends FlutterCommand {
+  InstallCommand() {
+    requiresPubspecYaml();
+  }
+
   @override
   final String name = 'install';
 
@@ -21,12 +25,11 @@ class InstallCommand extends FlutterCommand {
   Device device;
 
   @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
+  Future<Null> validateCommand() async {
+    await super.validateCommand();
     device = await findTargetDevice();
     if (device == null)
       throwToolExit('No target device found');
-    return super.verifyThenRunCommand();
   }
 
   @override

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

@@ -26,18 +26,13 @@ class PackagesCommand extends FlutterCommand {
   @override
   final String description = 'Commands for managing Flutter packages.';
 
-  @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
-  }
-
   @override
   Future<Null> runCommand() async { }
 }
 
 class PackagesGetCommand extends FlutterCommand {
   PackagesGetCommand(this.name, this.upgrade) {
+    requiresPubspecYaml();
     argParser.addFlag('offline',
       negatable: false,
       help: 'Use cached packages instead of accessing the network.'
@@ -84,6 +79,10 @@ class PackagesGetCommand extends FlutterCommand {
 }
 
 class PackagesTestCommand extends FlutterCommand {
+  PackagesTestCommand() {
+    requiresPubspecYaml();
+  }
+
   @override
   String get name => 'test';
 
@@ -107,7 +106,9 @@ class PackagesTestCommand extends FlutterCommand {
 }
 
 class PackagesPassthroughCommand extends FlutterCommand {
-  PackagesPassthroughCommand();
+  PackagesPassthroughCommand() {
+    requiresPubspecYaml();
+  }
 
   @override
   String get name => 'pub';

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

@@ -82,6 +82,8 @@ class RunCommand extends RunCommandBase {
   final String description = 'Run your Flutter app on an attached device.';
 
   RunCommand({ bool verboseHelp: false }) {
+    requiresPubspecYaml();
+
     argParser.addFlag('full-restart',
         defaultsTo: true,
         help: 'Stop any currently running application process before running the app.');
@@ -153,13 +155,6 @@ class RunCommand extends RunCommandBase {
             'measure the startup time and the app restart time, write the\n'
             'results out to "refresh_benchmark.json", and exit. This flag is\n'
             'intended for use in generating automated flutter benchmarks.');
-
-    commandValidator = () {
-      // When running with a prebuilt application, no command validation is
-      // necessary.
-      if (!runningWithPrebuiltApplication)
-        commonCommandValidator();
-    };
   }
 
   List<Device> devices;
@@ -222,14 +217,16 @@ class RunCommand extends RunCommandBase {
   bool get stayResident => argResults['resident'];
 
   @override
-  Future<FlutterCommandResult> verifyThenRunCommand() async {
-    commandValidator();
+  Future<Null> validateCommand() async {
+    // When running with a prebuilt application, no command validation is
+    // necessary.
+    if (!runningWithPrebuiltApplication)
+      await super.validateCommand();
     devices = await findAllTargetDevices();
     if (devices == null)
       throwToolExit(null);
     if (deviceManager.hasSpecifiedAllDevices && runningWithPrebuiltApplication)
       throwToolExit('Using -d all with --use-application-binary is not supported');
-    return super.verifyThenRunCommand();
   }
 
   DebuggingOptions _createDebuggingOptions() {

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

@@ -12,6 +12,10 @@ import '../globals.dart';
 import '../runner/flutter_command.dart';
 
 class StopCommand extends FlutterCommand {
+  StopCommand() {
+    requiresPubspecYaml();
+  }
+
   @override
   final String name = 'stop';
 
@@ -21,12 +25,11 @@ class StopCommand extends FlutterCommand {
   Device device;
 
   @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
+  Future<Null> validateCommand() async {
+    await super.validateCommand();
     device = await findTargetDevice();
     if (device == null)
       throwToolExit(null);
-    return super.verifyThenRunCommand();
   }
 
   @override

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

@@ -21,6 +21,7 @@ import '../test/watcher.dart';
 
 class TestCommand extends FlutterCommand {
   TestCommand({ bool verboseHelp: false }) {
+    requiresPubspecYaml();
     usesPubOption();
     argParser.addOption('name',
       help: 'A regular expression matching substrings of the names of tests to run.',
@@ -67,15 +68,6 @@ class TestCommand extends FlutterCommand {
         negatable: false,
         help: 'Handle machine structured JSON command input\n'
             'and provide output and progress in machine friendly format.');
-    commandValidator = () {
-      if (!fs.isFileSync('pubspec.yaml')) {
-        throwToolExit(
-          'Error: No pubspec.yaml file found in the current working directory.\n'
-          'Run this command from the root of your project. Test files must be\n'
-          'called *_test.dart and must reside in the package\'s \'test\'\n'
-          'directory (or one of its subdirectories).');
-      }
-    };
   }
 
   @override
@@ -143,6 +135,18 @@ class TestCommand extends FlutterCommand {
     return true;
   }
 
+  @override
+  Future<Null> validateCommand() async {
+    await super.validateCommand();
+    if (!fs.isFileSync('pubspec.yaml')) {
+      throwToolExit(
+          'Error: No pubspec.yaml file found in the current working directory.\n'
+              'Run this command from the root of your project. Test files must be\n'
+              'called *_test.dart and must reside in the package\'s \'test\'\n'
+              'directory (or one of its subdirectories).');
+    }
+  }
+
   @override
   Future<FlutterCommandResult> runCommand() async {
     if (platform.isWindows) {
@@ -152,7 +156,6 @@ class TestCommand extends FlutterCommand {
       );
     }
 
-    commandValidator();
     final List<String> names = argResults['name'];
     final List<String> plainNames = argResults['plain-name'];
 

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

@@ -21,6 +21,7 @@ const String kFirstUsefulFrameEventName = 'Widgets completed first useful frame'
 
 class TraceCommand extends FlutterCommand {
   TraceCommand() {
+    requiresPubspecYaml();
     argParser.addFlag('start', negatable: false, help: 'Start tracing.');
     argParser.addFlag('stop', negatable: false, help: 'Stop tracing.');
     argParser.addOption('out', help: 'Specify the path of the saved trace file.');
@@ -43,12 +44,6 @@ class TraceCommand extends FlutterCommand {
     'time (controlled by --duration), and stop tracing. To explicitly control tracing, call trace\n'
     'with --start and later with --stop.';
 
-  @override
-  Future<Null> verifyThenRunCommand() async {
-    commandValidator();
-    return super.verifyThenRunCommand();
-  }
-
   @override
   Future<Null> runCommand() async {
     final int observatoryPort = int.parse(argResults['debug-port']);

packages/flutter_tools/lib/src/runner/flutter_command.dart

@@ -22,8 +22,6 @@ import '../globals.dart';
 import '../usage.dart';
 import 'flutter_command_runner.dart';
 
-typedef void Validator();
-
 enum ExitStatus {
   success,
   warning,
@@ -57,13 +55,11 @@ class FlutterCommandResult {
 }
 
 abstract class FlutterCommand extends Command<Null> {
-  FlutterCommand() {
-    commandValidator = commonCommandValidator;
-  }
-
   @override
   FlutterCommandRunner get runner => super.runner;
 
+  bool _requiresPubspecYaml = false;
+
   /// Whether this command uses the 'target' option.
   bool _usesTargetOption = false;
 
@@ -75,6 +71,10 @@ abstract class FlutterCommand extends Command<Null> {
 
   BuildMode _defaultBuildMode;
 
+  void requiresPubspecYaml() {
+    _requiresPubspecYaml = true;
+  }
+
   void usesTargetOption() {
     argParser.addOption('target',
       abbr: 't',
@@ -219,6 +219,8 @@ abstract class FlutterCommand extends Command<Null> {
   /// rather than calling [runCommand] directly.
   @mustCallSuper
   Future<FlutterCommandResult> verifyThenRunCommand() async {
+    await validateCommand();
+
     // Populate the cache. We call this before pub get below so that the sky_engine
     // package is available in the flutter cache for pub to find.
     if (shouldUpdateCache)
@@ -313,11 +315,10 @@ abstract class FlutterCommand extends Command<Null> {
     printStatus('No connected devices.');
   }
 
-  // This is a field so that you can modify the value for testing.
-  Validator commandValidator;
-
-  void commonCommandValidator() {
-    if (!PackageMap.isUsingCustomPackagesPath) {
+  @protected
+  @mustCallSuper
+  Future<Null> validateCommand() async {
+    if (_requiresPubspecYaml && !PackageMap.isUsingCustomPackagesPath) {
       // Don't expect a pubspec.yaml file if the user passed in an explicit .packages file path.
       if (!fs.isFileSync('pubspec.yaml')) {
         throw new ToolExit(
@@ -326,6 +327,7 @@ abstract class FlutterCommand extends Command<Null> {
           'Do not run this command from the root of your git clone of Flutter.'
         );
       }
+
       if (fs.isFileSync('flutter.yaml')) {
         throw new ToolExit(
           'Please merge your flutter.yaml into your pubspec.yaml.\n\n'
@@ -343,6 +345,13 @@ abstract class FlutterCommand extends Command<Null> {
           'https://github.com/flutter/flutter/blob/master/examples/flutter_gallery/pubspec.yaml\n'
         );
       }
+
+      // Validate the current package map only if we will not be running "pub get" later.
+      if (!(_usesPubOption && argResults['pub'])) {
+        final String error = new PackageMap(PackageMap.globalPackagesPath).checkValid();
+        if (error != null)
+          throw new ToolExit(error);
+      }
     }
 
     if (_usesTargetOption) {
@@ -350,13 +359,6 @@ abstract class FlutterCommand extends Command<Null> {
       if (!fs.isFileSync(targetPath))
         throw new ToolExit('Target file "$targetPath" not found.');
     }
-
-    // Validate the current package map only if we will not be running "pub get" later.
-    if (!(_usesPubOption && argResults['pub'])) {
-      final String error = new PackageMap(PackageMap.globalPackagesPath).checkValid();
-      if (error != null)
-        throw new ToolExit(error);
-    }
   }
 
   ApplicationPackageStore applicationPackages;

packages/flutter_tools/test/commands/drive_test.dart

@@ -76,10 +76,11 @@ void main() {
 
       final String testApp = fs.path.join(cwd.path, 'test', 'e2e.dart');
       final String testFile = fs.path.join(cwd.path, 'test_driver', 'e2e_test.dart');
+      fs.file(testApp).createSync(recursive: true);
 
       final List<String> args = <String>[
         'drive',
-        '--target=$testApp}',
+        '--target=$testApp',
       ];
       try {
         await createTestCommandRunner(command).run(args);
@@ -120,6 +121,7 @@ void main() {
 
     testUsingContext('returns 1 when app file is outside package', () async {
       final String appFile = fs.path.join(cwd.dirname, 'other_app', 'app.dart');
+      fs.file(appFile).createSync(recursive: true);
       final List<String> args = <String>[
         'drive',
         '--target=$appFile',
@@ -139,6 +141,7 @@ void main() {
 
     testUsingContext('returns 1 when app file is in the root dir', () async {
       final String appFile = fs.path.join(cwd.path, 'main.dart');
+      fs.file(appFile).createSync(recursive: true);
       final List<String> args = <String>[
         'drive',
         '--target=$appFile',

packages/flutter_tools/test/src/mocks.dart

@@ -103,8 +103,7 @@ class MockDeviceLogReader extends DeviceLogReader {
 
 void applyMocksToCommand(FlutterCommand command) {
   command
-    ..applicationPackages = new MockApplicationPackageStore()
-    ..commandValidator = () => true;
+    ..applicationPackages = new MockApplicationPackageStore();
 }
 
 /// Common functionality for tracking mock interaction