From fc092d23d703b5e89da046b1f9968e46ee3c4f63 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 12 Sep 2019 13:50:34 -0700 Subject: [PATCH 1/5] Refactor and polish the 'felt' tool --- lib/web_ui/dev/chrome_installer.dart | 62 ++++- lib/web_ui/dev/environment.dart | 81 ------- lib/web_ui/dev/felt | 2 +- lib/web_ui/dev/felt.dart | 121 ++-------- lib/web_ui/dev/licenses.dart | 95 ++++++++ lib/web_ui/dev/test_platform.dart | 5 +- lib/web_ui/dev/test_runner.dart | 339 ++++++++++++++++----------- lib/web_ui/dev/utils.dart | 61 +++++ 8 files changed, 429 insertions(+), 337 deletions(-) create mode 100644 lib/web_ui/dev/licenses.dart create mode 100644 lib/web_ui/dev/utils.dart diff --git a/lib/web_ui/dev/chrome_installer.dart b/lib/web_ui/dev/chrome_installer.dart index f201d2af5ad8f..377b16f0946b2 100644 --- a/lib/web_ui/dev/chrome_installer.dart +++ b/lib/web_ui/dev/chrome_installer.dart @@ -4,19 +4,63 @@ import 'dart:io' as io; +import 'package:args/args.dart'; +import 'package:args/command_runner.dart'; import 'package:http/http.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; import 'environment.dart'; -void main(List args) async { - Environment.commandLineArguments = args; - try { - await getOrInstallChrome(); - } on ChromeInstallerException catch (error) { - io.stderr.writeln(error.toString()); +class ChromeInstallerCommand extends Command { + ChromeInstallerCommand() { + addChromeVersionOption(argParser); } + + @override + String get name => 'chrome-installer'; + + @override + String get description => 'Installs the desired version of Chrome.'; + + /// 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]. + String get chromeVersion => argResults['chrome-version']; + + @override + Future run() { + return getOrInstallChrome(chromeVersion); + } +} + +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 +75,10 @@ void main(List 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 getOrInstallChrome({String requestedVersion, StringSink infoLog}) async { - requestedVersion ??= environment.chromeVersion; +Future getOrInstallChrome( + String requestedVersion, { + StringSink infoLog, +}) async { infoLog ??= io.stdout; if (requestedVersion == 'system') { diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 1556f09417ed4..c1b85847c75d4 100644 --- a/lib/web_ui/dev/environment.dart +++ b/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 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 shards = options['shard']; - final bool isDebug = options['debug']; - final List 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,9 +29,6 @@ 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, @@ -82,10 +36,6 @@ class Environment { 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 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 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'); diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index f93a32a169a98..ac7c28ad5b258 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -44,4 +44,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" $@) diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index 5654e28a7d922..2ca08023ffc14 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -4,119 +4,28 @@ 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 _kShardNameToCode = { - 'licenses': _checkLicenseHeaders, - 'tests': runTests, -}; +CommandRunner runner = CommandRunner( + 'felt', + 'Command-line utility for building and testing Flutter web engine.', +) + ..addCommand(LicensesCommand()) + ..addCommand(TestsCommand()); void main(List 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); + try { + await runner.run(args); + } on UsageException catch (e) { + print(e); + io.exit(64); // Exit code 64 indicates a usage error. + } catch (e) { + rethrow; } - _copyAhemFontIntoWebUi(); - - final List 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()); - io.exit(1); - } - await _kShardNameToCode[shard](); - } // Sometimes the Dart VM refuses to quit. io.exit(io.exitCode); } - -void _checkLicenseHeaders() { - final List allSourceFiles = _flatListSourceFiles(environment.webUiRootDir); - _expect(allSourceFiles.isNotEmpty, 'Dart source listing of ${environment.webUiRootDir.path} must not be empty.'); - - final List allDartPaths = allSourceFiles.map((f) => f.path).toList(); - - for (String expectedDirectory in const ['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 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 _flatListSourceFiles(io.Directory directory) { - return directory - .listSync(recursive: true) - .whereType() - .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); -} diff --git a/lib/web_ui/dev/licenses.dart b/lib/web_ui/dev/licenses.dart new file mode 100644 index 0000000000000..e6b49162bc124 --- /dev/null +++ b/lib/web_ui/dev/licenses.dart @@ -0,0 +1,95 @@ +// Copyright 2013 The Flutter 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 io; + +import 'package:args/command_runner.dart'; +import 'package:path/path.dart' as path; + +import 'environment.dart'; + +class LicensesCommand extends Command { + @override + final String name = 'licenses'; + + @override + final String description = 'Check license headers.'; + + @override + void run() { + _checkLicenseHeaders(); + } + + void _checkLicenseHeaders() { + final List allSourceFiles = + _flatListSourceFiles(environment.webUiRootDir); + _expect(allSourceFiles.isNotEmpty, + 'Dart source listing of ${environment.webUiRootDir.path} must not be empty.'); + + final List allDartPaths = + allSourceFiles.map((f) => f.path).toList(); + + for (String expectedDirectory in const [ + 'lib', + 'test', + 'dev', + 'tool' + ]) { + final String expectedAbsoluteDirectory = + path.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 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 _flatListSourceFiles(io.Directory directory) { + return directory.listSync(recursive: true).whereType().where((f) { + if (!f.path.endsWith('.dart') && !f.path.endsWith('.js')) { + // Not a source file we're checking. + return false; + } + if (path.isWithin(environment.webUiBuildDir.path, f.path) || + path.isWithin(environment.webUiDartToolDir.path, f.path)) { + // Generated files. + return false; + } + return true; + }).toList(); + } +} diff --git a/lib/web_ui/dev/test_platform.dart b/lib/web_ui/dev/test_platform.dart index 3510c5159555e..f98d3f804803b 100644 --- a/lib/web_ui/dev/test_platform.dart +++ b/lib/web_ui/dev/test_platform.dart @@ -850,12 +850,15 @@ class Chrome extends Browser { @override final Future remoteDebuggerUrl; + static String version; + /// Starts a new instance of Chrome open to the given [url], which may be a /// [Uri] or a [String]. factory Chrome(Uri url, {bool debug = false}) { + assert(version != null); var remoteDebuggerCompleter = Completer.sync(); return Chrome._(() async { - final ChromeInstallation installation = await getOrInstallChrome(infoLog: _DevNull()); + final ChromeInstallation installation = await getOrInstallChrome(version, infoLog: _DevNull()); final bool isChromeNoSandbox = Platform.environment['CHROME_NO_SANDBOX'] == 'true'; var dir = createTempDir(); diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 64363cd8b04cc..8372c6640bf45 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -5,176 +5,235 @@ import 'dart:async'; import 'dart:io' as io; +import 'package:args/command_runner.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; -import 'package:test_core/src/runner/hack_register_platform.dart' as hack; // ignore: implementation_imports +import 'package:test_core/src/runner/hack_register_platform.dart' + as hack; // ignore: implementation_imports import 'package:test_api/src/backend/runtime.dart'; // ignore: implementation_imports -import 'package:test_core/src/executable.dart' as test; // ignore: implementation_imports +import 'package:test_core/src/executable.dart' + as test; // ignore: implementation_imports +import 'chrome_installer.dart'; import 'test_platform.dart'; import 'environment.dart'; +import 'utils.dart'; + +class TestsCommand extends Command { + TestsCommand() { + argParser + ..addMultiOption( + 'target', + abbr: 't', + help: 'The path to the target to run. When omitted, runs all targets.', + ) + ..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.', + ); -Future runTests() async { - await _buildHostPage(); - await _buildTests(); - - if (environment.targets.isEmpty) { - await _runAllTests(); - } else { - await _runSingleTest(environment.targets); + addChromeVersionOption(argParser); } -} -Future _runSingleTest(List targets) async { - await _runTestBatch(targets, concurrency: 1, expectFailure: false); - _checkExitCode(); -} + @override + final String name = 'tests'; -Future _runAllTests() async { - final io.Directory testDir = io.Directory(path.join( - environment.webUiRootDir.path, - 'test', - )); - - // Separate screenshot tests from unit-tests. Screenshot tests must run - // one at a time. Otherwise, they will end up screenshotting each other. - // This is not an issue for unit-tests. - const String failureSmokeTestPath = 'test/golden_tests/golden_failure_smoke_test.dart'; - final List screenshotTestFiles = []; - final List unitTestFiles = []; - - for (io.File testFile in testDir.listSync(recursive: true).whereType()) { - final String testFilePath = path.relative(testFile.path, from: environment.webUiRootDir.path); - if (!testFilePath.endsWith('_test.dart')) { - // Not a test file at all. Skip. - continue; - } - if (testFilePath.endsWith(failureSmokeTestPath)) { - // A smoke test that fails on purpose. Skip. - continue; - } - if (path.split(testFilePath).contains('golden_tests')) { - screenshotTestFiles.add(testFilePath); + @override + final String description = 'Run tests.'; + + @override + Future run() async { + Chrome.version = chromeVersion; + + _copyAhemFontIntoWebUi(); + await _buildHostPage(); + await _buildTests(); + + final List targets = + this.targets.map((t) => FilePath.fromCwd(t)).toList(); + if (targets.isEmpty) { + return _runAllTests(); } else { - unitTestFiles.add(testFilePath); + return _runTargetTests(targets); } } - // This test returns a non-zero exit code on purpose. Run it separately. - if (io.Platform.environment['CIRRUS_CI'] != 'true') { - await _runTestBatch( - [failureSmokeTestPath], - concurrency: 1, - expectFailure: true, - ); + /// 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. + bool get isDebug => argResults['debug']; + + /// Paths to targets to run, e.g. a single test. + List get targets => argResults['target']; + + /// See [ChromeInstallerCommand.chromeVersion]. + String get chromeVersion => argResults['chrome-version']; + + Future _runTargetTests(List targets) async { + await _runTestBatch(targets, concurrency: 1, expectFailure: false); _checkExitCode(); } - // Run all unit-tests as a single batch. - await _runTestBatch(unitTestFiles, concurrency: 10, expectFailure: false); - _checkExitCode(); + Future _runAllTests() async { + final io.Directory testDir = io.Directory(path.join( + environment.webUiRootDir.path, + 'test', + )); + + // Separate screenshot tests from unit-tests. Screenshot tests must run + // one at a time. Otherwise, they will end up screenshotting each other. + // This is not an issue for unit-tests. + final FilePath failureSmokeTestPath = FilePath.fromWebUi( + 'test/golden_tests/golden_failure_smoke_test.dart', + ); + final List screenshotTestFiles = []; + final List unitTestFiles = []; + + for (io.File testFile + in testDir.listSync(recursive: true).whereType()) { + final FilePath testFilePath = FilePath.fromCwd(testFile.path); + if (!testFilePath.absolute.endsWith('_test.dart')) { + // Not a test file at all. Skip. + continue; + } + if (testFilePath == failureSmokeTestPath) { + // A smoke test that fails on purpose. Skip. + continue; + } + if (path.split(testFilePath.relativeToWebUi).contains('golden_tests')) { + screenshotTestFiles.add(testFilePath); + } else { + unitTestFiles.add(testFilePath); + } + } + + // This test returns a non-zero exit code on purpose. Run it separately. + if (io.Platform.environment['CIRRUS_CI'] != 'true') { + await _runTestBatch( + [failureSmokeTestPath], + concurrency: 1, + expectFailure: true, + ); + _checkExitCode(); + } - // Run screenshot tests one at a time. - for (String testFilePath in screenshotTestFiles) { - await _runTestBatch([testFilePath], concurrency: 1, expectFailure: false); + // Run all unit-tests as a single batch. + await _runTestBatch(unitTestFiles, concurrency: 10, expectFailure: false); _checkExitCode(); + + // Run screenshot tests one at a time. + for (FilePath testFilePath in screenshotTestFiles) { + await _runTestBatch( + [testFilePath], + concurrency: 1, + expectFailure: false, + ); + _checkExitCode(); + } } -} -void _checkExitCode() { - if (io.exitCode != 0) { - io.stderr.writeln('Process exited with exit code ${io.exitCode}.'); - io.exit(1); + void _checkExitCode() { + if (io.exitCode != 0) { + io.stderr.writeln('Process exited with exit code ${io.exitCode}.'); + io.exit(1); + } } -} -// TODO(yjbanov): skip rebuild if host.dart hasn't changed. -Future _buildHostPage() async { - final io.Process pubRunTest = await io.Process.start( - environment.dart2jsExecutable, - [ - 'lib/static/host.dart', - '-o', - 'lib/static/host.dart.js', - ], - workingDirectory: environment.goldenTesterRootDir.path, - ); - - final StreamSubscription stdoutSub = pubRunTest.stdout.listen(io.stdout.add); - final StreamSubscription stderrSub = pubRunTest.stderr.listen(io.stderr.add); - final int exitCode = await pubRunTest.exitCode; - stdoutSub.cancel(); - stderrSub.cancel(); - - if (exitCode != 0) { - io.stderr.writeln('Failed to compile tests. Compiler exited with exit code $exitCode'); - io.exit(1); + // TODO(yjbanov): skip rebuild if host.dart hasn't changed. + Future _buildHostPage() async { + final int exitCode = await runProcess( + environment.dart2jsExecutable, + [ + 'lib/static/host.dart', + '-o', + 'lib/static/host.dart.js', + ], + workingDirectory: environment.goldenTesterRootDir.path, + ); + + if (exitCode != 0) { + io.stderr.writeln( + 'Failed to compile tests. Compiler exited with exit code $exitCode'); + io.exit(1); + } } -} -Future _buildTests() async { - // TODO(yjbanov): learn to build only requested tests: https://github.com/flutter/flutter/issues/37810 - final io.Process pubRunTest = await io.Process.start( - environment.pubExecutable, - [ - 'run', - 'build_runner', - 'build', - 'test', - '-o', - 'build', - ], - ); - - final StreamSubscription stdoutSub = pubRunTest.stdout.listen(io.stdout.add); - final StreamSubscription stderrSub = pubRunTest.stderr.listen(io.stderr.add); - final int exitCode = await pubRunTest.exitCode; - stdoutSub.cancel(); - stderrSub.cancel(); - - if (exitCode != 0) { - io.stderr.writeln('Failed to compile tests. Compiler exited with exit code $exitCode'); - io.exit(1); + Future _buildTests() async { + // TODO(yjbanov): learn to build only requested tests: https://github.com/flutter/flutter/issues/37810 + final int exitCode = await runProcess( + environment.pubExecutable, + [ + 'run', + 'build_runner', + 'build', + 'test', + '-o', + 'build', + ], + workingDirectory: environment.webUiRootDir.path, + ); + + if (exitCode != 0) { + io.stderr.writeln( + 'Failed to compile tests. Compiler exited with exit code $exitCode'); + io.exit(1); + } } -} -/// Runs a batch of tests. -/// -/// Unless [expectFailure] is set to false, sets [io.exitCode] to a non-zero value if any tests fail. -Future _runTestBatch( - List testFiles, { + /// Runs a batch of tests. + /// + /// Unless [expectFailure] is set to false, sets [io.exitCode] to a non-zero value if any tests fail. + Future _runTestBatch( + List testFiles, { @required int concurrency, @required bool expectFailure, - } -) async { - final List testArgs = [ - '--no-color', - ...['-r', 'compact'], - '--concurrency=$concurrency', - if (environment.isDebug) - '--pause-after-load', - '--platform=chrome', - '--precompiled=${environment.webUiRootDir.path}/build', - '--', - ...testFiles, - ]; - hack.registerPlatformPlugin( - [Runtime.chrome], - () { + }) async { + final List testArgs = [ + '--no-color', + ...['-r', 'compact'], + '--concurrency=$concurrency', + if (isDebug) '--pause-after-load', + '--platform=chrome', + '--precompiled=${environment.webUiRootDir.path}/build', + '--', + ...testFiles.map((f) => f.relativeToWebUi).toList(), + ]; + hack.registerPlatformPlugin([Runtime.chrome], () { return BrowserPlatform.start(root: io.Directory.current.path); - } - ); - await test.main(testArgs); - - if (expectFailure) { - if (io.exitCode != 0) { - // It failed, as expected. - io.exitCode = 0; - } else { - io.stderr.writeln( - 'Tests ${testFiles.join(', ')} did not fail. Expected failure.', - ); - io.exitCode = 1; + }); + + // We want to run tests with `web_ui` as a working directory. + final dynamic backupCwd = io.Directory.current; + io.Directory.current = environment.webUiRootDir.path; + await test.main(testArgs); + io.Directory.current = backupCwd; + + if (expectFailure) { + if (io.exitCode != 0) { + // It failed, as expected. + io.exitCode = 0; + } else { + io.stderr.writeln( + 'Tests ${testFiles.join(', ')} did not fail. Expected failure.', + ); + io.exitCode = 1; + } } } } + +void _copyAhemFontIntoWebUi() { + final io.File sourceAhemTtf = io.File(path.join( + environment.flutterDirectory.path, + 'third_party', + 'txt', + 'third_party', + 'fonts', + 'ahem.ttf')); + final String destinationAhemTtfPath = + path.join(environment.webUiRootDir.path, 'lib', 'assets', 'ahem.ttf'); + sourceAhemTtf.copySync(destinationAhemTtfPath); +} diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart new file mode 100644 index 0000000000000..19181bf9309a7 --- /dev/null +++ b/lib/web_ui/dev/utils.dart @@ -0,0 +1,61 @@ +// Copyright 2013 The Flutter 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:async'; +import 'dart:io' as io; + +import 'package:path/path.dart' as path; + +import 'environment.dart'; + +class FilePath { + FilePath.fromCwd(String filePath) : _path = path.absolute(filePath); + FilePath.fromWebUi(String filePath) + : _path = path.join(environment.webUiRootDir.path, filePath); + + final String _path; + + String get relativeToCwd => path.relative(_path); + String get relativeToWebUi => + path.relative(_path, from: environment.webUiRootDir.path); + + String get absolute => _path; + + @override + bool operator ==(dynamic other) { + if (other is String) { + return _path == other; + } + if (other is FilePath) { + return _path == other._path; + } + return false; + } + + @override + String toString() => _path; +} + +Future runProcess( + String executable, + List arguments, { + String workingDirectory, +}) async { + final io.Process process = await io.Process.start( + executable, + arguments, + workingDirectory: workingDirectory, + ); + return _forwardIOAndWait(process); +} + +Future _forwardIOAndWait(io.Process process) { + final StreamSubscription stdoutSub = process.stdout.listen(io.stdout.add); + final StreamSubscription stderrSub = process.stderr.listen(io.stderr.add); + return process.exitCode.then((int exitCode) { + stdoutSub.cancel(); + stderrSub.cancel(); + return exitCode; + }); +} From e78aecd79657c93286bbefa10e9c0fafcbfe155f Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 13 Sep 2019 10:20:33 -0700 Subject: [PATCH 2/5] Fix a few things caught by Yegor --- .cirrus.yml | 4 +++- lib/web_ui/dev/felt.dart | 11 +++++++++-- lib/web_ui/dev/licenses.dart | 5 +++-- lib/web_ui/dev/test_runner.dart | 9 +++++---- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index e7c83969c081d..d69bc0f53ee5d 100644 --- a/.cirrus.yml +++ b/.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 licenses + CHROME_NO_SANDBOX=true $FELT tests fetch_framework_script: | mkdir -p $FRAMEWORK_PATH cd $FRAMEWORK_PATH diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index 2ca08023ffc14..6b32e32681270 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -9,7 +9,7 @@ import 'package:args/command_runner.dart'; import 'licenses.dart'; import 'test_runner.dart'; -CommandRunner runner = CommandRunner( +CommandRunner runner = CommandRunner( 'felt', 'Command-line utility for building and testing Flutter web engine.', ) @@ -18,7 +18,14 @@ CommandRunner runner = CommandRunner( void main(List args) async { try { - await runner.run(args); + final bool result = await runner.run(args); + if (result == null) { + runner.printUsage(); + io.exit(64); + } else if (!result) { + print('Command returned false: `felt ${args.join(' ')}`'); + io.exit(1); + } } on UsageException catch (e) { print(e); io.exit(64); // Exit code 64 indicates a usage error. diff --git a/lib/web_ui/dev/licenses.dart b/lib/web_ui/dev/licenses.dart index e6b49162bc124..e226471d96859 100644 --- a/lib/web_ui/dev/licenses.dart +++ b/lib/web_ui/dev/licenses.dart @@ -9,7 +9,7 @@ import 'package:path/path.dart' as path; import 'environment.dart'; -class LicensesCommand extends Command { +class LicensesCommand extends Command { @override final String name = 'licenses'; @@ -17,8 +17,9 @@ class LicensesCommand extends Command { final String description = 'Check license headers.'; @override - void run() { + bool run() { _checkLicenseHeaders(); + return false; } void _checkLicenseHeaders() { diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 8372c6640bf45..181164f5d179b 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -19,7 +19,7 @@ import 'test_platform.dart'; import 'environment.dart'; import 'utils.dart'; -class TestsCommand extends Command { +class TestsCommand extends Command { TestsCommand() { argParser ..addMultiOption( @@ -44,7 +44,7 @@ class TestsCommand extends Command { final String description = 'Run tests.'; @override - Future run() async { + Future run() async { Chrome.version = chromeVersion; _copyAhemFontIntoWebUi(); @@ -54,10 +54,11 @@ class TestsCommand extends Command { final List targets = this.targets.map((t) => FilePath.fromCwd(t)).toList(); if (targets.isEmpty) { - return _runAllTests(); + await _runAllTests(); } else { - return _runTargetTests(targets); + await _runTargetTests(targets); } + return true; } /// Whether to start the browser in debug mode. From 36ab07ddc5931409a3fd7a573fddc739860e2a95 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 13 Sep 2019 13:18:59 -0700 Subject: [PATCH 3/5] Few minor changes --- lib/web_ui/dev/chrome_installer.dart | 33 ------------------- lib/web_ui/dev/felt | 3 +- lib/web_ui/dev/felt.dart | 2 +- lib/web_ui/dev/licenses.dart | 4 +-- lib/web_ui/dev/test_runner.dart | 2 +- lib/web_ui/dev/utils.dart | 26 ++++++--------- .../lib/src/engine/surface/surface.dart | 13 +++++--- 7 files changed, 24 insertions(+), 59 deletions(-) diff --git a/lib/web_ui/dev/chrome_installer.dart b/lib/web_ui/dev/chrome_installer.dart index 377b16f0946b2..39abff2e3ea0c 100644 --- a/lib/web_ui/dev/chrome_installer.dart +++ b/lib/web_ui/dev/chrome_installer.dart @@ -5,45 +5,12 @@ import 'dart:io' as io; import 'package:args/args.dart'; -import 'package:args/command_runner.dart'; import 'package:http/http.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; import 'environment.dart'; -class ChromeInstallerCommand extends Command { - ChromeInstallerCommand() { - addChromeVersionOption(argParser); - } - - @override - String get name => 'chrome-installer'; - - @override - String get description => 'Installs the desired version of Chrome.'; - - /// 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]. - String get chromeVersion => argResults['chrome-version']; - - @override - Future run() { - return getOrInstallChrome(chromeVersion); - } -} - void addChromeVersionOption(ArgParser argParser) { final String pinnedChromeVersion = io.File(path.join(environment.webUiRootDir.path, 'dev', 'chrome.lock')) diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index ac7c28ad5b258..d95bcfdb6a41b 100755 --- a/lib/web_ui/dev/felt +++ b/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 -($DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@) +$DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@ diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index 6b32e32681270..36488b260f4d8 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -21,7 +21,7 @@ void main(List args) async { final bool result = await runner.run(args); if (result == null) { runner.printUsage(); - io.exit(64); + io.exit(64); // Exit code 64 indicates a usage error. } else if (!result) { print('Command returned false: `felt ${args.join(' ')}`'); io.exit(1); diff --git a/lib/web_ui/dev/licenses.dart b/lib/web_ui/dev/licenses.dart index e226471d96859..d86a64edb5863 100644 --- a/lib/web_ui/dev/licenses.dart +++ b/lib/web_ui/dev/licenses.dart @@ -11,7 +11,7 @@ import 'environment.dart'; class LicensesCommand extends Command { @override - final String name = 'licenses'; + final String name = 'check-licenses'; @override final String description = 'Check license headers.'; @@ -19,7 +19,7 @@ class LicensesCommand extends Command { @override bool run() { _checkLicenseHeaders(); - return false; + return true; } void _checkLicenseHeaders() { diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 181164f5d179b..8a4fddc72ca23 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -38,7 +38,7 @@ class TestsCommand extends Command { } @override - final String name = 'tests'; + final String name = 'test'; @override final String description = 'Run tests.'; diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart index 19181bf9309a7..9078648fb9feb 100644 --- a/lib/web_ui/dev/utils.dart +++ b/lib/web_ui/dev/utils.dart @@ -10,31 +10,25 @@ import 'package:path/path.dart' as path; import 'environment.dart'; class FilePath { - FilePath.fromCwd(String filePath) : _path = path.absolute(filePath); - FilePath.fromWebUi(String filePath) - : _path = path.join(environment.webUiRootDir.path, filePath); + FilePath.fromCwd(String relativePath) + : _absolutePath = path.absolute(relativePath); + FilePath.fromWebUi(String relativePath) + : _absolutePath = path.join(environment.webUiRootDir.path, relativePath); - final String _path; + final String _absolutePath; - String get relativeToCwd => path.relative(_path); + String get absolute => _absolutePath; + String get relativeToCwd => path.relative(_absolutePath); String get relativeToWebUi => - path.relative(_path, from: environment.webUiRootDir.path); - - String get absolute => _path; + path.relative(_absolutePath, from: environment.webUiRootDir.path); @override bool operator ==(dynamic other) { - if (other is String) { - return _path == other; - } - if (other is FilePath) { - return _path == other._path; - } - return false; + return other is FilePath && _absolutePath == other._absolutePath; } @override - String toString() => _path; + String toString() => _absolutePath; } Future runProcess( diff --git a/lib/web_ui/lib/src/engine/surface/surface.dart b/lib/web_ui/lib/src/engine/surface/surface.dart index 0dccf7cffc181..73af15c56d3e4 100644 --- a/lib/web_ui/lib/src/engine/surface/surface.dart +++ b/lib/web_ui/lib/src/engine/surface/surface.dart @@ -86,12 +86,15 @@ void commitScene(PersistedScene scene) { _debugPrintSurfaceStats(scene, _debugFrameNumber); _debugRepaintSurfaceStatsOverlay(scene); } + assert(() { - final List validationErrors = []; - scene.debugValidate(validationErrors); - if (validationErrors.isNotEmpty) { - print('ENGINE LAYER TREE INCONSISTENT:\n' - '${validationErrors.map((String e) => ' - $e\n').join()}'); + if (!ui.debugEmulateFlutterTesterEnvironment) { + final List validationErrors = []; + scene.debugValidate(validationErrors); + if (validationErrors.isNotEmpty) { + print('ENGINE LAYER TREE INCONSISTENT:\n' + '${validationErrors.map((String e) => ' - $e\n').join()}'); + } } return true; }()); From 7363f823d867de340b5deb867f8cdb65fa327b30 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 13 Sep 2019 14:25:23 -0700 Subject: [PATCH 4/5] Correct cirrus commands --- .cirrus.yml | 4 ++-- lib/web_ui/dev/felt.dart | 11 +++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index d69bc0f53ee5d..2c7fd035ddca2 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -42,8 +42,8 @@ task: cd $ENGINE_PATH/src/flutter/lib/web_ui $ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/pub get export FELT="$ENGINE_PATH/src/out/host_debug_unopt/dart-sdk/bin/dart dev/felt.dart" - $FELT licenses - CHROME_NO_SANDBOX=true $FELT tests + $FELT check-licenses + CHROME_NO_SANDBOX=true $FELT test fetch_framework_script: | mkdir -p $FRAMEWORK_PATH cd $FRAMEWORK_PATH diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index 36488b260f4d8..cf55f8ff0f50e 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -17,12 +17,15 @@ CommandRunner runner = CommandRunner( ..addCommand(TestsCommand()); void main(List args) async { + 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. + } + try { final bool result = await runner.run(args); - if (result == null) { - runner.printUsage(); - io.exit(64); // Exit code 64 indicates a usage error. - } else if (!result) { + if (result == false) { print('Command returned false: `felt ${args.join(' ')}`'); io.exit(1); } From 50fbd100107b0e58d3d65676e22eeaa5d95a84cc Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 13 Sep 2019 15:08:34 -0700 Subject: [PATCH 5/5] Fix nits --- lib/web_ui/dev/felt.dart | 2 +- lib/web_ui/lib/src/engine/surface/surface.dart | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index cf55f8ff0f50e..c6185ff29ef2d 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -26,7 +26,7 @@ void main(List args) async { try { final bool result = await runner.run(args); if (result == false) { - print('Command returned false: `felt ${args.join(' ')}`'); + print('Sub-command returned false: `${args.join(' ')}`'); io.exit(1); } } on UsageException catch (e) { diff --git a/lib/web_ui/lib/src/engine/surface/surface.dart b/lib/web_ui/lib/src/engine/surface/surface.dart index 73af15c56d3e4..b97930c20f594 100644 --- a/lib/web_ui/lib/src/engine/surface/surface.dart +++ b/lib/web_ui/lib/src/engine/surface/surface.dart @@ -88,13 +88,11 @@ void commitScene(PersistedScene scene) { } assert(() { - if (!ui.debugEmulateFlutterTesterEnvironment) { - final List validationErrors = []; - scene.debugValidate(validationErrors); - if (validationErrors.isNotEmpty) { - print('ENGINE LAYER TREE INCONSISTENT:\n' - '${validationErrors.map((String e) => ' - $e\n').join()}'); - } + final List validationErrors = []; + scene.debugValidate(validationErrors); + if (validationErrors.isNotEmpty) { + print('ENGINE LAYER TREE INCONSISTENT:\n' + '${validationErrors.map((String e) => ' - $e\n').join()}'); } return true; }());