Refactor and polish the 'felt' tool

.cirrus.yml

@@ -41,7 +41,9 @@ task:
         $ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/pub get
         cd $ENGINE_PATH/src/flutter/lib/web_ui
         $ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/pub get
-        CHROME_NO_SANDBOX=true $ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/dart dev/felt.dart
+        export FELT="$ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/dart dev/felt.dart"
+        $FELT check-licenses
+        CHROME_NO_SANDBOX=true $FELT test
       fetch_framework_script: |
         mkdir -p $FRAMEWORK_PATH
         cd $FRAMEWORK_PATH

lib/web_ui/dev/chrome_installer.dart

@@ -4,19 +4,30 @@
 
 import 'dart:io' as io;
 
+import 'package:args/args.dart';
 import 'package:http/http.dart';
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 
 import 'environment.dart';
 
-void main(List<String> args) async {
-  Environment.commandLineArguments = args;
-  try {
-    await getOrInstallChrome();
-  } on ChromeInstallerException catch (error) {
-    io.stderr.writeln(error.toString());
-  }
+void addChromeVersionOption(ArgParser argParser) {
+  final String pinnedChromeVersion =
+      io.File(path.join(environment.webUiRootDir.path, 'dev', 'chrome.lock'))
+          .readAsStringSync()
+          .trim();
+
+  argParser
+    ..addOption(
+      'chrome-version',
+      defaultsTo: pinnedChromeVersion,
+      help: 'The Chrome version to use while running tests. If the requested '
+          'version has not been installed, it will be downloaded and installed '
+          'automatically. A specific Chrome build version number, such as 695653 '
+          'this use that version of Chrome. Value "latest" will use the latest '
+          'available build of Chrome, installing it if necessary. Value "system" '
+          'will use the manually installed version of Chrome on this computer.',
+    );
 }
 
 /// Returns the installation of Chrome, installing it if necessary.
@@ -31,8 +42,10 @@ void main(List<String> args) async {
 /// exact build nuber, such as 695653. Build numbers can be found here:
 ///
 /// https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Linux_x64/
-Future<ChromeInstallation> getOrInstallChrome({String requestedVersion, StringSink infoLog}) async {
-  requestedVersion ??= environment.chromeVersion;
+Future<ChromeInstallation> getOrInstallChrome(
+  String requestedVersion, {
+  StringSink infoLog,
+}) async {
   infoLog ??= io.stdout;
 
   if (requestedVersion == 'system') {

lib/web_ui/dev/environment.dart

@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 import 'dart:io' as io;
-import 'package:args/args.dart' as args;
 import 'package:path/path.dart' as pathlib;
 
 /// Contains various environment variables, such as common file paths and command-line options.
@@ -13,51 +12,9 @@ Environment get environment {
 }
 Environment _environment;
 
-args.ArgParser get _argParser {
-  return args.ArgParser()
-    ..addMultiOption(
-      'target',
-      abbr: 't',
-      help: 'The path to the target to run. When omitted, runs all targets.',
-    )
-    ..addMultiOption(
-      'shard',
-      abbr: 's',
-      help: 'The category of tasks to run.',
-    )
-    ..addFlag(
-      'debug',
-      help: 'Pauses the browser before running a test, giving you an '
-        'opportunity to add breakpoints or inspect loaded code before '
-        'running the code.',
-    )
-    ..addOption(
-      'chrome-version',
-      help: 'The Chrome version to use while running tests. If the requested '
-        'version has not been installed, it will be downloaded and installed '
-        'automatically. A specific Chrome build version number, such as 695653 '
-        'this use that version of Chrome. Value "latest" will use the latest '
-        'available build of Chrome, installing it if necessary. Value "system" '
-        'will use the manually installed version of Chrome on this computer.',
-    );
-}
-
 /// Contains various environment variables, such as common file paths and command-line options.
 class Environment {
-  /// Command-line arguments.
-  static List<String> commandLineArguments;
-
   factory Environment() {
-    if (commandLineArguments == null) {
-      io.stderr.writeln('Command-line arguments not set.');
-      io.exit(1);
-    }
-
-    final args.ArgResults options = _argParser.parse(commandLineArguments);
-    final List<String> shards = options['shard'];
-    final bool isDebug = options['debug'];
-    final List<String> targets = options['target'];
-
     final io.File self = io.File.fromUri(io.Platform.script);
     final io.Directory engineSrcDir = self.parent.parent.parent.parent.parent;
     final io.Directory outDir = io.Directory(pathlib.join(engineSrcDir.path, 'out'));
@@ -72,20 +29,13 @@ class Environment {
       }
     }
 
-    final String pinnedChromeVersion = io.File(pathlib.join(webUiRootDir.path, 'dev', 'chrome.lock')).readAsStringSync().trim();
-    final String chromeVersion = options['chrome-version'] ?? pinnedChromeVersion;
-
     return Environment._(
       self: self,
       webUiRootDir: webUiRootDir,
       engineSrcDir: engineSrcDir,
       outDir: outDir,
       hostDebugUnoptDir: hostDebugUnoptDir,
       dartSdkDir: dartSdkDir,
-      requestedShards: shards,
-      isDebug: isDebug,
-      targets: targets,
-      chromeVersion: chromeVersion,
     );
   }
 
@@ -96,10 +46,6 @@ class Environment {
     this.outDir,
     this.hostDebugUnoptDir,
     this.dartSdkDir,
-    this.requestedShards,
-    this.isDebug,
-    this.targets,
-    this.chromeVersion,
   });
 
   /// The Dart script that's currently running.
@@ -122,33 +68,6 @@ class Environment {
   /// The root of the Dart SDK.
   final io.Directory dartSdkDir;
 
-  /// Shards specified on the command-line.
-  final List<String> requestedShards;
-
-  /// Whether to start the browser in debug mode.
-  ///
-  /// In this mode the browser pauses before running the test to allow
-  /// you set breakpoints or inspect the code.
-  final bool isDebug;
-
-  /// Paths to targets to run, e.g. a single test.
-  final List<String> targets;
-
-  /// The Chrome version used for testing.
-  ///
-  /// The value must be one of:
-  ///
-  /// - "system", which indicates the Chrome installed on the local machine.
-  /// - "latest", which indicates the latest available Chrome build specified by:
-  ///   https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/Linux_x64%2FLAST_CHANGE?alt=media
-  /// - A build number pointing at a pre-built version of Chrome available at:
-  ///   https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Linux_x64/
-  ///
-  /// The "system" Chrome is assumed to be already properly installed and will be invoked directly.
-  ///
-  /// The "latest" or a specific build number will be downloaded and cached in [webUiDartToolDir].
-  final String chromeVersion;
-
   /// The "dart" executable file.
   String get dartExecutable => pathlib.join(dartSdkDir.path, 'bin', 'dart');
 

lib/web_ui/dev/felt

@@ -1,4 +1,5 @@
 #!/bin/bash
+set -e
 
 # felt: a command-line utility for building and testing Flutter web engine.
 #       It stands for Flutter Engine Local Tester.
@@ -44,4 +45,4 @@ then
   ninja -C $HOST_DEBUG_UNOPT_DIR
 fi
 
-(cd $WEB_UI_DIR && $DART_SDK_DIR/bin/dart dev/felt.dart $@)
+$DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@

lib/web_ui/dev/felt.dart

@@ -4,119 +4,38 @@
 
 import 'dart:io' as io;
 
-import 'package:path/path.dart' as pathlib;
+import 'package:args/command_runner.dart';
 
-import 'environment.dart';
+import 'licenses.dart';
 import 'test_runner.dart';
 
-// A "shard" is a named subset of tasks this script runs. If not specified,
-// it runs all shards. That's what we do on CI.
-const Map<String, Function> _kShardNameToCode = <String, Function>{
-  'licenses': _checkLicenseHeaders,
-  'tests': runTests,
-};
+CommandRunner runner = CommandRunner<bool>(
+  'felt',
+  'Command-line utility for building and testing Flutter web engine.',
+)
+  ..addCommand(LicensesCommand())
+  ..addCommand(TestsCommand());
 
 void main(List<String> args) async {
-  Environment.commandLineArguments = args;
-  if (io.Directory.current.absolute.path != environment.webUiRootDir.absolute.path) {
-    io.stderr.writeln('Current directory is not the root of the web_ui package directory.');
-    io.stderr.writeln('web_ui directory is: ${environment.webUiRootDir.absolute.path}');
-    io.stderr.writeln('current directory is: ${io.Directory.current.absolute.path}');
-    io.exit(1);
+  if (args.isEmpty) {
+    // The felt tool was invoked with no arguments. Print usage.
+    runner.printUsage();
+    io.exit(64); // Exit code 64 indicates a usage error.
   }
 
-  _copyAhemFontIntoWebUi();
-
-  final List<String> shardsToRun = environment.requestedShards.isNotEmpty
-    ? environment.requestedShards
-    : _kShardNameToCode.keys.toList();
-
-  for (String shard in shardsToRun) {
-    print('Running shard $shard');
-    if (!_kShardNameToCode.containsKey(shard)) {
-      io.stderr.writeln('''
-ERROR:
-  Unsupported test shard: $shard.
-  Supported test shards: ${_kShardNameToCode.keys.join(', ')}
-TESTS FAILED
-'''.trim());
+  try {
+    final bool result = await runner.run(args);
+    if (result == false) {
+      print('Sub-command returned false: `${args.join(' ')}`');
       io.exit(1);
     }
-    await _kShardNameToCode[shard]();
+  } on UsageException catch (e) {
+    print(e);
+    io.exit(64); // Exit code 64 indicates a usage error.
+  } catch (e) {
+    rethrow;
   }
+
   // Sometimes the Dart VM refuses to quit.
   io.exit(io.exitCode);
 }
-
-void _checkLicenseHeaders() {
-  final List<io.File> allSourceFiles = _flatListSourceFiles(environment.webUiRootDir);
-  _expect(allSourceFiles.isNotEmpty, 'Dart source listing of ${environment.webUiRootDir.path} must not be empty.');
-
-  final List<String> allDartPaths = allSourceFiles.map((f) => f.path).toList();
-
-  for (String expectedDirectory in const <String>['lib', 'test', 'dev', 'tool']) {
-    final String expectedAbsoluteDirectory = pathlib.join(environment.webUiRootDir.path, expectedDirectory);
-    _expect(
-      allDartPaths.where((p) => p.startsWith(expectedAbsoluteDirectory)).isNotEmpty,
-      'Must include the $expectedDirectory/ directory',
-    );
-  }
-
-  allSourceFiles.forEach(_expectLicenseHeader);
-  print('License headers OK!');
-}
-
-final _copyRegex = RegExp(r'// Copyright 2013 The Flutter Authors\. All rights reserved\.');
-
-void _expectLicenseHeader(io.File file) {
-  List<String> head = file.readAsStringSync().split('\n').take(3).toList();
-
-  _expect(head.length >= 3, 'File too short: ${file.path}');
-  _expect(
-    _copyRegex.firstMatch(head[0]) != null,
-    'Invalid first line of license header in file ${file.path}',
-  );
-  _expect(
-    head[1] == '// Use of this source code is governed by a BSD-style license that can be',
-    'Invalid second line of license header in file ${file.path}',
-  );
-  _expect(
-    head[2] == '// found in the LICENSE file.',
-    'Invalid second line of license header in file ${file.path}',
-  );
-}
-
-void _expect(bool value, String requirement) {
-  if (!value) {
-    throw Exception('Test failed: ${requirement}');
-  }
-}
-
-List<io.File> _flatListSourceFiles(io.Directory directory) {
-  return directory
-      .listSync(recursive: true)
-      .whereType<io.File>()
-      .where((f) {
-        if (!f.path.endsWith('.dart') && !f.path.endsWith('.js')) {
-          // Not a source file we're checking.
-          return false;
-        }
-        if (pathlib.isWithin(environment.webUiBuildDir.path, f.path) ||
-            pathlib.isWithin(environment.webUiDartToolDir.path, f.path)) {
-          // Generated files.
-          return false;
-        }
-        return true;
-      })
-      .toList();
-}
-
-void _copyAhemFontIntoWebUi() {
-  final io.File sourceAhemTtf = io.File(pathlib.join(
-    environment.flutterDirectory.path, 'third_party', 'txt', 'third_party', 'fonts', 'ahem.ttf'
-  ));
-  final String destinationAhemTtfPath = pathlib.join(
-    environment.webUiRootDir.path, 'lib', 'assets', 'ahem.ttf'
-  );
-  sourceAhemTtf.copySync(destinationAhemTtfPath);
-}