From 9e86ef12402973938159f9018e0760b49c7f7ca7 Mon Sep 17 00:00:00 2001 From: Nurhan Turgut Date: Fri, 6 Mar 2020 13:37:35 -0800 Subject: [PATCH 01/22] adding arguments to felt for running different type of tests --- lib/web_ui/dev/test_runner.dart | 64 ++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 12 deletions(-) diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 0779ad450fad3..11703064e2268 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -20,6 +20,18 @@ import 'test_platform.dart'; import 'environment.dart'; import 'utils.dart'; +/// The type of tests requested by the tool user. +enum TestTypesRequested { + /// For running the unit tests only. + unit, + /// For running the integration tests only. + integration, + /// For running both unit and integration tests. + all, + /// Run no tests. + none, +} + class TestCommand extends Command { TestCommand() { argParser @@ -29,6 +41,17 @@ class TestCommand extends Command { 'opportunity to add breakpoints or inspect loaded code before ' 'running the code.', ) + ..addFlag( + 'unit-tests-only', + help: 'felt test command runs the unit tests and the integration tests ' + 'at the same time. If this flag is set, only run the unit tests.', + ) + ..addFlag( + 'integration-tests-only', + help: 'felt test command runs the unit tests and the integration tests ' + 'at the same time. If this flag is set, only run the integration ' + 'tests.', + ) ..addFlag( 'update-screenshot-goldens', defaultsTo: false, @@ -54,11 +77,28 @@ class TestCommand extends Command { @override final String description = 'Run tests.'; + TestTypesRequested testTypesRequested = TestTypesRequested.none; + @override Future run() async { SupportedBrowsers.instance ..argParsers.forEach((t) => t.parseOptions(argResults)); + // Check the flags to see what type of integration tests are requested. + if (argResults['unit-tests-only'] && argResults['integration-tests-only']) { + throw ArgumentError('Conflicting arguments: unit-tests-only and ' + 'integration-tests-only are both set'); + } else if (argResults['unit-tests-only']) { + print('Running the unit tests only'); + testTypesRequested = TestTypesRequested.unit; + } else if (argResults['integration-tests-only']) { + print('Running the integration tests only. Note that they are only ' + 'available for Chrome on Linux for now.'); + testTypesRequested = TestTypesRequested.integration; + } else { + testTypesRequested = TestTypesRequested.all; + } + _copyTestFontsIntoWebUi(); await _buildHostPage(); if (io.Platform.isWindows) { @@ -252,18 +292,18 @@ class TestCommand extends Command { Future _buildTests({List targets}) async { List arguments = [ - 'run', - 'build_runner', - 'build', - 'test', - '-o', - 'build', - if (targets != null) - for (FilePath path in targets) ...[ - '--build-filter=${path.relativeToWebUi}.js', - '--build-filter=${path.relativeToWebUi}.browser_test.dart.js', - ], - ]; + 'run', + 'build_runner', + 'build', + 'test', + '-o', + 'build', + if (targets != null) + for (FilePath path in targets) ...[ + '--build-filter=${path.relativeToWebUi}.js', + '--build-filter=${path.relativeToWebUi}.browser_test.dart.js', + ], + ]; final int exitCode = await runProcess( environment.pubExecutable, arguments, From 1c146744132437578f7d4a0acbe3f5f4c5394acc Mon Sep 17 00:00:00 2001 From: Nurhan Turgut Date: Fri, 6 Mar 2020 14:39:28 -0800 Subject: [PATCH 02/22] adding test type enums to felt --- lib/web_ui/dev/test_runner.dart | 35 ++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 11703064e2268..70498e88fe4e9 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -24,10 +24,13 @@ import 'utils.dart'; enum TestTypesRequested { /// For running the unit tests only. unit, + /// For running the integration tests only. integration, + /// For running both unit and integration tests. all, + /// Run no tests. none, } @@ -79,26 +82,40 @@ class TestCommand extends Command { TestTypesRequested testTypesRequested = TestTypesRequested.none; - @override - Future run() async { - SupportedBrowsers.instance - ..argParsers.forEach((t) => t.parseOptions(argResults)); - - // Check the flags to see what type of integration tests are requested. + /// Check the flags to see what type of integration tests are requested. + TestTypesRequested findTestType() { if (argResults['unit-tests-only'] && argResults['integration-tests-only']) { throw ArgumentError('Conflicting arguments: unit-tests-only and ' 'integration-tests-only are both set'); } else if (argResults['unit-tests-only']) { print('Running the unit tests only'); - testTypesRequested = TestTypesRequested.unit; + return TestTypesRequested.unit; } else if (argResults['integration-tests-only']) { print('Running the integration tests only. Note that they are only ' 'available for Chrome on Linux for now.'); - testTypesRequested = TestTypesRequested.integration; + return TestTypesRequested.integration; } else { - testTypesRequested = TestTypesRequested.all; + return TestTypesRequested.all; + } + } + + @override + Future run() async { + SupportedBrowsers.instance + ..argParsers.forEach((t) => t.parseOptions(argResults)); + + // Check the flags to see what type of integration tests are requested. + testTypesRequested = findTestType(); + + switch (testTypesRequested) { + case TestTypesRequested.unit: + case TestTypesRequested.integration: + case TestTypesRequested.all: + case TestTypesRequested.none: } + } + Future runUnitTests() async { _copyTestFontsIntoWebUi(); await _buildHostPage(); if (io.Platform.isWindows) { From c669c13d5a2ae52ed2561dd14d9893bc698486a8 Mon Sep 17 00:00:00 2001 From: nturgut Date: Tue, 10 Mar 2020 17:19:17 -0700 Subject: [PATCH 03/22] more changes to felt for testing branches --- lib/web_ui/dev/integration_tests.dart | 13 +++++++++++++ lib/web_ui/dev/test_runner.dart | 16 +++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 lib/web_ui/dev/integration_tests.dart diff --git a/lib/web_ui/dev/integration_tests.dart b/lib/web_ui/dev/integration_tests.dart new file mode 100644 index 0000000000000..e8617574ff665 --- /dev/null +++ b/lib/web_ui/dev/integration_tests.dart @@ -0,0 +1,13 @@ + +class IntegrationTestsManager { + // TODO: update the paths on environment.dart.s + // TODO: Go to integration tests directory and get a list of tests to run. + + // TODO: Check the browser version. Give exception if not chrome. + // TODO: Install and run driver for chrome. + // TODO: Run the tests one by one. + // TODO: For each run print out success fail. Also print out how to rerun. + void runTests() { + print('run tests IntegrationTestsManager'); + } +} diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 70498e88fe4e9..6c80cd0a21610 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -14,6 +14,7 @@ import 'package:test_core/src/runner/hack_register_platform.dart' 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:ui/src/engine.dart'; import 'supported_browsers.dart'; import 'test_platform.dart'; @@ -92,7 +93,11 @@ class TestCommand extends Command { return TestTypesRequested.unit; } else if (argResults['integration-tests-only']) { print('Running the integration tests only. Note that they are only ' - 'available for Chrome on Linux for now.'); + 'available for Chrome Desktop.'); + if (!isChrome) { + throw UnimplementedError( + 'Integration tests are only awailable on Chrome Desktop for now'); + } return TestTypesRequested.integration; } else { return TestTypesRequested.all; @@ -109,10 +114,19 @@ class TestCommand extends Command { switch (testTypesRequested) { case TestTypesRequested.unit: + return runUnitTests(); case TestTypesRequested.integration: + return runIntegrationTests(); case TestTypesRequested.all: + return await runIntegrationTests() && await runUnitTests(); case TestTypesRequested.none: + throw ArgumentError('One test type should be chosed to run felt test.'); } + return false; + } + + Future runIntegrationTests() async { + } Future runUnitTests() async { From 26cb3cd7511bd7201b2307eb1d9462dc7fd8851e Mon Sep 17 00:00:00 2001 From: nturgut Date: Tue, 10 Mar 2020 22:41:33 -0700 Subject: [PATCH 04/22] more additions to code --- lib/web_ui/dev/felt | 1 + lib/web_ui/dev/test_runner.dart | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 07a71f37e8211..4adf38d6df571 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -68,6 +68,7 @@ if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then rm -f "$SNAPSHOT_PATH" rm -f "$STAMP_PATH" install_deps + echo "starting felt" $DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@ else # Create a new snapshot if any of the following is true: diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 6c80cd0a21610..8551b1cc15f64 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -14,8 +14,8 @@ import 'package:test_core/src/runner/hack_register_platform.dart' 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:ui/src/engine.dart'; +import 'integration_tests.dart'; import 'supported_browsers.dart'; import 'test_platform.dart'; import 'environment.dart'; @@ -47,11 +47,13 @@ class TestCommand extends Command { ) ..addFlag( 'unit-tests-only', + defaultsTo: false, help: 'felt test command runs the unit tests and the integration tests ' 'at the same time. If this flag is set, only run the unit tests.', ) ..addFlag( 'integration-tests-only', + defaultsTo: false, help: 'felt test command runs the unit tests and the integration tests ' 'at the same time. If this flag is set, only run the integration ' 'tests.', @@ -85,6 +87,10 @@ class TestCommand extends Command { /// Check the flags to see what type of integration tests are requested. TestTypesRequested findTestType() { + print('chrome: $isChrome'); + print('unit test only: ${argResults['unit-tests-only']}'); + print('int test only: ${argResults['integration-tests-only']}'); + if (argResults['unit-tests-only'] && argResults['integration-tests-only']) { throw ArgumentError('Conflicting arguments: unit-tests-only and ' 'integration-tests-only are both set'); @@ -108,6 +114,7 @@ class TestCommand extends Command { Future run() async { SupportedBrowsers.instance ..argParsers.forEach((t) => t.parseOptions(argResults)); + print('run tests started'); // Check the flags to see what type of integration tests are requested. testTypesRequested = findTestType(); @@ -118,7 +125,11 @@ class TestCommand extends Command { case TestTypesRequested.integration: return runIntegrationTests(); case TestTypesRequested.all: - return await runIntegrationTests() && await runUnitTests(); + bool integrationTestResult = await runIntegrationTests(); + bool unitTestResult = await runUnitTests(); + print('Tests run. Integratiion tests passed: $integrationTestResult ' + 'unit tests passed: $unitTestResult'); + return integrationTestResult && unitTestResult; case TestTypesRequested.none: throw ArgumentError('One test type should be chosed to run felt test.'); } @@ -126,7 +137,7 @@ class TestCommand extends Command { } Future runIntegrationTests() async { - + IntegrationTestsManager().runTests(); } Future runUnitTests() async { From 533eda75649b8e5cab7e8b7207363bb04bfbb086 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 11 Mar 2020 13:03:48 -0700 Subject: [PATCH 05/22] more changes to felt --- lib/web_ui/dev/environment.dart | 6 ++++++ lib/web_ui/dev/integration_tests.dart | 16 +++++++++++++--- lib/web_ui/dev/test_runner.dart | 2 +- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 35c11a9e073dc..6af6e6e874271 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -22,6 +22,7 @@ class Environment { final io.Directory hostDebugUnoptDir = io.Directory(pathlib.join(outDir.path, 'host_debug_unopt')); final io.Directory dartSdkDir = io.Directory(pathlib.join(hostDebugUnoptDir.path, 'dart-sdk')); final io.Directory webUiRootDir = io.Directory(pathlib.join(engineSrcDir.path, 'flutter', 'lib', 'web_ui')); + final io.Directory integrationTestsDir = io.Directory(pathlib.join(engineSrcDir.path, 'flutter', 'e2etests', 'web')); for (io.Directory expectedDirectory in [engineSrcDir, outDir, hostDebugUnoptDir, dartSdkDir, webUiRootDir]) { if (!expectedDirectory.existsSync()) { @@ -34,6 +35,7 @@ class Environment { self: self, webUiRootDir: webUiRootDir, engineSrcDir: engineSrcDir, + integrationTestsDir: integrationTestsDir, outDir: outDir, hostDebugUnoptDir: hostDebugUnoptDir, dartSdkDir: dartSdkDir, @@ -44,6 +46,7 @@ class Environment { this.self, this.webUiRootDir, this.engineSrcDir, + this.integrationTestsDir, this.outDir, this.hostDebugUnoptDir, this.dartSdkDir, @@ -58,6 +61,9 @@ class Environment { /// Path to the engine's "src" directory. final io.Directory engineSrcDir; + /// Path to the web integration tests. + final io.Directory integrationTestsDir; + /// Path to the engine's "out" directory. /// /// This is where you'll find the ninja output, such as the Dart SDK. diff --git a/lib/web_ui/dev/integration_tests.dart b/lib/web_ui/dev/integration_tests.dart index e8617574ff665..6085086d7256d 100644 --- a/lib/web_ui/dev/integration_tests.dart +++ b/lib/web_ui/dev/integration_tests.dart @@ -1,13 +1,23 @@ class IntegrationTestsManager { - // TODO: update the paths on environment.dart.s + + final String _browser; + IntegrationTestsManager(this._browser); + + // (DONE) TODO: update the paths on environment.dart. // TODO: Go to integration tests directory and get a list of tests to run. - // TODO: Check the browser version. Give exception if not chrome. + // (DONE) TODO: Check the browser version. Give warning if not chrome. // TODO: Install and run driver for chrome. // TODO: Run the tests one by one. // TODO: For each run print out success fail. Also print out how to rerun. void runTests() { - print('run tests IntegrationTestsManager'); + print('run testssss: $_browser'); + if(_browser != 'chrome') { + print('WARNING: integration tests are only suppoted on chrome for now'); + return; + } else { + print('run tests IntegrationTestsManager'); + } } } diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 8551b1cc15f64..700b051b79623 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -137,7 +137,7 @@ class TestCommand extends Command { } Future runIntegrationTests() async { - IntegrationTestsManager().runTests(); + IntegrationTestsManager(browser).runTests(); } Future runUnitTests() async { From 3e634386d698ec3b8d46d4a1e3170c80d6e17a7b Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 11 Mar 2020 19:12:44 -0700 Subject: [PATCH 06/22] adding code for cloning web_drivers --- lib/web_ui/dev/firefox_installer.dart | 1 + lib/web_ui/dev/integration_tests.dart | 23 --- lib/web_ui/dev/integration_tests_manager.dart | 160 ++++++++++++++++++ .../dev/integration_tests_manager_test.dart | 29 ++++ lib/web_ui/dev/test_runner.dart | 9 +- 5 files changed, 192 insertions(+), 30 deletions(-) delete mode 100644 lib/web_ui/dev/integration_tests.dart create mode 100644 lib/web_ui/dev/integration_tests_manager.dart create mode 100644 lib/web_ui/dev/integration_tests_manager_test.dart diff --git a/lib/web_ui/dev/firefox_installer.dart b/lib/web_ui/dev/firefox_installer.dart index d3043e9f68303..56105e5d8fe77 100644 --- a/lib/web_ui/dev/firefox_installer.dart +++ b/lib/web_ui/dev/firefox_installer.dart @@ -189,6 +189,7 @@ class FirefoxInstaller { versionDir.createSync(recursive: true); final String url = PlatformBinding.instance.getFirefoxDownloadUrl(version); + print('url to download: $url'); final StreamedResponse download = await client.send(Request( 'GET', Uri.parse(url), diff --git a/lib/web_ui/dev/integration_tests.dart b/lib/web_ui/dev/integration_tests.dart deleted file mode 100644 index 6085086d7256d..0000000000000 --- a/lib/web_ui/dev/integration_tests.dart +++ /dev/null @@ -1,23 +0,0 @@ - -class IntegrationTestsManager { - - final String _browser; - IntegrationTestsManager(this._browser); - - // (DONE) TODO: update the paths on environment.dart. - // TODO: Go to integration tests directory and get a list of tests to run. - - // (DONE) TODO: Check the browser version. Give warning if not chrome. - // TODO: Install and run driver for chrome. - // TODO: Run the tests one by one. - // TODO: For each run print out success fail. Also print out how to rerun. - void runTests() { - print('run testssss: $_browser'); - if(_browser != 'chrome') { - print('WARNING: integration tests are only suppoted on chrome for now'); - return; - } else { - print('run tests IntegrationTestsManager'); - } - } -} diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart new file mode 100644 index 0000000000000..a5a7c183d1f74 --- /dev/null +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -0,0 +1,160 @@ +// 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:path/path.dart' as pathlib; + +import 'environment.dart'; +import 'utils.dart'; + +class IntegrationTestsManager { + final String _browser; + + /// Installation directory for browser's driver. + /// + /// Always re-install since driver can change frequently. + /// It usually changes with each the browser version changes. + /// A better solution would be installing the browser and the driver at the + /// same time. + // TODO(nurhan): + io.Directory _driverDir; + + IntegrationTestsManager(this._browser) { + _driverDir = io.Directory( + pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); + } + + // (DONE) TODO: update the paths on environment.dart. + // TODO: Go to integration tests directory and get a list of tests to run. + + // (DONE) TODO: Check the browser version. Give warning if not chrome. + // TODO: Install and run driver for chrome. + // TODO: Run the tests one by one. + // TODO: For each run print out success fail. Also print out how to rerun. + Future runTests() async { + print('run testssss: $_browser'); + if (_browser != 'chrome') { + print('WARNING: integration tests are only suppoted on chrome for now'); + return false; + } else { + print('run tests IntegrationTestsManager'); + return prepareDriver(); + } + } + Future _cloneWebInstalllers() async { + final int exitCode = await runProcess( + 'git', + [ + 'clone', + 'https://github.com/flutter/web_installers.git', + ], + workingDirectory: _driverDir.path, + ); + + if (exitCode != 0) { + io.stderr.writeln( + 'Failed to clone web installers. Exited with exit code $exitCode'); + return false; + } else { + return true; + } + } + + Future _runPubGet() async { + final int exitCode = await runProcess( + environment.pubExecutable, + [ + 'get', + ], + workingDirectory: pathlib.join( + _driverDir.path, 'web_installers', 'packages', 'web_drivers'), + ); + + if (exitCode != 0) { + io.stderr + .writeln('Failed to run pub get. Exited with exit code $exitCode'); + return false; + } else { + return true; + } + } + + Future _runDriver() async { + final int exitCode = await runProcess( + environment.dartExecutable, + [ + 'lib/web_driver_installer.dart', + '${_browser}driver', + '--install-only', + ], + workingDirectory: pathlib.join( + _driverDir.path, 'web_installers', 'packages', 'web_drivers'), + ); + + if (exitCode != 0) { + io.stderr + .writeln('Failed to run driver. Exited with exit code $exitCode'); + return false; + } else { + return true; + } + } + + Future prepareDriver() async { + if (_driverDir.existsSync()) { + _driverDir.deleteSync(recursive: true); + } + + _driverDir.createSync(recursive: true); + + bool installWebInstallers = await _cloneWebInstalllers(); + if(installWebInstallers) { + bool pubGet = await _runPubGet(); + if(pubGet) { + // test install only for now. + return await _runDriver(); + } + } + return false; + } + + } + + // List getListOfTests(String browser) { + // // Only list the files under web_ui/test. + // final Directory testDirectory = + // Directory(pathlib.join(environment.webUiRootDir.path, 'test')); + + // final List entities = + // testDirectory.listSync(recursive: true, followLinks: false); + // final List testFilesToBuild = List(); + + // print( + // 'Listing test files under directory: ${testDirectory.path.toString()}'); + // int count = 0; + // for (FileSystemEntity e in entities) { + // if (e is File) { + // final String path = e.path.toString(); + // // Listing only test files and not the test doubles. + // if (path.endsWith('_test.dart')) { + // // Add Goldens only for Linux Chrome. + // if (path.contains('golden_tests') && + // Platform.isLinux && + // browser == 'chrome') { + // print('file ${e.path.toString()}'); + // testFilesToBuild.add(e); + // count++; + // } else if (!path.contains('golden_tests')) { + // print('file ${e.path.toString()}'); + // testFilesToBuild.add(e); + // count++; + // } else { + // print('file skipped ${e.path.toString()}'); + // } + // } + // } + // } + // print('Listed $count items. ${testFilesToBuild.length}'); + // return testFilesToBuild; + // } diff --git a/lib/web_ui/dev/integration_tests_manager_test.dart b/lib/web_ui/dev/integration_tests_manager_test.dart new file mode 100644 index 0000000000000..2997b058543b5 --- /dev/null +++ b/lib/web_ui/dev/integration_tests_manager_test.dart @@ -0,0 +1,29 @@ +// 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. + +@TestOn('vm && linux') + +// @dart = 2.6 +import 'dart:io' as io; + +import 'package:path/path.dart' as path; +import 'package:test/test.dart'; + +import 'common.dart'; +import 'environment.dart'; +import 'integration_tests_manager.dart'; + +void main() async { + IntegrationTestsManager testsManager; + setUpAll(() { + testsManager = IntegrationTestsManager('chrome'); + }); + + tearDown(() { + }); + + test('run an install chrome driver', () async { + testsManager.prepareDriver(); + }); +} diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 700b051b79623..bd64077a85676 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -15,7 +15,7 @@ import 'package:test_api/src/backend/runtime.dart'; // ignore: implementation_im import 'package:test_core/src/executable.dart' as test; // ignore: implementation_imports -import 'integration_tests.dart'; +import 'integration_tests_manager.dart'; import 'supported_browsers.dart'; import 'test_platform.dart'; import 'environment.dart'; @@ -87,10 +87,6 @@ class TestCommand extends Command { /// Check the flags to see what type of integration tests are requested. TestTypesRequested findTestType() { - print('chrome: $isChrome'); - print('unit test only: ${argResults['unit-tests-only']}'); - print('int test only: ${argResults['integration-tests-only']}'); - if (argResults['unit-tests-only'] && argResults['integration-tests-only']) { throw ArgumentError('Conflicting arguments: unit-tests-only and ' 'integration-tests-only are both set'); @@ -114,7 +110,6 @@ class TestCommand extends Command { Future run() async { SupportedBrowsers.instance ..argParsers.forEach((t) => t.parseOptions(argResults)); - print('run tests started'); // Check the flags to see what type of integration tests are requested. testTypesRequested = findTestType(); @@ -127,7 +122,7 @@ class TestCommand extends Command { case TestTypesRequested.all: bool integrationTestResult = await runIntegrationTests(); bool unitTestResult = await runUnitTests(); - print('Tests run. Integratiion tests passed: $integrationTestResult ' + print('Tests run. Integration tests passed: $integrationTestResult ' 'unit tests passed: $unitTestResult'); return integrationTestResult && unitTestResult; case TestTypesRequested.none: From c969d82214cbf9ec4adf70284459fd787b05c2e7 Mon Sep 17 00:00:00 2001 From: nturgut Date: Tue, 17 Mar 2020 19:39:41 -0700 Subject: [PATCH 07/22] validating test directories. running the integration tests. update the readme file partially --- lib/web_ui/dev/README.md | 18 +- lib/web_ui/dev/felt | 4 +- lib/web_ui/dev/integration_tests_manager.dart | 406 +++++++++++++----- lib/web_ui/dev/test_runner.dart | 2 +- lib/web_ui/dev/utils.dart | 19 + 5 files changed, 346 insertions(+), 103 deletions(-) diff --git a/lib/web_ui/dev/README.md b/lib/web_ui/dev/README.md index 5e5f5d45c0832..b418ac17df59e 100644 --- a/lib/web_ui/dev/README.md +++ b/lib/web_ui/dev/README.md @@ -31,16 +31,28 @@ felt build [-w] -j 100 If you are a Google employee, you can use an internal instance of Goma to parallelize your builds. Because Goma compiles code on remote servers, this option is effective even on low-powered laptops. ## Running web engine tests -To run all tests on Chrome: +To run all tests on Chrome. This will run both integration tests and the unit tests: ``` felt test ``` +To run unit tests only: + +``` +felt test --unit-tests-only +``` + +To run integration tests only. For now these tests are only available on Chrome Desktop browsers. + +``` +felt test --integration-tests-only +``` + To run tests on Firefox (this will work only on a Linux device): ``` -felt test --browser=firefox +felt test --browser=firefox ``` For Chrome and Firefox, the tests run on a version locked on the [browser_lock.yaml](https://github.com/flutter/engine/blob/master/lib/web_ui/dev/browser_lock.yaml). In order to use another version, add the version argument: @@ -55,7 +67,7 @@ To run tests on Safari use the following command. It works on MacOS devices and felt test --browser=safari ``` -To run tests on Windows Edge use the following command. It works on Windows devices and it uses the Edge installed on the OS. +To run tests on Windows Edge use the following command. It works on Windows devices and it uses the Edge installed on the OS. ``` felt_windows.bat test --browser=edge diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 4adf38d6df571..43bb340013ddc 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -57,8 +57,8 @@ KERNEL_NAME=`uname` if [[ $KERNEL_NAME == *"Darwin"* ]] then echo "Running on MacOS. Increase the user limits" - ulimit -n 50000 - ulimit -u 4096 + # ulimit -n 50000 + # ulimit -u 4096 fi if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index a5a7c183d1f74..ee42a8a4cc1dd 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -5,6 +5,7 @@ import 'dart:io' as io; import 'package:path/path.dart' as pathlib; +import 'common.dart'; import 'environment.dart'; import 'utils.dart'; @@ -29,132 +30,343 @@ class IntegrationTestsManager { // TODO: Go to integration tests directory and get a list of tests to run. // (DONE) TODO: Check the browser version. Give warning if not chrome. - // TODO: Install and run driver for chrome. + // (DONE) TODO: Install the driver + // (DONE) TODO: run driver in the background. + // (DONE) TODO: Go to the list of integration tests directory and loop for each one. + // TODO: Fetch flutter for CIs. // TODO: Run the tests one by one. // TODO: For each run print out success fail. Also print out how to rerun. - Future runTests() async { - print('run testssss: $_browser'); + // TODO: Make sure tests finish when they run + // TODO: Make sure the error is visible. + Future run() async { if (_browser != 'chrome') { print('WARNING: integration tests are only suppoted on chrome for now'); return false; } else { - print('run tests IntegrationTestsManager'); - return prepareDriver(); + bool driverReady = await prepareDriver(); + // TODO(nurhan): provide a flag for running as if on CI. Also use this + // flag from LUCI. + if (isCirrus != null && isCirrus) { + // TODO(nurhan): fetch Flutter, we will use flutter pub get in the next steps. + } + if (driverReady) { + return await _runTests(); + } else { + return false; + } } } - Future _cloneWebInstalllers() async { - final int exitCode = await runProcess( - 'git', - [ - 'clone', - 'https://github.com/flutter/web_installers.git', - ], - workingDirectory: _driverDir.path, - ); - if (exitCode != 0) { - io.stderr.writeln( - 'Failed to clone web installers. Exited with exit code $exitCode'); - return false; - } else { - return true; - } + Future _cloneWebInstalllers() async { + final int exitCode = await runProcess( + 'git', + [ + 'clone', + 'https://github.com/flutter/web_installers.git', + ], + workingDirectory: _driverDir.path, + ); + + if (exitCode != 0) { + io.stderr.writeln('ERROR: ' + 'Failed to clone web installers. Exited with exit code $exitCode'); + return false; + } else { + return true; } + } + + Future _runPubGet(String workingDirectory) async { + // TODO(nurhan): Run flutter pub get from the fetched flutter on CI. + final int exitCode = await runProcess( + //environment.pubExecutable, + 'flutter', + [ + 'pub', + 'get', + ], + workingDirectory: workingDirectory, + ); + + if (exitCode != 0) { + io.stderr.writeln( + 'ERROR: Failed to run pub get. Exited with exit code $exitCode'); + return false; + } else { + return true; + } + } + + Future _runDriver() async { + final int exitCode = await runProcess( + environment.dartExecutable, + [ + 'lib/web_driver_installer.dart', + '${_browser}driver', + '--install-only', + ], + workingDirectory: pathlib.join( + _driverDir.path, 'web_installers', 'packages', 'web_drivers'), + ); - Future _runPubGet() async { - final int exitCode = await runProcess( - environment.pubExecutable, - [ - 'get', - ], + if (exitCode != 0) { + io.stderr.writeln( + 'ERROR: Failed to run driver. Exited with exit code $exitCode'); + return false; + } else { + runProcessInTheBackground( + './chromedriver/chromedriver', + ['--port=4444'], workingDirectory: pathlib.join( _driverDir.path, 'web_installers', 'packages', 'web_drivers'), ); + print('INFO: Driver started'); + return true; + } + } - if (exitCode != 0) { - io.stderr - .writeln('Failed to run pub get. Exited with exit code $exitCode'); - return false; + Future prepareDriver() async { + if (_driverDir.existsSync()) { + _driverDir.deleteSync(recursive: true); + } + + _driverDir.createSync(recursive: true); + + bool installWebInstallers = await _cloneWebInstalllers(); + if (installWebInstallers) { + bool pubGet = await _runPubGet(pathlib.join( + _driverDir.path, 'web_installers', 'packages', 'web_drivers')); + if (pubGet) { + return await _runDriver(); } else { - return true; + return false; } } + return false; + } - Future _runDriver() async { - final int exitCode = await runProcess( - environment.dartExecutable, - [ - 'lib/web_driver_installer.dart', - '${_browser}driver', - '--install-only', - ], - workingDirectory: pathlib.join( - _driverDir.path, 'web_installers', 'packages', 'web_drivers'), - ); + /// Runs the all the web tests under e2e_tests/web. + Future _runTests() async { + // Only list the files under e2e_tests/web. + final List entities = + environment.integrationTestsDir.listSync(followLinks: false); - if (exitCode != 0) { - io.stderr - .writeln('Failed to run driver. Exited with exit code $exitCode'); - return false; - } else { - return true; + print('INFO: Listing test files under directory: ' + '${environment.integrationTestsDir.path.toString()}'); + bool testResults = true; + for (io.FileSystemEntity e in entities) { + // The tests should be under this directories. + if (e is io.Directory) { + testResults = testResults && await _runTestsInTheDirectory(e); } } + return testResults; + } + + /// Run tests in a single directory under: e2e_tests/web. + /// + /// Run `flutter pub get` as the first step. + /// + /// Validate the directory before running the tests. Each directory is + /// expected to be a test project which includes a `pubspec.yaml` file + /// and a `test_driver` directory. + Future _runTestsInTheDirectory(io.Directory directory) async { + // TODO: Validation step. check if the files the directory has. + // (DONE) (1) pubspec yaml. + // (DONE) (2) test_driver directory for tests. + + // (DONE) Run pub get. + // (DONE) Get the list of tests under driver. .dart which has _test.dart. + // (DONE) Print WARNING: for cases where there is a file missing _test.dart + // (DONE) Print WARNING: if no tests to run. + + // TODO: Run the tests in debug mode. + // TODO: print out a statement where to run the test and the command. + // TODO: Print the result. + // TODO: return the result as boolean. + final bool directoryContentValid = _validateTestDirectory(directory); + if (directoryContentValid) { + _runPubGet(directory.path); + _runTestsInDirectory(directory); + } else { + return false; + } + } + + Future _runTestsInDirectory(io.Directory directory) async { + final io.Directory testDirectory = + io.Directory(pathlib.join(directory.path, 'test_driver')); + final List entities = + testDirectory.listSync(followLinks: false); - Future prepareDriver() async { - if (_driverDir.existsSync()) { - _driverDir.deleteSync(recursive: true); + final List e2eTestsToRun = List(); + + // The following loops over the contents of the directory and saves an + // expected driver file name for each e2e test assuming any dart file + // not ending with `_test.dart` is an e2e test. + // Other files are not considered since developers can add files such as + // README. + for (io.File f in entities) { + final String basename = pathlib.basename(f.path); + if (!basename.contains('_test.dart') && basename.endsWith('.dart')) { + e2eTestsToRun.add(basename); } + } - _driverDir.createSync(recursive: true); + print( + 'INFO: In project ${directory} ${e2eTestsToRun.length} tests to run.'); - bool installWebInstallers = await _cloneWebInstalllers(); - if(installWebInstallers) { - bool pubGet = await _runPubGet(); - if(pubGet) { - // test install only for now. - return await _runDriver(); - } + int numberOfPassedTests; + int numberOfFailedTests; + for (String fileName in e2eTestsToRun) { + print('test to run: $fileName'); + final bool testResults = await _runTestsInDebugMode(directory, fileName); + if (testResults) { + numberOfPassedTests++; + } else { + numberOfFailedTests++; } + } + + final int numberOfTestsRun = numberOfPassedTests + numberOfFailedTests; + + print('${numberOfTestsRun} tests run ${numberOfPassedTests} passed and' + '${numberOfFailedTests} failed.'); + + return numberOfFailedTests == 0; + } + + Future _runTestsInDebugMode( + io.Directory directory, String testName) async { + // TODO(nurhan): Give options to the developer to run tests in another mode. + final String statementToRun = 'flutter drive ' + '--target=test_driver/${testName} -d web-server --release ' + '--browser-name=$_browser --local-engine=host_debug_unopt'; + print('INFO: To manually run the test use $statementToRun under ' + 'directory ${directory.path}'); + final int exitCode = await runProcess( + //environment.pubExecutable, + 'flutter', + [ + 'drive', + '--target=test_driver/${testName}', + '-d', + 'web-server', + '--release', + '--browser-name=$_browser', + '--local-engine=host_debug_unopt', + ], + workingDirectory: directory.path, + ); + + if (exitCode != 0) { + io.stderr + .writeln('ERROR: Failed to run test. Exited with exit code $exitCode.' + ' Statement to run $statementToRun'); return false; + } else { + return true; } + } + /// Validate the directory has a `pubspec.yaml` file and a `test_driver` + /// directory. + /// + /// Also check the validity of files under `test_driver` directory calling + /// [_checkE2ETestsValidity] method. + bool _validateTestDirectory(io.Directory directory) { + final List entities = + directory.listSync(followLinks: false); + + // Whether the project has the pubspec.yaml file. + bool pubSpecFound = false; + // The test directory 'test_driver'. + io.Directory testDirectory = null; + + for (io.FileSystemEntity e in entities) { + // The tests should be under this directories. + final String baseName = pathlib.basename(e.path); + if (e is io.Directory && baseName == 'test_driver') { + testDirectory = e; + } + if (e is io.File && baseName == 'pubspec.yaml') { + pubSpecFound = true; + } + } + if (!pubSpecFound) { + io.stderr + .writeln('ERROR: pubspec.yaml file not found in the test project.'); + return false; + } + if (testDirectory == null) { + io.stderr + .writeln('ERROR: test_driver folder not found in the test project.'); + return false; + } else { + return _checkE2ETestsValidity(testDirectory); + } } - // List getListOfTests(String browser) { - // // Only list the files under web_ui/test. - // final Directory testDirectory = - // Directory(pathlib.join(environment.webUiRootDir.path, 'test')); - - // final List entities = - // testDirectory.listSync(recursive: true, followLinks: false); - // final List testFilesToBuild = List(); - - // print( - // 'Listing test files under directory: ${testDirectory.path.toString()}'); - // int count = 0; - // for (FileSystemEntity e in entities) { - // if (e is File) { - // final String path = e.path.toString(); - // // Listing only test files and not the test doubles. - // if (path.endsWith('_test.dart')) { - // // Add Goldens only for Linux Chrome. - // if (path.contains('golden_tests') && - // Platform.isLinux && - // browser == 'chrome') { - // print('file ${e.path.toString()}'); - // testFilesToBuild.add(e); - // count++; - // } else if (!path.contains('golden_tests')) { - // print('file ${e.path.toString()}'); - // testFilesToBuild.add(e); - // count++; - // } else { - // print('file skipped ${e.path.toString()}'); - // } - // } - // } - // } - // print('Listed $count items. ${testFilesToBuild.length}'); - // return testFilesToBuild; - // } + /// Checks if each e2e test file in the directory has a driver test + /// file to run it. + /// + /// Prints informative message to the developer if an error has found. + /// For each e2e test which has name {name}.dart there will be a driver + /// file which drives it. The driver file should be named: + /// {name}_test.dart + bool _checkE2ETestsValidity(io.Directory testDirectory) { + final List entities = + testDirectory.listSync(followLinks: false); + + final Set expectedDriverFileNames = Set(); + final Set foundDriverFileNames = Set(); + int numberOfTests = 0; + + // The following loops over the contents of the directory and saves an + // expected driver file name for each e2e test assuming any file + // not ending with `_test.dart` is an e2e test. + for (io.File f in entities) { + final String basename = pathlib.basename(f.path); + print('basename: $basename'); + if (basename.contains('_test.dart')) { + // First remove this from expectedSet if not there add to the foundSet. + if (expectedDriverFileNames.contains(basename)) { + expectedDriverFileNames.remove(basename); + } else { + foundDriverFileNames.add(basename); + } + } else if (basename.contains('.dart')) { + // Only run on dart files. + final String e2efileName = + basename.substring(0, basename.indexOf('.dart')); + final String expectedDriverName = '${e2efileName}_test.dart'; + numberOfTests++; + // First remove this from foundSet if not there add to the expectedSet. + if (foundDriverFileNames.contains(expectedDriverName)) { + foundDriverFileNames.remove(expectedDriverName); + } else { + expectedDriverFileNames.add(expectedDriverName); + } + } + } + + if (numberOfTests == 0) { + print('WARNING: No tests to run in this directory.'); + } + + // TODO(nurhan): In order to reduce the work required from team members, + // remove the need for driver file, by using the same template file. + // Some driver files are missing. + if (expectedDriverFileNames.length > 0) { + for (String expectedDriverName in expectedDriverFileNames) { + print('ERROR: Test driver file named has ${expectedDriverName} ' + 'not found under directory ${testDirectory.path}. Stopping the ' + 'integration tests. Please add ${expectedDriverName}. Check to ' + 'README file on more details on how to setup integration tests.'); + } + return false; + } else { + return true; + } + } +} diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index bd64077a85676..b5d988e649142 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -132,7 +132,7 @@ class TestCommand extends Command { } Future runIntegrationTests() async { - IntegrationTestsManager(browser).runTests(); + return IntegrationTestsManager(browser).run(); } Future runUnitTests() async { diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart index d60df326b3eea..1588584a06287 100644 --- a/lib/web_ui/dev/utils.dart +++ b/lib/web_ui/dev/utils.dart @@ -62,6 +62,25 @@ Future runProcess( return exitCode; } +/// Runs [executable]. Do not follow the exit code or the output. +void runProcessInTheBackground( + String executable, + List arguments, { + String workingDirectory, + bool mustSucceed: false, +}) async { + arguments.add('&'); + io.Process.start( + executable, + arguments, + workingDirectory: workingDirectory, + // Running the process in a system shell for Windows. Otherwise + // the process is not able to get Dart from path. + runInShell: io.Platform.isWindows, + mode: io.ProcessStartMode.inheritStdio, + ); +} + /// Runs [executable] and returns its standard output as a string. /// /// If the process fails, throws a [ProcessException]. From dd7845d89ed1e74ea58e4aaa4139f6ae9603e8c5 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 18 Mar 2020 11:25:35 -0700 Subject: [PATCH 08/22] Removing todo lists used for development --- lib/web_ui/dev/integration_tests_manager.dart | 47 ++++--------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index ee42a8a4cc1dd..8f51a0467377a 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -18,7 +18,8 @@ class IntegrationTestsManager { /// It usually changes with each the browser version changes. /// A better solution would be installing the browser and the driver at the /// same time. - // TODO(nurhan): + // TODO(nurhan): change the web installers to install driver and the browser + // at the same time. io.Directory _driverDir; IntegrationTestsManager(this._browser) { @@ -26,18 +27,6 @@ class IntegrationTestsManager { pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); } - // (DONE) TODO: update the paths on environment.dart. - // TODO: Go to integration tests directory and get a list of tests to run. - - // (DONE) TODO: Check the browser version. Give warning if not chrome. - // (DONE) TODO: Install the driver - // (DONE) TODO: run driver in the background. - // (DONE) TODO: Go to the list of integration tests directory and loop for each one. - // TODO: Fetch flutter for CIs. - // TODO: Run the tests one by one. - // TODO: For each run print out success fail. Also print out how to rerun. - // TODO: Make sure tests finish when they run - // TODO: Make sure the error is visible. Future run() async { if (_browser != 'chrome') { print('WARNING: integration tests are only suppoted on chrome for now'); @@ -171,23 +160,11 @@ class IntegrationTestsManager { /// expected to be a test project which includes a `pubspec.yaml` file /// and a `test_driver` directory. Future _runTestsInTheDirectory(io.Directory directory) async { - // TODO: Validation step. check if the files the directory has. - // (DONE) (1) pubspec yaml. - // (DONE) (2) test_driver directory for tests. - - // (DONE) Run pub get. - // (DONE) Get the list of tests under driver. .dart which has _test.dart. - // (DONE) Print WARNING: for cases where there is a file missing _test.dart - // (DONE) Print WARNING: if no tests to run. - - // TODO: Run the tests in debug mode. - // TODO: print out a statement where to run the test and the command. - // TODO: Print the result. - // TODO: return the result as boolean. final bool directoryContentValid = _validateTestDirectory(directory); if (directoryContentValid) { - _runPubGet(directory.path); - _runTestsInDirectory(directory); + await _runPubGet(directory.path); + final bool testResults = await _runTestsInDirectory(directory); + return testResults; } else { return false; } @@ -216,10 +193,9 @@ class IntegrationTestsManager { print( 'INFO: In project ${directory} ${e2eTestsToRun.length} tests to run.'); - int numberOfPassedTests; - int numberOfFailedTests; + int numberOfPassedTests = 0; + int numberOfFailedTests = 0; for (String fileName in e2eTestsToRun) { - print('test to run: $fileName'); final bool testResults = await _runTestsInDebugMode(directory, fileName); if (testResults) { numberOfPassedTests++; @@ -227,32 +203,29 @@ class IntegrationTestsManager { numberOfFailedTests++; } } - final int numberOfTestsRun = numberOfPassedTests + numberOfFailedTests; - print('${numberOfTestsRun} tests run ${numberOfPassedTests} passed and' + print('${numberOfTestsRun} tests run ${numberOfPassedTests} passed and ' '${numberOfFailedTests} failed.'); - return numberOfFailedTests == 0; } Future _runTestsInDebugMode( io.Directory directory, String testName) async { - // TODO(nurhan): Give options to the developer to run tests in another mode. final String statementToRun = 'flutter drive ' '--target=test_driver/${testName} -d web-server --release ' '--browser-name=$_browser --local-engine=host_debug_unopt'; print('INFO: To manually run the test use $statementToRun under ' 'directory ${directory.path}'); + // TODO(nurhan): Give options to the developer to run tests in another mode. final int exitCode = await runProcess( - //environment.pubExecutable, 'flutter', [ 'drive', '--target=test_driver/${testName}', '-d', 'web-server', - '--release', + '--profile', '--browser-name=$_browser', '--local-engine=host_debug_unopt', ], From 66451f74b400a9d98ef916424cad664e037c7eab Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 18 Mar 2020 15:00:38 -0700 Subject: [PATCH 09/22] addressing reviewers comments --- lib/web_ui/dev/README.md | 2 +- lib/web_ui/dev/felt | 7 +++---- lib/web_ui/dev/firefox_installer.dart | 1 - lib/web_ui/dev/integration_tests_manager.dart | 15 +++++++-------- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/web_ui/dev/README.md b/lib/web_ui/dev/README.md index b418ac17df59e..c79dc1f3d6c46 100644 --- a/lib/web_ui/dev/README.md +++ b/lib/web_ui/dev/README.md @@ -52,7 +52,7 @@ felt test --integration-tests-only To run tests on Firefox (this will work only on a Linux device): ``` -felt test --browser=firefox +felt test --browser=firefox ``` For Chrome and Firefox, the tests run on a version locked on the [browser_lock.yaml](https://github.com/flutter/engine/blob/master/lib/web_ui/dev/browser_lock.yaml). In order to use another version, add the version argument: diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 43bb340013ddc..56b031ce0b616 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -56,9 +56,9 @@ install_deps() { KERNEL_NAME=`uname` if [[ $KERNEL_NAME == *"Darwin"* ]] then - echo "Running on MacOS. Increase the user limits" - # ulimit -n 50000 - # ulimit -u 4096 + echo "Running on MacOS. Increase the user limits" + ulimit -n 50000 + ulimit -u 4096 fi if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then @@ -68,7 +68,6 @@ if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then rm -f "$SNAPSHOT_PATH" rm -f "$STAMP_PATH" install_deps - echo "starting felt" $DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@ else # Create a new snapshot if any of the following is true: diff --git a/lib/web_ui/dev/firefox_installer.dart b/lib/web_ui/dev/firefox_installer.dart index 56105e5d8fe77..d3043e9f68303 100644 --- a/lib/web_ui/dev/firefox_installer.dart +++ b/lib/web_ui/dev/firefox_installer.dart @@ -189,7 +189,6 @@ class FirefoxInstaller { versionDir.createSync(recursive: true); final String url = PlatformBinding.instance.getFirefoxDownloadUrl(version); - print('url to download: $url'); final StreamedResponse download = await client.send(Request( 'GET', Uri.parse(url), diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 8f51a0467377a..57abe00cdbc67 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -20,16 +20,15 @@ class IntegrationTestsManager { /// same time. // TODO(nurhan): change the web installers to install driver and the browser // at the same time. - io.Directory _driverDir; + final io.Directory _driverDir; - IntegrationTestsManager(this._browser) { - _driverDir = io.Directory( - pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); - } + IntegrationTestsManager(this._browser) + : this._driverDir = io.Directory( + pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); Future run() async { if (_browser != 'chrome') { - print('WARNING: integration tests are only suppoted on chrome for now'); + print('WARNING: integration tests are only supported on chrome for now'); return false; } else { bool driverReady = await prepareDriver(); @@ -216,7 +215,7 @@ class IntegrationTestsManager { '--target=test_driver/${testName} -d web-server --release ' '--browser-name=$_browser --local-engine=host_debug_unopt'; print('INFO: To manually run the test use $statementToRun under ' - 'directory ${directory.path}'); + 'directory ${directory.path}'); // TODO(nurhan): Give options to the developer to run tests in another mode. final int exitCode = await runProcess( 'flutter', @@ -225,7 +224,7 @@ class IntegrationTestsManager { '--target=test_driver/${testName}', '-d', 'web-server', - '--profile', + '--profile', '--browser-name=$_browser', '--local-engine=host_debug_unopt', ], From 027b55d198c1c647cedf3a334c1729b545c5b940 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 18 Mar 2020 15:01:23 -0700 Subject: [PATCH 10/22] remove unused imports --- lib/web_ui/dev/integration_tests_manager_test.dart | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager_test.dart b/lib/web_ui/dev/integration_tests_manager_test.dart index 2997b058543b5..880ec6a6379da 100644 --- a/lib/web_ui/dev/integration_tests_manager_test.dart +++ b/lib/web_ui/dev/integration_tests_manager_test.dart @@ -4,14 +4,8 @@ @TestOn('vm && linux') -// @dart = 2.6 -import 'dart:io' as io; - -import 'package:path/path.dart' as path; import 'package:test/test.dart'; -import 'common.dart'; -import 'environment.dart'; import 'integration_tests_manager.dart'; void main() async { From 8352a928735a5c531045978871ce2e7ed315f0b7 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 18 Mar 2020 16:31:53 -0700 Subject: [PATCH 11/22] addressing more reviewer comments --- lib/web_ui/dev/integration_tests_manager.dart | 101 +++++++++--------- lib/web_ui/dev/test_runner.dart | 2 +- 2 files changed, 51 insertions(+), 52 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 57abe00cdbc67..4d916e0c0cef5 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -26,7 +26,7 @@ class IntegrationTestsManager { : this._driverDir = io.Directory( pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); - Future run() async { + Future runTests() async { if (_browser != 'chrome') { print('WARNING: integration tests are only supported on chrome for now'); return false; @@ -45,7 +45,7 @@ class IntegrationTestsManager { } } - Future _cloneWebInstalllers() async { + Future _cloneWebInstallers() async { final int exitCode = await runProcess( 'git', [ @@ -65,14 +65,19 @@ class IntegrationTestsManager { } Future _runPubGet(String workingDirectory) async { - // TODO(nurhan): Run flutter pub get from the fetched flutter on CI. + final String executable = + (isCirrus != null && isCirrus) ? environment.pubExecutable : 'flutter'; + final List arguments = (isCirrus != null && isCirrus) + ? [ + 'get', + ] + : [ + 'pub', + 'get', + ]; final int exitCode = await runProcess( - //environment.pubExecutable, - 'flutter', - [ - 'pub', - 'get', - ], + executable, + arguments, workingDirectory: workingDirectory, ); @@ -120,7 +125,7 @@ class IntegrationTestsManager { _driverDir.createSync(recursive: true); - bool installWebInstallers = await _cloneWebInstalllers(); + bool installWebInstallers = await _cloneWebInstallers(); if (installWebInstallers) { bool pubGet = await _runPubGet(pathlib.join( _driverDir.path, 'web_installers', 'packages', 'web_drivers')); @@ -133,7 +138,7 @@ class IntegrationTestsManager { return false; } - /// Runs the all the web tests under e2e_tests/web. + /// Runs all the web tests under e2e_tests/web. Future _runTests() async { // Only list the files under e2e_tests/web. final List entities = @@ -141,14 +146,14 @@ class IntegrationTestsManager { print('INFO: Listing test files under directory: ' '${environment.integrationTestsDir.path.toString()}'); - bool testResults = true; + bool allTestsPassed = true; for (io.FileSystemEntity e in entities) { // The tests should be under this directories. if (e is io.Directory) { - testResults = testResults && await _runTestsInTheDirectory(e); + allTestsPassed = allTestsPassed && await _validateAndRunTests(e); } } - return testResults; + return allTestsPassed; } /// Run tests in a single directory under: e2e_tests/web. @@ -158,22 +163,20 @@ class IntegrationTestsManager { /// Validate the directory before running the tests. Each directory is /// expected to be a test project which includes a `pubspec.yaml` file /// and a `test_driver` directory. - Future _runTestsInTheDirectory(io.Directory directory) async { - final bool directoryContentValid = _validateTestDirectory(directory); - if (directoryContentValid) { - await _runPubGet(directory.path); - final bool testResults = await _runTestsInDirectory(directory); - return testResults; - } else { - return false; - } + Future _validateAndRunTests(io.Directory directory) async { + _validateTestDirectory(directory); + await _runPubGet(directory.path); + final bool testResults = await _runTestsInDirectory(directory); + return testResults; } Future _runTestsInDirectory(io.Directory directory) async { final io.Directory testDirectory = io.Directory(pathlib.join(directory.path, 'test_driver')); - final List entities = - testDirectory.listSync(followLinks: false); + final List entities = testDirectory + .listSync(followLinks: false) + .whereType() + .toList(); final List e2eTestsToRun = List(); @@ -195,7 +198,8 @@ class IntegrationTestsManager { int numberOfPassedTests = 0; int numberOfFailedTests = 0; for (String fileName in e2eTestsToRun) { - final bool testResults = await _runTestsInDebugMode(directory, fileName); + final bool testResults = + await _runTestsInProfileMode(directory, fileName); if (testResults) { numberOfPassedTests++; } else { @@ -204,18 +208,13 @@ class IntegrationTestsManager { } final int numberOfTestsRun = numberOfPassedTests + numberOfFailedTests; - print('${numberOfTestsRun} tests run ${numberOfPassedTests} passed and ' - '${numberOfFailedTests} failed.'); + print('INFO: ${numberOfTestsRun} tests run. ${numberOfPassedTests} passed ' + 'and ${numberOfFailedTests} failed.'); return numberOfFailedTests == 0; } - Future _runTestsInDebugMode( + Future _runTestsInProfileMode( io.Directory directory, String testName) async { - final String statementToRun = 'flutter drive ' - '--target=test_driver/${testName} -d web-server --release ' - '--browser-name=$_browser --local-engine=host_debug_unopt'; - print('INFO: To manually run the test use $statementToRun under ' - 'directory ${directory.path}'); // TODO(nurhan): Give options to the developer to run tests in another mode. final int exitCode = await runProcess( 'flutter', @@ -232,9 +231,13 @@ class IntegrationTestsManager { ); if (exitCode != 0) { + final String statementToRun = 'flutter drive ' + '--target=test_driver/${testName} -d web-server --profile ' + '--browser-name=$_browser --local-engine=host_debug_unopt'; io.stderr - .writeln('ERROR: Failed to run test. Exited with exit code $exitCode.' - ' Statement to run $statementToRun'); + .writeln('ERROR: Failed to run test. Exited with exit code $exitCode' + '. Statement to run $testName locally use the following ' + 'command:\n\n$statementToRun'); return false; } else { return true; @@ -246,7 +249,7 @@ class IntegrationTestsManager { /// /// Also check the validity of files under `test_driver` directory calling /// [_checkE2ETestsValidity] method. - bool _validateTestDirectory(io.Directory directory) { + void _validateTestDirectory(io.Directory directory) { final List entities = directory.listSync(followLinks: false); @@ -266,16 +269,15 @@ class IntegrationTestsManager { } } if (!pubSpecFound) { - io.stderr - .writeln('ERROR: pubspec.yaml file not found in the test project.'); - return false; + throw StateError('ERROR: pubspec.yaml file not found in the test project ' + 'in the directory ${directory.path}.'); } if (testDirectory == null) { - io.stderr - .writeln('ERROR: test_driver folder not found in the test project.'); - return false; + throw StateError( + 'ERROR: test_driver folder not found in the test project.' + 'in the directory ${directory.path}.'); } else { - return _checkE2ETestsValidity(testDirectory); + _checkE2ETestsValidity(testDirectory); } } @@ -286,7 +288,7 @@ class IntegrationTestsManager { /// For each e2e test which has name {name}.dart there will be a driver /// file which drives it. The driver file should be named: /// {name}_test.dart - bool _checkE2ETestsValidity(io.Directory testDirectory) { + void _checkE2ETestsValidity(io.Directory testDirectory) { final List entities = testDirectory.listSync(followLinks: false); @@ -299,7 +301,6 @@ class IntegrationTestsManager { // not ending with `_test.dart` is an e2e test. for (io.File f in entities) { final String basename = pathlib.basename(f.path); - print('basename: $basename'); if (basename.contains('_test.dart')) { // First remove this from expectedSet if not there add to the foundSet. if (expectedDriverFileNames.contains(basename)) { @@ -309,8 +310,7 @@ class IntegrationTestsManager { } } else if (basename.contains('.dart')) { // Only run on dart files. - final String e2efileName = - basename.substring(0, basename.indexOf('.dart')); + final String e2efileName = pathlib.basenameWithoutExtension(f.path); final String expectedDriverName = '${e2efileName}_test.dart'; numberOfTests++; // First remove this from foundSet if not there add to the expectedSet. @@ -336,9 +336,8 @@ class IntegrationTestsManager { 'integration tests. Please add ${expectedDriverName}. Check to ' 'README file on more details on how to setup integration tests.'); } - return false; - } else { - return true; + throw StateError('Error in test files. Check the logs for ' + 'further instructions'); } } } diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index b5d988e649142..ea239f70b441d 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -132,7 +132,7 @@ class TestCommand extends Command { } Future runIntegrationTests() async { - return IntegrationTestsManager(browser).run(); + return IntegrationTestsManager(browser).runTests(); } Future runUnitTests() async { From 9533b4bd43f7bf7065250a72d566f239a9bdb0a9 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 18 Mar 2020 17:46:01 -0700 Subject: [PATCH 12/22] addressing more reviewer comments --- lib/web_ui/dev/integration_tests_manager.dart | 10 +++------ lib/web_ui/dev/test_runner.dart | 21 +++++++------------ lib/web_ui/dev/utils.dart | 5 ++--- 3 files changed, 13 insertions(+), 23 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 4d916e0c0cef5..37438dd7da8d3 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -107,7 +107,7 @@ class IntegrationTestsManager { 'ERROR: Failed to run driver. Exited with exit code $exitCode'); return false; } else { - runProcessInTheBackground( + startDetachedProcess( './chromedriver/chromedriver', ['--port=4444'], workingDirectory: pathlib.join( @@ -303,9 +303,7 @@ class IntegrationTestsManager { final String basename = pathlib.basename(f.path); if (basename.contains('_test.dart')) { // First remove this from expectedSet if not there add to the foundSet. - if (expectedDriverFileNames.contains(basename)) { - expectedDriverFileNames.remove(basename); - } else { + if (!expectedDriverFileNames.remove(basename)) { foundDriverFileNames.add(basename); } } else if (basename.contains('.dart')) { @@ -314,9 +312,7 @@ class IntegrationTestsManager { final String expectedDriverName = '${e2efileName}_test.dart'; numberOfTests++; // First remove this from foundSet if not there add to the expectedSet. - if (foundDriverFileNames.contains(expectedDriverName)) { - foundDriverFileNames.remove(expectedDriverName); - } else { + if(!foundDriverFileNames.remove(expectedDriverName)) { expectedDriverFileNames.add(expectedDriverName); } } diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index ea239f70b441d..77a51d0caf89e 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -31,9 +31,6 @@ enum TestTypesRequested { /// For running both unit and integration tests. all, - - /// Run no tests. - none, } class TestCommand extends Command { @@ -83,9 +80,9 @@ class TestCommand extends Command { @override final String description = 'Run tests.'; - TestTypesRequested testTypesRequested = TestTypesRequested.none; + TestTypesRequested testTypesRequested = null; - /// Check the flags to see what type of integration tests are requested. + /// Check the flags to see what type of tests are requested. TestTypesRequested findTestType() { if (argResults['unit-tests-only'] && argResults['integration-tests-only']) { throw ArgumentError('Conflicting arguments: unit-tests-only and ' @@ -94,11 +91,9 @@ class TestCommand extends Command { print('Running the unit tests only'); return TestTypesRequested.unit; } else if (argResults['integration-tests-only']) { - print('Running the integration tests only. Note that they are only ' - 'available for Chrome Desktop.'); if (!isChrome) { throw UnimplementedError( - 'Integration tests are only awailable on Chrome Desktop for now'); + 'Integration tests are only available on Chrome Desktop for now'); } return TestTypesRequested.integration; } else { @@ -122,17 +117,17 @@ class TestCommand extends Command { case TestTypesRequested.all: bool integrationTestResult = await runIntegrationTests(); bool unitTestResult = await runUnitTests(); - print('Tests run. Integration tests passed: $integrationTestResult ' - 'unit tests passed: $unitTestResult'); + if (integrationTestResult != unitTestResult) { + print('Tests run. Integration tests passed: $integrationTestResult ' + 'unit tests passed: $unitTestResult'); + } return integrationTestResult && unitTestResult; - case TestTypesRequested.none: - throw ArgumentError('One test type should be chosed to run felt test.'); } return false; } Future runIntegrationTests() async { - return IntegrationTestsManager(browser).runTests(); + return IntegrationTestsManager(browser).runTests(); } Future runUnitTests() async { diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart index 1588584a06287..e683e1aca973d 100644 --- a/lib/web_ui/dev/utils.dart +++ b/lib/web_ui/dev/utils.dart @@ -63,13 +63,12 @@ Future runProcess( } /// Runs [executable]. Do not follow the exit code or the output. -void runProcessInTheBackground( +void startDetachedProcess( String executable, List arguments, { String workingDirectory, bool mustSucceed: false, }) async { - arguments.add('&'); io.Process.start( executable, arguments, @@ -77,7 +76,7 @@ void runProcessInTheBackground( // Running the process in a system shell for Windows. Otherwise // the process is not able to get Dart from path. runInShell: io.Platform.isWindows, - mode: io.ProcessStartMode.inheritStdio, + mode: io.ProcessStartMode.detached, ); } From 0236d6115fdaee37be04f8dd842e6dbb1cc83d88 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 11:23:03 -0700 Subject: [PATCH 13/22] addressing reviewer comments. --- lib/web_ui/dev/common.dart | 5 ++++ lib/web_ui/dev/felt.dart | 8 +++++++ lib/web_ui/dev/integration_tests_manager.dart | 16 ++++++------- .../dev/integration_tests_manager_test.dart | 23 ------------------- lib/web_ui/dev/utils.dart | 8 ++++--- 5 files changed, 26 insertions(+), 34 deletions(-) delete mode 100644 lib/web_ui/dev/integration_tests_manager_test.dart diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index 9fa8d126c86ae..988c66583053a 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -243,3 +243,8 @@ class DevNull implements StringSink { } bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; + +/// There might be proccessed started during the tests. +/// +/// Use this list to store those Processes, for cleaning up before shutdown. +final List processesToCleanUp = List(); diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index d192431791b39..e3f29f3ebe4ed 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -9,6 +9,7 @@ import 'package:args/command_runner.dart'; import 'build.dart'; import 'clean.dart'; +import 'common.dart'; import 'licenses.dart'; import 'test_runner.dart'; @@ -43,6 +44,13 @@ void main(List args) async { rethrow; } + // Cleanup remaining processes if any. + if(processesToCleanUp.length > 0) { + for(io.Process process in processesToCleanUp) { + process.kill(); + } + } + // Sometimes the Dart VM refuses to quit. io.exit(io.exitCode); } diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 37438dd7da8d3..3d869e12d9582 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -34,9 +34,8 @@ class IntegrationTestsManager { bool driverReady = await prepareDriver(); // TODO(nurhan): provide a flag for running as if on CI. Also use this // flag from LUCI. - if (isCirrus != null && isCirrus) { - // TODO(nurhan): fetch Flutter, we will use flutter pub get in the next steps. - } + // TODO(nurhan): if we are running on cirrus. fetch Flutter, we will use + // flutter pub get in the next steps. if (driverReady) { return await _runTests(); } else { @@ -65,9 +64,10 @@ class IntegrationTestsManager { } Future _runPubGet(String workingDirectory) async { - final String executable = - (isCirrus != null && isCirrus) ? environment.pubExecutable : 'flutter'; - final List arguments = (isCirrus != null && isCirrus) + final String executable = isCirrus + ? environment.pubExecutable + : 'flutter'; + final List arguments = isCirrus ? [ 'get', ] @@ -107,7 +107,7 @@ class IntegrationTestsManager { 'ERROR: Failed to run driver. Exited with exit code $exitCode'); return false; } else { - startDetachedProcess( + startProcess( './chromedriver/chromedriver', ['--port=4444'], workingDirectory: pathlib.join( @@ -312,7 +312,7 @@ class IntegrationTestsManager { final String expectedDriverName = '${e2efileName}_test.dart'; numberOfTests++; // First remove this from foundSet if not there add to the expectedSet. - if(!foundDriverFileNames.remove(expectedDriverName)) { + if (!foundDriverFileNames.remove(expectedDriverName)) { expectedDriverFileNames.add(expectedDriverName); } } diff --git a/lib/web_ui/dev/integration_tests_manager_test.dart b/lib/web_ui/dev/integration_tests_manager_test.dart deleted file mode 100644 index 880ec6a6379da..0000000000000 --- a/lib/web_ui/dev/integration_tests_manager_test.dart +++ /dev/null @@ -1,23 +0,0 @@ -// 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. - -@TestOn('vm && linux') - -import 'package:test/test.dart'; - -import 'integration_tests_manager.dart'; - -void main() async { - IntegrationTestsManager testsManager; - setUpAll(() { - testsManager = IntegrationTestsManager('chrome'); - }); - - tearDown(() { - }); - - test('run an install chrome driver', () async { - testsManager.prepareDriver(); - }); -} diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart index e683e1aca973d..5b7e6f306d9a5 100644 --- a/lib/web_ui/dev/utils.dart +++ b/lib/web_ui/dev/utils.dart @@ -9,6 +9,7 @@ import 'dart:io' as io; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; +import 'common.dart'; import 'environment.dart'; class FilePath { @@ -63,21 +64,22 @@ Future runProcess( } /// Runs [executable]. Do not follow the exit code or the output. -void startDetachedProcess( +void startProcess( String executable, List arguments, { String workingDirectory, bool mustSucceed: false, }) async { - io.Process.start( + final io.Process process = await io.Process.start( executable, arguments, workingDirectory: workingDirectory, // Running the process in a system shell for Windows. Otherwise // the process is not able to get Dart from path. runInShell: io.Platform.isWindows, - mode: io.ProcessStartMode.detached, + mode: io.ProcessStartMode.inheritStdio, ); + processesToCleanUp.add(process); } /// Runs [executable] and returns its standard output as a string. From 1bfce16d89eb12a7dfa472211f06d3ecea5015f6 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 11:35:20 -0700 Subject: [PATCH 14/22] addressing reviewer comments. --- lib/web_ui/dev/integration_tests_manager.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 3d869e12d9582..e1e12a74d06ed 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -64,9 +64,7 @@ class IntegrationTestsManager { } Future _runPubGet(String workingDirectory) async { - final String executable = isCirrus - ? environment.pubExecutable - : 'flutter'; + final String executable = isCirrus ? environment.pubExecutable : 'flutter'; final List arguments = isCirrus ? [ 'get', @@ -289,8 +287,16 @@ class IntegrationTestsManager { /// file which drives it. The driver file should be named: /// {name}_test.dart void _checkE2ETestsValidity(io.Directory testDirectory) { - final List entities = - testDirectory.listSync(followLinks: false); + final List directories = + testDirectory.listSync(followLinks: false).whereType(); + + if (directories.length > 0) { + throw StateError('${testDirectory.path} directory should not containd ' + 'any sub-sriectories'); + } + + final List entities = + testDirectory.listSync(followLinks: false).whereType(); final Set expectedDriverFileNames = Set(); final Set foundDriverFileNames = Set(); From 0df2e9df1ee147fcc5e102d4824d5cd3cc9bd3e6 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 12:17:55 -0700 Subject: [PATCH 15/22] fixing typos --- lib/web_ui/dev/common.dart | 2 +- lib/web_ui/dev/integration_tests_manager.dart | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index 988c66583053a..4a0960a1c1372 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -244,7 +244,7 @@ class DevNull implements StringSink { bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; -/// There might be proccessed started during the tests. +/// There might be proccesses started during the tests. /// /// Use this list to store those Processes, for cleaning up before shutdown. final List processesToCleanUp = List(); diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index e1e12a74d06ed..c7dc067cb9873 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -291,8 +291,8 @@ class IntegrationTestsManager { testDirectory.listSync(followLinks: false).whereType(); if (directories.length > 0) { - throw StateError('${testDirectory.path} directory should not containd ' - 'any sub-sriectories'); + throw StateError('${testDirectory.path} directory should not contain ' + 'any sub-directories'); } final List entities = From 19c399ca633cf2a2116893a082b1ec9ca873b43b Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 14:27:49 -0700 Subject: [PATCH 16/22] using chromedriverinstaller as a library. Fixing the commit number used from web_installers repo --- lib/web_ui/dev/integration_tests_manager.dart | 19 ++++++++++++++----- lib/web_ui/pubspec.yaml | 5 +++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index c7dc067cb9873..e2ea5316adc6d 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -4,6 +4,7 @@ import 'dart:io' as io; import 'package:path/path.dart' as pathlib; +import 'package:web_driver_installer/chrome_driver_installer.dart'; import 'common.dart'; import 'environment.dart'; @@ -117,17 +118,25 @@ class IntegrationTestsManager { } Future prepareDriver() async { + final io.Directory priorCurrentDirectory = io.Directory.current; if (_driverDir.existsSync()) { _driverDir.deleteSync(recursive: true); } _driverDir.createSync(recursive: true); + // TODO(nurhan): We currently need git clone for getting the driver lock + // file. Remove this after making changes in web_installers. bool installWebInstallers = await _cloneWebInstallers(); if (installWebInstallers) { - bool pubGet = await _runPubGet(pathlib.join( - _driverDir.path, 'web_installers', 'packages', 'web_drivers')); - if (pubGet) { + // Change the directory to the driver_lock.yaml file's directory. + io.Directory.current = pathlib.join( + _driverDir.path, 'web_installers', 'packages', 'web_drivers'); + ChromeDriverInstaller chromeDriverInstaller = ChromeDriverInstaller(); + bool installation = await chromeDriverInstaller.install(); + + if (installation) { + io.Directory.current = priorCurrentDirectory; return await _runDriver(); } else { return false; @@ -287,7 +296,7 @@ class IntegrationTestsManager { /// file which drives it. The driver file should be named: /// {name}_test.dart void _checkE2ETestsValidity(io.Directory testDirectory) { - final List directories = + final Iterable directories = testDirectory.listSync(followLinks: false).whereType(); if (directories.length > 0) { @@ -295,7 +304,7 @@ class IntegrationTestsManager { 'any sub-directories'); } - final List entities = + final Iterable entities = testDirectory.listSync(followLinks: false).whereType(); final Set expectedDriverFileNames = Set(); diff --git a/lib/web_ui/pubspec.yaml b/lib/web_ui/pubspec.yaml index 12e7d8d8f7d7a..997e119224c72 100644 --- a/lib/web_ui/pubspec.yaml +++ b/lib/web_ui/pubspec.yaml @@ -21,3 +21,8 @@ dev_dependencies: watcher: 0.9.7+12 web_engine_tester: path: ../../web_sdk/web_engine_tester + web_driver_installer: + git: + url: git://github.com/flutter/web_installers.git + path: packages/web_drivers/ + ref: dae38d8839cc39f997fb4229f1382680b8758b4f From d1d8abfadc1ae3dae9f9a99f6019e0d098ab50dd Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 15:09:18 -0700 Subject: [PATCH 17/22] clean drivers directory after tests finish --- lib/web_ui/dev/common.dart | 22 ++++++++----- lib/web_ui/dev/felt.dart | 20 ++++++++--- lib/web_ui/dev/integration_tests_manager.dart | 33 ++++++++++++------- 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index 4a0960a1c1372..b0419d64b8ed6 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -77,11 +77,10 @@ class _WindowsBinding implements PlatformBinding { @override String getFirefoxDownloadUrl(String version) => 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/${version}/win64/en-US/' - '${getFirefoxDownloadFilename(version)}'; + '${getFirefoxDownloadFilename(version)}'; @override - String getFirefoxDownloadFilename(String version) => - 'firefox-${version}.exe'; + String getFirefoxDownloadFilename(String version) => 'firefox-${version}.exe'; @override String getFirefoxExecutablePath(io.Directory versionDir) => @@ -117,7 +116,7 @@ class _LinuxBinding implements PlatformBinding { @override String getFirefoxDownloadUrl(String version) => 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/${version}/linux-x86_64/en-US/' - '${getFirefoxDownloadFilename(version)}'; + '${getFirefoxDownloadFilename(version)}'; @override String getFirefoxDownloadFilename(String version) => @@ -161,16 +160,15 @@ class _MacBinding implements PlatformBinding { @override String getFirefoxDownloadUrl(String version) => - 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/${version}/mac/en-US/' - '${getFirefoxDownloadFilename(version)}'; + 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/${version}/mac/en-US/' + '${getFirefoxDownloadFilename(version)}'; @override - String getFirefoxDownloadFilename(String version) => - 'Firefox ${version}.dmg'; + String getFirefoxDownloadFilename(String version) => 'Firefox ${version}.dmg'; @override String getFirefoxExecutablePath(io.Directory versionDir) => - path.join(versionDir.path, 'Firefox.app','Contents','MacOS', 'firefox'); + path.join(versionDir.path, 'Firefox.app', 'Contents', 'MacOS', 'firefox'); @override String getFirefoxLatestVersionUrl() => @@ -248,3 +246,9 @@ bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; /// /// Use this list to store those Processes, for cleaning up before shutdown. final List processesToCleanUp = List(); + +/// There might be temporary directories created during the tests. +/// +/// Use this list to store those directories and for deleteing them before +/// shutdown. +final List temporaryDirectories = List(); diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index e3f29f3ebe4ed..fd4d2f6c73645 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -42,17 +42,27 @@ void main(List args) async { io.exit(64); // Exit code 64 indicates a usage error. } catch (e) { rethrow; + } finally { + _cleanup(); } + // Sometimes the Dart VM refuses to quit. + io.exit(io.exitCode); +} + +void _cleanup() { // Cleanup remaining processes if any. - if(processesToCleanUp.length > 0) { - for(io.Process process in processesToCleanUp) { + if (processesToCleanUp.length > 0) { + for (io.Process process in processesToCleanUp) { process.kill(); } } - - // Sometimes the Dart VM refuses to quit. - io.exit(io.exitCode); + // Delete temporary directories. + if (temporaryDirectories.length > 0) { + for (io.Directory directory in temporaryDirectories) { + directory.deleteSync(recursive: true); + } + } } void _listenToShutdownSignals() { diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index e2ea5316adc6d..6164f1aabef4c 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -21,11 +21,19 @@ class IntegrationTestsManager { /// same time. // TODO(nurhan): change the web installers to install driver and the browser // at the same time. - final io.Directory _driverDir; + final io.Directory _browserDriverDir; + + /// This is the parent directory for all drivers. + /// + /// This directory is saved to [temporaryDirectories] and deleted before + /// tests shutdown. + final io.Directory _drivers; IntegrationTestsManager(this._browser) - : this._driverDir = io.Directory( - pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)); + : this._browserDriverDir = io.Directory( + pathlib.join(environment.webUiRootDir.path, 'drivers', _browser)), + this._drivers = io.Directory( + pathlib.join(environment.webUiRootDir.path, 'drivers')); Future runTests() async { if (_browser != 'chrome') { @@ -52,7 +60,7 @@ class IntegrationTestsManager { 'clone', 'https://github.com/flutter/web_installers.git', ], - workingDirectory: _driverDir.path, + workingDirectory: _browserDriverDir.path, ); if (exitCode != 0) { @@ -98,7 +106,7 @@ class IntegrationTestsManager { '--install-only', ], workingDirectory: pathlib.join( - _driverDir.path, 'web_installers', 'packages', 'web_drivers'), + _browserDriverDir.path, 'web_installers', 'packages', 'web_drivers'), ); if (exitCode != 0) { @@ -109,8 +117,8 @@ class IntegrationTestsManager { startProcess( './chromedriver/chromedriver', ['--port=4444'], - workingDirectory: pathlib.join( - _driverDir.path, 'web_installers', 'packages', 'web_drivers'), + workingDirectory: pathlib.join(_browserDriverDir.path, 'web_installers', + 'packages', 'web_drivers'), ); print('INFO: Driver started'); return true; @@ -119,11 +127,12 @@ class IntegrationTestsManager { Future prepareDriver() async { final io.Directory priorCurrentDirectory = io.Directory.current; - if (_driverDir.existsSync()) { - _driverDir.deleteSync(recursive: true); + if (_browserDriverDir.existsSync()) { + _browserDriverDir.deleteSync(recursive: true); } - _driverDir.createSync(recursive: true); + _browserDriverDir.createSync(recursive: true); + temporaryDirectories.add(_drivers); // TODO(nurhan): We currently need git clone for getting the driver lock // file. Remove this after making changes in web_installers. @@ -131,12 +140,12 @@ class IntegrationTestsManager { if (installWebInstallers) { // Change the directory to the driver_lock.yaml file's directory. io.Directory.current = pathlib.join( - _driverDir.path, 'web_installers', 'packages', 'web_drivers'); + _browserDriverDir.path, 'web_installers', 'packages', 'web_drivers'); ChromeDriverInstaller chromeDriverInstaller = ChromeDriverInstaller(); bool installation = await chromeDriverInstaller.install(); if (installation) { - io.Directory.current = priorCurrentDirectory; + io.Directory.current = priorCurrentDirectory; return await _runDriver(); } else { return false; From 1565a2b254383a9712d98a1111744a8089fe2804 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 15:17:11 -0700 Subject: [PATCH 18/22] addressing more reviewer comments --- lib/web_ui/dev/integration_tests_manager.dart | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 6164f1aabef4c..d84735e4a515c 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -160,8 +160,6 @@ class IntegrationTestsManager { final List entities = environment.integrationTestsDir.listSync(followLinks: false); - print('INFO: Listing test files under directory: ' - '${environment.integrationTestsDir.path.toString()}'); bool allTestsPassed = true; for (io.FileSystemEntity e in entities) { // The tests should be under this directories. @@ -343,7 +341,8 @@ class IntegrationTestsManager { } if (numberOfTests == 0) { - print('WARNING: No tests to run in this directory.'); + throw StateError( + 'WARNING: No tests to run in this directory ${testDirectory.path}'); } // TODO(nurhan): In order to reduce the work required from team members, From 790d986216595e86115a37c404e5482edfb458c7 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 15:44:19 -0700 Subject: [PATCH 19/22] throwing all critical exceptions instead of logging and returning boolean flags --- lib/web_ui/dev/common.dart | 9 +++ lib/web_ui/dev/integration_tests_manager.dart | 66 ++++++++----------- 2 files changed, 38 insertions(+), 37 deletions(-) diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index b0419d64b8ed6..37b8e2bf93473 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -27,6 +27,15 @@ class BrowserInstallerException implements Exception { String toString() => message; } +class DriverException implements Exception { + DriverException(this.message); + + final String message; + + @override + String toString() => message; +} + abstract class PlatformBinding { static PlatformBinding get instance { if (_instance == null) { diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index d84735e4a515c..2cff865469be7 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -40,20 +40,16 @@ class IntegrationTestsManager { print('WARNING: integration tests are only supported on chrome for now'); return false; } else { - bool driverReady = await prepareDriver(); + await prepareDriver(); // TODO(nurhan): provide a flag for running as if on CI. Also use this // flag from LUCI. // TODO(nurhan): if we are running on cirrus. fetch Flutter, we will use // flutter pub get in the next steps. - if (driverReady) { - return await _runTests(); - } else { - return false; - } + return await _runTests(); } } - Future _cloneWebInstallers() async { + void _cloneWebInstallers() async { final int exitCode = await runProcess( 'git', [ @@ -66,9 +62,8 @@ class IntegrationTestsManager { if (exitCode != 0) { io.stderr.writeln('ERROR: ' 'Failed to clone web installers. Exited with exit code $exitCode'); - return false; - } else { - return true; + throw DriverException('ERROR: ' + 'Failed to clone web installers. Exited with exit code $exitCode'); } } @@ -97,7 +92,7 @@ class IntegrationTestsManager { } } - Future _runDriver() async { + void _runDriver() async { final int exitCode = await runProcess( environment.dartExecutable, [ @@ -112,20 +107,19 @@ class IntegrationTestsManager { if (exitCode != 0) { io.stderr.writeln( 'ERROR: Failed to run driver. Exited with exit code $exitCode'); - return false; - } else { - startProcess( - './chromedriver/chromedriver', - ['--port=4444'], - workingDirectory: pathlib.join(_browserDriverDir.path, 'web_installers', - 'packages', 'web_drivers'), - ); - print('INFO: Driver started'); - return true; + throw DriverException( + 'ERROR: Failed to run driver. Exited with exit code $exitCode'); } + startProcess( + './chromedriver/chromedriver', + ['--port=4444'], + workingDirectory: pathlib.join( + _browserDriverDir.path, 'web_installers', 'packages', 'web_drivers'), + ); + print('INFO: Driver started'); } - Future prepareDriver() async { + void prepareDriver() async { final io.Directory priorCurrentDirectory = io.Directory.current; if (_browserDriverDir.existsSync()) { _browserDriverDir.deleteSync(recursive: true); @@ -136,22 +130,20 @@ class IntegrationTestsManager { // TODO(nurhan): We currently need git clone for getting the driver lock // file. Remove this after making changes in web_installers. - bool installWebInstallers = await _cloneWebInstallers(); - if (installWebInstallers) { - // Change the directory to the driver_lock.yaml file's directory. - io.Directory.current = pathlib.join( - _browserDriverDir.path, 'web_installers', 'packages', 'web_drivers'); - ChromeDriverInstaller chromeDriverInstaller = ChromeDriverInstaller(); - bool installation = await chromeDriverInstaller.install(); - - if (installation) { - io.Directory.current = priorCurrentDirectory; - return await _runDriver(); - } else { - return false; - } + await _cloneWebInstallers(); + // Change the directory to the driver_lock.yaml file's directory. + io.Directory.current = pathlib.join( + _browserDriverDir.path, 'web_installers', 'packages', 'web_drivers'); + // Chrome is the only browser supporting integration tests for now. + ChromeDriverInstaller chromeDriverInstaller = ChromeDriverInstaller(); + bool installation = await chromeDriverInstaller.install(); + + if (installation) { + io.Directory.current = priorCurrentDirectory; + await _runDriver(); + } else { + throw DriverException('ERROR: Installing driver failed'); } - return false; } /// Runs all the web tests under e2e_tests/web. From 18d66d37d4f074e1ff378ddeb8808ac2abd7e798 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 15:59:12 -0700 Subject: [PATCH 20/22] adding todos for items we can do for making felt easier to use for local development --- lib/web_ui/dev/integration_tests_manager.dart | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 2cff865469be7..5782b730025d7 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -121,6 +121,8 @@ class IntegrationTestsManager { void prepareDriver() async { final io.Directory priorCurrentDirectory = io.Directory.current; + // TODO(nurhan): Add a flag for using the existing driver. + // This will can be used to speed up local development. if (_browserDriverDir.existsSync()) { _browserDriverDir.deleteSync(recursive: true); } @@ -186,6 +188,9 @@ class IntegrationTestsManager { final List e2eTestsToRun = List(); + // TODO(nurhan): Add an option to only run one test. This can be useful for + // local development. + // The following loops over the contents of the directory and saves an // expected driver file name for each e2e test assuming any dart file // not ending with `_test.dart` is an e2e test. From 1f14972937407b35af0fcbe1fb3e00f1710a09e8 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 16:10:34 -0700 Subject: [PATCH 21/22] do not run integration tests on CIs. Added an issue to the TODO. --- lib/web_ui/dev/integration_tests_manager.dart | 4 ---- lib/web_ui/dev/test_runner.dart | 5 +++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 5782b730025d7..fcd50ed790780 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -41,10 +41,6 @@ class IntegrationTestsManager { return false; } else { await prepareDriver(); - // TODO(nurhan): provide a flag for running as if on CI. Also use this - // flag from LUCI. - // TODO(nurhan): if we are running on cirrus. fetch Flutter, we will use - // flutter pub get in the next steps. return await _runTests(); } } diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 77a51d0caf89e..fa66b05954d3c 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -127,6 +127,11 @@ class TestCommand extends Command { } Future runIntegrationTests() async { + // TODO(nurhan): https://github.com/flutter/flutter/issues/52983 + if (io.Platform.environment['LUCI_CONTEXT'] != null || isCirrus) { + return true; + } + return IntegrationTestsManager(browser).runTests(); } From 4f2abdedb706dc237ce4536c37f87e5232b0d9bd Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 20 Mar 2020 16:19:41 -0700 Subject: [PATCH 22/22] changing todo's with issues. --- lib/web_ui/dev/integration_tests_manager.dart | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index fcd50ed790780..bd97ec878a7ef 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -41,6 +41,7 @@ class IntegrationTestsManager { return false; } else { await prepareDriver(); + // TODO(nurhan): https://github.com/flutter/flutter/issues/52987 return await _runTests(); } } @@ -117,8 +118,6 @@ class IntegrationTestsManager { void prepareDriver() async { final io.Directory priorCurrentDirectory = io.Directory.current; - // TODO(nurhan): Add a flag for using the existing driver. - // This will can be used to speed up local development. if (_browserDriverDir.existsSync()) { _browserDriverDir.deleteSync(recursive: true); } @@ -184,9 +183,6 @@ class IntegrationTestsManager { final List e2eTestsToRun = List(); - // TODO(nurhan): Add an option to only run one test. This can be useful for - // local development. - // The following loops over the contents of the directory and saves an // expected driver file name for each e2e test assuming any dart file // not ending with `_test.dart` is an e2e test. @@ -222,7 +218,6 @@ class IntegrationTestsManager { Future _runTestsInProfileMode( io.Directory directory, String testName) async { - // TODO(nurhan): Give options to the developer to run tests in another mode. final int exitCode = await runProcess( 'flutter', [