From afd35da03cf066a1b83e07c0be286ae8a75326b9 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 8 Apr 2020 11:26:52 -0700 Subject: [PATCH 01/15] changing felt script to fetch flutter --- lib/web_ui/dev/felt | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 56b031ce0b616..93772abb44cc1 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -4,6 +4,9 @@ set -e # felt: a command-line utility for building and testing Flutter web engine. # It stands for Flutter Engine Local Tester. +echo "engine path" +echo $ENGINE_PATH + FELT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" if [ -z "`which gclient`" ] @@ -24,6 +27,7 @@ NINJA_PATH=`which ninja` ENGINE_SRC_DIR="$(dirname $(dirname $(dirname $(dirname ${FELT_DIR}))))" FLUTTER_DIR="${ENGINE_SRC_DIR}/flutter" +FLUTTER_CLONE_SCRIPT="${ENGINE_SRC_DIR}/flutter/tools/clone_flutter.sh" WEB_UI_DIR="${FLUTTER_DIR}/lib/web_ui" DEV_DIR="${WEB_UI_DIR}/dev" OUT_DIR="${ENGINE_SRC_DIR}/out" @@ -61,6 +65,13 @@ then ulimit -u 4096 fi +if [[ is_ci = true ]] +then + echo "Clone flutter to use flutter driver" + # TODO: if runs on cirrus. fetch the flutter framework. + $FLUTTER_CLONE_SCRIPT +fi + if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then echo "[Snapshot mode: off]" # Running without snapshot means there is high likelyhood of local changes. In @@ -86,3 +97,22 @@ else $DART_SDK_DIR/bin/dart --packages="$WEB_UI_DIR/.packages" "$SNAPSHOT_PATH" $@ fi + +is_cirrus() { + if [[ "$CIRRUS_CI" = true ]] + then + echo "Running on CI environement." + return 1 + else + echo "Running on local environement." + return 0 + fi +} + +# is_luci() { + +# } + +# is_ci() { + +# } From 8324ebf352fe6d8f2029074bd36fa1d2adf542d3 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 8 Apr 2020 15:42:43 -0700 Subject: [PATCH 02/15] changing the clone_flutter.sh code --- lib/web_ui/dev/clone_flutter_repo.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 lib/web_ui/dev/clone_flutter_repo.sh diff --git a/lib/web_ui/dev/clone_flutter_repo.sh b/lib/web_ui/dev/clone_flutter_repo.sh new file mode 100644 index 0000000000000..ad3a2ff490232 --- /dev/null +++ b/lib/web_ui/dev/clone_flutter_repo.sh @@ -0,0 +1,25 @@ +#!/bin/bash +set -e +set -x + +FLUTTER_CLONE_REPO=$1 + +if [[ -z $FLUTTER_CLONE_REPO ]] +then + if [[ -z $FRAMEWORK_PATH ]] + then + echo "Framework path should be set to run the script." + exit 1 + fi + # Do rest of the task in the root directory + cd ~ + mkdir -p $FRAMEWORK_PATH + cd $FRAMEWORK_PATH +else + cd $FLUTTER_CLONE_REPO +fi + + +# Clone the Flutter Framework. +git clone https://github.com/flutter/flutter.git +cd flutter From 20ebc73e17c637bf462f576fc5297e718c5784d6 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 8 Apr 2020 17:53:23 -0700 Subject: [PATCH 03/15] running integration tests with felt on cirrus. fetch framework in CI (not in local). --- lib/web_ui/dev/clone_flutter_repo.sh | 25 ----------- lib/web_ui/dev/common.dart | 4 ++ lib/web_ui/dev/environment.dart | 23 +++++++++++ lib/web_ui/dev/felt | 27 ------------ lib/web_ui/dev/integration_tests_manager.dart | 41 ++++++++++++------- lib/web_ui/dev/test_runner.dart | 2 +- tools/clone_flutter.sh | 23 +++++++++-- 7 files changed, 73 insertions(+), 72 deletions(-) delete mode 100644 lib/web_ui/dev/clone_flutter_repo.sh diff --git a/lib/web_ui/dev/clone_flutter_repo.sh b/lib/web_ui/dev/clone_flutter_repo.sh deleted file mode 100644 index ad3a2ff490232..0000000000000 --- a/lib/web_ui/dev/clone_flutter_repo.sh +++ /dev/null @@ -1,25 +0,0 @@ -#!/bin/bash -set -e -set -x - -FLUTTER_CLONE_REPO=$1 - -if [[ -z $FLUTTER_CLONE_REPO ]] -then - if [[ -z $FRAMEWORK_PATH ]] - then - echo "Framework path should be set to run the script." - exit 1 - fi - # Do rest of the task in the root directory - cd ~ - mkdir -p $FRAMEWORK_PATH - cd $FRAMEWORK_PATH -else - cd $FLUTTER_CLONE_REPO -fi - - -# Clone the Flutter Framework. -git clone https://github.com/flutter/flutter.git -cd flutter diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index 344316be76511..039fd83030629 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -232,3 +232,7 @@ class DevNull implements StringSink { } bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; + +bool get isLuci => io.Platform.environment['LUCI_CONTEXT'] != null; + +bool get isCi => isCirrus || isLuci; diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 3214c8a083b3d..814ffeb3fd19e 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -20,6 +20,7 @@ class Environment { factory Environment() { final io.File self = io.File.fromUri(io.Platform.script); final io.Directory engineSrcDir = self.parent.parent.parent.parent.parent; + final io.Directory engineToolsDir = io.Directory(pathlib.join(engineSrcDir.path, 'flutter', 'tools')); final io.Directory outDir = io.Directory(pathlib.join(engineSrcDir.path, 'out')); 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')); @@ -36,6 +37,7 @@ class Environment { self: self, webUiRootDir: webUiRootDir, engineSrcDir: engineSrcDir, + engineToolsDir: engineToolsDir, integrationTestsDir: integrationTestsDir, outDir: outDir, hostDebugUnoptDir: hostDebugUnoptDir, @@ -47,6 +49,7 @@ class Environment { this.self, this.webUiRootDir, this.engineSrcDir, + this.engineToolsDir, this.integrationTestsDir, this.outDir, this.hostDebugUnoptDir, @@ -62,6 +65,9 @@ class Environment { /// Path to the engine's "src" directory. final io.Directory engineSrcDir; + /// Path to the engine's "tools" directory. + final io.Directory engineToolsDir; + /// Path to the web integration tests. final io.Directory integrationTestsDir; @@ -124,4 +130,21 @@ class Environment { webUiDartToolDir.path, 'goldens', )); + + /// Path to the script that clones the Flutter repo. + io.File get cloneScript => io.File(pathlib.join( + engineToolsDir.path, + 'clone_flutter.sh', + )); + + /// Path to flutter to be used for `flutter pub get`. + /// + /// Only use [cloneScript] to clone flutter to the engine repo. + io.File get flutterCommandDir => io.File(pathlib.join( + webUiDartToolDir.path, + 'flutter' + 'bin', + 'flutter', + )); + } diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 93772abb44cc1..32808d45d3f0b 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -27,7 +27,6 @@ NINJA_PATH=`which ninja` ENGINE_SRC_DIR="$(dirname $(dirname $(dirname $(dirname ${FELT_DIR}))))" FLUTTER_DIR="${ENGINE_SRC_DIR}/flutter" -FLUTTER_CLONE_SCRIPT="${ENGINE_SRC_DIR}/flutter/tools/clone_flutter.sh" WEB_UI_DIR="${FLUTTER_DIR}/lib/web_ui" DEV_DIR="${WEB_UI_DIR}/dev" OUT_DIR="${ENGINE_SRC_DIR}/out" @@ -65,13 +64,6 @@ then ulimit -u 4096 fi -if [[ is_ci = true ]] -then - echo "Clone flutter to use flutter driver" - # TODO: if runs on cirrus. fetch the flutter framework. - $FLUTTER_CLONE_SCRIPT -fi - if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then echo "[Snapshot mode: off]" # Running without snapshot means there is high likelyhood of local changes. In @@ -97,22 +89,3 @@ else $DART_SDK_DIR/bin/dart --packages="$WEB_UI_DIR/.packages" "$SNAPSHOT_PATH" $@ fi - -is_cirrus() { - if [[ "$CIRRUS_CI" = true ]] - then - echo "Running on CI environement." - return 1 - else - echo "Running on local environement." - return 0 - fi -} - -# is_luci() { - -# } - -# is_ci() { - -# } diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 811e876664541..eb27bf5214857 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -47,18 +47,17 @@ class IntegrationTestsManager { } Future _runPubGet(String workingDirectory) async { - final String executable = isCirrus ? environment.pubExecutable : 'flutter'; - final List arguments = isCirrus - ? [ - 'get', - ] - : [ - 'pub', - 'get', - ]; + if (isCi) { + await _cloneFlutterRepo(); + } + final String executable = + isCi ? environment.flutterCommandDir.path : 'flutter'; final int exitCode = await runProcess( executable, - arguments, + [ + 'pub', + 'get', + ], workingDirectory: workingDirectory, ); @@ -71,12 +70,24 @@ class IntegrationTestsManager { } } - void _runDriver() async { - startProcess( - './chromedriver/chromedriver', - ['--port=4444'], - workingDirectory: io.Directory.current.path + Future _cloneFlutterRepo() async { + final int exitCode = await runProcess( + environment.cloneScript.path, + [ + // Use .dart_tools to clone the Flutter repo. + environment.webUiDartToolDir.path, + ], + workingDirectory: environment.webUiRootDir.path, ); + + if (exitCode != 0) { + // TODO: throw tool exception. + } + } + + void _runDriver() async { + startProcess('./chromedriver/chromedriver', ['--port=4444'], + workingDirectory: io.Directory.current.path); print('INFO: Driver started'); } diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 5b62a064d7b8d..d5d26eb9fae35 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -134,7 +134,7 @@ class TestCommand extends Command with ArgUtils { Future runIntegrationTests() async { // TODO(nurhan): https://github.com/flutter/flutter/issues/52983 - if (io.Platform.environment['LUCI_CONTEXT'] != null || isCirrus) { + if (io.Platform.environment['LUCI_CONTEXT'] != null) { return true; } diff --git a/tools/clone_flutter.sh b/tools/clone_flutter.sh index cb9f4916bc965..cfbc7ea1c30f4 100755 --- a/tools/clone_flutter.sh +++ b/tools/clone_flutter.sh @@ -33,10 +33,25 @@ fi LATEST_COMMIT_TIME_ENGINE=`git log -1 --date=local --format="%cd"` echo "Latest commit time on engine found as $LATEST_COMMIT_TIME_ENGINE" -# Do rest of the task in the root directory -cd ~ -mkdir -p $FRAMEWORK_PATH -cd $FRAMEWORK_PATH +# Check if there is an argument added for repo location. +# If not use the location that should be set by Cirrus/LUCI. +FLUTTER_CLONE_REPO=$1 + +if [[ -z $FLUTTER_CLONE_REPO ]] +then + if [[ -z $FRAMEWORK_PATH ]] + then + echo "Framework path should be set to run the script." + exit 1 + fi + # Do rest of the task in the root directory + cd ~ + mkdir -p $FRAMEWORK_PATH + cd $FRAMEWORK_PATH +else + cd $FLUTTER_CLONE_REPO +fi + # Clone the Flutter Framework. git clone https://github.com/flutter/flutter.git cd flutter From 9c4f976dceeb5c676cde048b6f3260e8ef1b41f1 Mon Sep 17 00:00:00 2001 From: nturgut Date: Wed, 8 Apr 2020 22:49:28 -0700 Subject: [PATCH 04/15] only run cirrus tests on chrome. fix a comma in the flutter command path --- lib/web_ui/dev/environment.dart | 2 +- lib/web_ui/dev/felt | 3 --- lib/web_ui/dev/test_runner.dart | 3 ++- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 814ffeb3fd19e..b046b2e4d4053 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -142,7 +142,7 @@ class Environment { /// Only use [cloneScript] to clone flutter to the engine repo. io.File get flutterCommandDir => io.File(pathlib.join( webUiDartToolDir.path, - 'flutter' + 'flutter', 'bin', 'flutter', )); diff --git a/lib/web_ui/dev/felt b/lib/web_ui/dev/felt index 32808d45d3f0b..56b031ce0b616 100755 --- a/lib/web_ui/dev/felt +++ b/lib/web_ui/dev/felt @@ -4,9 +4,6 @@ set -e # felt: a command-line utility for building and testing Flutter web engine. # It stands for Flutter Engine Local Tester. -echo "engine path" -echo $ENGINE_PATH - FELT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" if [ -z "`which gclient`" ] diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index d5d26eb9fae35..6e1fc2b62ee6b 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -117,7 +117,8 @@ class TestCommand extends Command with ArgUtils { return runIntegrationTests(); case TestTypesRequested.all: // TODO(nurhan): https://github.com/flutter/flutter/issues/53322 - if (runAllTests) { + // TODO(nurhan): Expand browser matrix for felt integration tests. + if (runAllTests && isChrome) { bool integrationTestResult = await runIntegrationTests(); bool unitTestResult = await runUnitTests(); if (integrationTestResult != unitTestResult) { From c3ff9e2000c12fb0952b3050736b4e9caf737a0d Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 10:26:15 -0700 Subject: [PATCH 05/15] adding comments to public flags --- lib/web_ui/dev/common.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index 039fd83030629..c285b965c856d 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -231,8 +231,12 @@ class DevNull implements StringSink { void writeln([Object obj = ""]) {} } +/// Whether the tests are running on Cirrus CI. bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; +/// Whether the tests are running on LUCI. bool get isLuci => io.Platform.environment['LUCI_CONTEXT'] != null; +/// Whether the tests are running on one of the Continuous Integration +/// environements. bool get isCi => isCirrus || isLuci; From efbfb0e5ff267fd157454e7e30622aade44c6177 Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 12:23:00 -0700 Subject: [PATCH 06/15] use local engine parameter for flutter pub get --- lib/web_ui/dev/integration_tests_manager.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index eb27bf5214857..046e6df1d239b 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -57,6 +57,7 @@ class IntegrationTestsManager { [ 'pub', 'get', + '--local-engine=host_debug_unopt', ], workingDirectory: workingDirectory, ); From 7c9abc5bbec155e5bc7cec6e764704efdcc55fbb Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 13:51:27 -0700 Subject: [PATCH 07/15] change flutter executable used for flutter drive command --- lib/web_ui/dev/environment.dart | 6 ++++-- lib/web_ui/dev/integration_tests_manager.dart | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index b046b2e4d4053..6c9701aa88d4d 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -137,10 +137,12 @@ class Environment { 'clone_flutter.sh', )); - /// Path to flutter to be used for `flutter pub get`. + /// Path to flutter. + /// + /// For example, this can be used to run `flutter pub get`. /// /// Only use [cloneScript] to clone flutter to the engine repo. - io.File get flutterCommandDir => io.File(pathlib.join( + io.File get flutterCommand => io.File(pathlib.join( webUiDartToolDir.path, 'flutter', 'bin', diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 046e6df1d239b..403373962ae25 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -51,7 +51,7 @@ class IntegrationTestsManager { await _cloneFlutterRepo(); } final String executable = - isCi ? environment.flutterCommandDir.path : 'flutter'; + isCi ? environment.flutterCommand.path : 'flutter'; final int exitCode = await runProcess( executable, [ @@ -187,8 +187,10 @@ class IntegrationTestsManager { Future _runTestsInProfileMode( io.Directory directory, String testName) async { + final String executable = + isCi ? environment.flutterCommand.path : 'flutter'; final int exitCode = await runProcess( - 'flutter', + executable, [ 'drive', '--target=test_driver/${testName}', From bedf99989affdcbdc8a3ed450ced52575c344556 Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 16:45:57 -0700 Subject: [PATCH 08/15] fix a cleanup issue. address comments. add toolException. enable web in flutter --- lib/web_ui/dev/common.dart | 6 ++--- lib/web_ui/dev/environment.dart | 4 ++-- lib/web_ui/dev/felt.dart | 1 - lib/web_ui/dev/integration_tests_manager.dart | 23 +++++++++++++++++-- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/web_ui/dev/common.dart b/lib/web_ui/dev/common.dart index c285b965c856d..588fe67eedf6f 100644 --- a/lib/web_ui/dev/common.dart +++ b/lib/web_ui/dev/common.dart @@ -231,12 +231,12 @@ class DevNull implements StringSink { void writeln([Object obj = ""]) {} } -/// Whether the tests are running on Cirrus CI. +/// Whether the felt command is running on Cirrus CI. bool get isCirrus => io.Platform.environment['CIRRUS_CI'] == 'true'; -/// Whether the tests are running on LUCI. +/// Whether the felt command is running on LUCI. bool get isLuci => io.Platform.environment['LUCI_CONTEXT'] != null; -/// Whether the tests are running on one of the Continuous Integration +/// Whether the felt command is running on one of the Continuous Integration /// environements. bool get isCi => isCirrus || isLuci; diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 6c9701aa88d4d..699b3e1190cdc 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -132,7 +132,7 @@ class Environment { )); /// Path to the script that clones the Flutter repo. - io.File get cloneScript => io.File(pathlib.join( + io.File get cloneFlutterScript => io.File(pathlib.join( engineToolsDir.path, 'clone_flutter.sh', )); @@ -141,7 +141,7 @@ class Environment { /// /// For example, this can be used to run `flutter pub get`. /// - /// Only use [cloneScript] to clone flutter to the engine repo. + /// Only use [cloneFlutterScript] to clone flutter to the engine build. io.File get flutterCommand => io.File(pathlib.join( webUiDartToolDir.path, 'flutter', diff --git a/lib/web_ui/dev/felt.dart b/lib/web_ui/dev/felt.dart index 32f3619dc7014..6366b6152991a 100644 --- a/lib/web_ui/dev/felt.dart +++ b/lib/web_ui/dev/felt.dart @@ -37,7 +37,6 @@ void main(List args) async { final bool result = (await runner.run(args)) as bool; if (result == false) { print('Sub-command returned false: `${args.join(' ')}`'); - await cleanup(); exitCode = 1; } } on UsageException catch (e) { diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 403373962ae25..8f3d926918ab3 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -9,6 +9,7 @@ import 'package:web_driver_installer/chrome_driver_installer.dart'; import 'chrome_installer.dart'; import 'common.dart'; import 'environment.dart'; +import 'exceptions.dart'; import 'utils.dart'; class IntegrationTestsManager { @@ -49,6 +50,7 @@ class IntegrationTestsManager { Future _runPubGet(String workingDirectory) async { if (isCi) { await _cloneFlutterRepo(); + await _enableWeb(workingDirectory); } final String executable = isCi ? environment.flutterCommand.path : 'flutter'; @@ -73,7 +75,7 @@ class IntegrationTestsManager { Future _cloneFlutterRepo() async { final int exitCode = await runProcess( - environment.cloneScript.path, + environment.cloneFlutterScript.path, [ // Use .dart_tools to clone the Flutter repo. environment.webUiDartToolDir.path, @@ -82,7 +84,24 @@ class IntegrationTestsManager { ); if (exitCode != 0) { - // TODO: throw tool exception. + throw ToolException('ERROR: Failed to clone flutter repo. Exited with ' + 'exit code $exitCode'); + } + } + + Future _enableWeb(String workingDirectory) async { + final int exitCode = await runProcess( + environment.flutterCommand.path, + [ + 'config', + '--enable-web', + ], + workingDirectory: workingDirectory, + ); + + if (exitCode != 0) { + throw ToolException( + 'ERROR: Failed to enable web. Exited with exit code $exitCode'); } } From 7fdc03e87f5876f945b9cfc143dcaebe82569bf5 Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 17:53:38 -0700 Subject: [PATCH 09/15] address reviwer comments. fix issue with local-engine --- lib/web_ui/dev/integration_tests_manager.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 8f3d926918ab3..1ce1cff1d1b54 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -94,6 +94,7 @@ class IntegrationTestsManager { environment.flutterCommand.path, [ 'config', + '--local-engine=host_debug_unopt', '--enable-web', ], workingDirectory: workingDirectory, From c5ddc34d6ac6df51dd69af693a574b64bc08a8a7 Mon Sep 17 00:00:00 2001 From: nturgut Date: Thu, 9 Apr 2020 17:54:06 -0700 Subject: [PATCH 10/15] address reviwer comments. fix issue with local-engine --- tools/clone_flutter.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/clone_flutter.sh b/tools/clone_flutter.sh index cfbc7ea1c30f4..d2af07d152998 100755 --- a/tools/clone_flutter.sh +++ b/tools/clone_flutter.sh @@ -35,9 +35,9 @@ echo "Latest commit time on engine found as $LATEST_COMMIT_TIME_ENGINE" # Check if there is an argument added for repo location. # If not use the location that should be set by Cirrus/LUCI. -FLUTTER_CLONE_REPO=$1 +FLUTTER_CLONE_REPO_PATH=$1 -if [[ -z $FLUTTER_CLONE_REPO ]] +if [[ -z $FLUTTER_CLONE_REPO_PATH ]] then if [[ -z $FRAMEWORK_PATH ]] then @@ -49,7 +49,7 @@ then mkdir -p $FRAMEWORK_PATH cd $FRAMEWORK_PATH else - cd $FLUTTER_CLONE_REPO + cd $FLUTTER_CLONE_REPO_PATH fi # Clone the Flutter Framework. From 21d7dd5ecd3ab76f51d55a108332c723ea236cdd Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 10 Apr 2020 12:35:59 -0700 Subject: [PATCH 11/15] using engine/flutter/.dart_tools as clone directory. enabling clone script for local usage --- lib/web_ui/dev/environment.dart | 12 +++++++++++- lib/web_ui/dev/integration_tests_manager.dart | 4 ++-- tools/clone_flutter.sh | 5 ++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/web_ui/dev/environment.dart b/lib/web_ui/dev/environment.dart index 699b3e1190cdc..824bded595267 100644 --- a/lib/web_ui/dev/environment.dart +++ b/lib/web_ui/dev/environment.dart @@ -118,6 +118,16 @@ class Environment { '.dart_tool', )); + /// Path to the ".dart_tool" directory living under `engine/src/flutter`. + /// + /// This is a designated area for tool downloads which can be used by + /// multiple platforms. For exampe: Flutter repo for e2e tests. + io.Directory get engineDartToolDir => io.Directory(pathlib.join( + engineSrcDir.path, + 'flutter', + '.dart_tool', + )); + /// Path to the "dev" directory containing engine developer tools and /// configuration files. io.Directory get webUiDevDir => io.Directory(pathlib.join( @@ -143,7 +153,7 @@ class Environment { /// /// Only use [cloneFlutterScript] to clone flutter to the engine build. io.File get flutterCommand => io.File(pathlib.join( - webUiDartToolDir.path, + engineDartToolDir.path, 'flutter', 'bin', 'flutter', diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 1ce1cff1d1b54..28aca0ee9c339 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -77,8 +77,8 @@ class IntegrationTestsManager { final int exitCode = await runProcess( environment.cloneFlutterScript.path, [ - // Use .dart_tools to clone the Flutter repo. - environment.webUiDartToolDir.path, + // Use engine/src/flutter/.dart_tools to clone the Flutter repo. + environment.engineDartToolDir.path, ], workingDirectory: environment.webUiRootDir.path, ); diff --git a/tools/clone_flutter.sh b/tools/clone_flutter.sh index d2af07d152998..48debdf44475c 100755 --- a/tools/clone_flutter.sh +++ b/tools/clone_flutter.sh @@ -4,13 +4,12 @@ set -x if [[ "$CIRRUS_CI" = false || -z $CIRRUS_CI ]] then - echo "This script is aimed to be run on CI environments. Do not run locally." - exit 1 + echo "Cloning Flutter repo to local machine." fi if [[ -z $ENGINE_PATH ]] then - echo "Engine path should be set to run the script." + echo "Please set ENGINE_PATH environment variable." exit 1 fi From 958af7c0d6bdfad792ec8eebf598b9446b9ff626 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 10 Apr 2020 15:19:03 -0700 Subject: [PATCH 12/15] clean flutter repo with felt clean. add a flag to skip cloning the repo. always clone the repo even for local development, unless this flag is set --- lib/web_ui/dev/clean.dart | 9 ++++++++ lib/web_ui/dev/integration_tests_manager.dart | 22 ++++++++++++++----- lib/web_ui/dev/test_runner.dart | 20 +++++++++++++++-- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/lib/web_ui/dev/clean.dart b/lib/web_ui/dev/clean.dart index ba2da00c857f6..b3d065fc66534 100644 --- a/lib/web_ui/dev/clean.dart +++ b/lib/web_ui/dev/clean.dart @@ -15,6 +15,11 @@ import 'utils.dart'; class CleanCommand extends Command with ArgUtils { CleanCommand() { argParser + ..addFlag( + 'flutter', + defaultsTo: true, + help: 'Cleans up the .dart_tool directory under engine/src/flutter.', + ) ..addFlag( 'ninja', defaultsTo: false, @@ -27,6 +32,8 @@ class CleanCommand extends Command with ArgUtils { bool get _alsoCleanNinja => boolArg('ninja'); + bool get _alsoCleanFlutterRepo => boolArg('flutter'); + @override String get description => 'Deletes build caches and artifacts.'; @@ -48,6 +55,8 @@ class CleanCommand extends Command with ArgUtils { ...fontFiles, if (_alsoCleanNinja) environment.outDir, + if(_alsoCleanFlutterRepo) + environment.engineDartToolDir, ]; await Future.wait( diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 28aca0ee9c339..807abc1eb89be 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -30,7 +30,9 @@ class IntegrationTestsManager { /// tests shutdown. final io.Directory _drivers; - IntegrationTestsManager(this._browser) + final bool _useSystemFlutter; + + IntegrationTestsManager(this._browser, this._useSystemFlutter) : this._browserDriverDir = io.Directory(pathlib.join( environment.webUiDartToolDir.path, 'drivers', _browser)), this._drivers = io.Directory( @@ -48,12 +50,12 @@ class IntegrationTestsManager { } Future _runPubGet(String workingDirectory) async { - if (isCi) { + if (!_useSystemFlutter) { await _cloneFlutterRepo(); await _enableWeb(workingDirectory); } final String executable = - isCi ? environment.flutterCommand.path : 'flutter'; + _useSystemFlutter ? 'flutter' : environment.flutterCommand.path; final int exitCode = await runProcess( executable, [ @@ -73,11 +75,21 @@ class IntegrationTestsManager { } } + /// Clone flutter repository, use the youngest commit older than the engine + /// commit. + /// + /// Use engine/src/flutter/.dart_tools to clone the Flutter repo. + /// TODO(nurhan): Use git pull instead if repo exists. Future _cloneFlutterRepo() async { + // Delete directory if exists. + if(environment.cloneFlutterScript.existsSync()) { + environment.cloneFlutterScript.deleteSync(); + } + environment.cloneFlutterScript.createSync(); + final int exitCode = await runProcess( environment.cloneFlutterScript.path, [ - // Use engine/src/flutter/.dart_tools to clone the Flutter repo. environment.engineDartToolDir.path, ], workingDirectory: environment.webUiRootDir.path, @@ -208,7 +220,7 @@ class IntegrationTestsManager { Future _runTestsInProfileMode( io.Directory directory, String testName) async { final String executable = - isCi ? environment.flutterCommand.path : 'flutter'; + _useSystemFlutter ? 'flutter' : environment.flutterCommand.path; final int exitCode = await runProcess( executable, [ diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 6e1fc2b62ee6b..2d3cc23c3e641 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -15,11 +15,11 @@ import 'package:test_api/src/backend/runtime.dart'; // ignore: implementation_im import 'package:test_core/src/executable.dart' as test; // ignore: implementation_imports +import 'environment.dart'; import 'exceptions.dart'; import 'integration_tests_manager.dart'; import 'supported_browsers.dart'; import 'test_platform.dart'; -import 'environment.dart'; import 'utils.dart'; /// The type of tests requested by the tool user. @@ -56,6 +56,17 @@ class TestCommand extends Command with ArgUtils { 'at the same time. If this flag is set, only run the integration ' 'tests.', ) + ..addFlag('use-system-flutter', + defaultsTo: false, + help: 'integration tests are using flutter repository for various tasks' + ', such as flutter drive, flutter pub get. If this flag is set, felt ' + 'will use flutter command without cloning the repository. This flag ' + 'can save internet bandwidth. However use with caution. Note that ' + 'since flutter repo is always synced to youngest commit older than ' + 'the engine commit for the tests running in CI, the tests results ' + 'won\'t be consistent with CIs when this flag ise set. flutter ' + 'command should be set in the PATH for this flag to be useful.' + ) ..addFlag( 'update-screenshot-goldens', defaultsTo: false, @@ -139,7 +150,7 @@ class TestCommand extends Command with ArgUtils { return true; } - return IntegrationTestsManager(browser).runTests(); + return IntegrationTestsManager(browser, useSystemFlutter).runTests(); } Future runUnitTests() async { @@ -206,6 +217,11 @@ class TestCommand extends Command with ArgUtils { /// Whether [browser] is set to "chrome". bool get isChrome => browser == 'chrome'; + /// Use system flutter instead of cloning the repository. + /// + /// Read the flag help for more details. + bool get useSystemFlutter => boolArg('use-system-flutter'); + /// When running screenshot tests writes them to the file system into /// ".dart_tool/goldens". bool get doUpdateScreenshotGoldens => boolArg('update-screenshot-goldens'); From 2bafbbc9cdc265381e4a8fcb9d38f58e0dc27dd6 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 10 Apr 2020 15:31:27 -0700 Subject: [PATCH 13/15] fixing typos. updating readme for the new flag. --- lib/web_ui/dev/README.md | 8 +++++++- lib/web_ui/dev/integration_tests_manager.dart | 1 - lib/web_ui/dev/test_runner.dart | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/dev/README.md b/lib/web_ui/dev/README.md index c79dc1f3d6c46..6d52e10640065 100644 --- a/lib/web_ui/dev/README.md +++ b/lib/web_ui/dev/README.md @@ -43,12 +43,18 @@ 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. +To run integration tests only. For now these tests are only available on Chrome Desktop browsers. These tests will fetch the flutter repository for using `flutter drive` and `flutter pub get` commands. The repository will be synced to the youngest commit older than the engine commit. ``` felt test --integration-tests-only ``` +To skip cloning the flutter repository use the following flag. This flag can save internet bandwidth. However use with caution. Note the tests results will not be consistent with CIs when this flag is set. flutter command should be set in the PATH for this flag to be useful + +``` +felt test --integration-tests-only --use-system-flutter +``` + To run tests on Firefox (this will work only on a Linux device): ``` diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 807abc1eb89be..8db8da7a6c568 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -7,7 +7,6 @@ import 'package:path/path.dart' as pathlib; import 'package:web_driver_installer/chrome_driver_installer.dart'; import 'chrome_installer.dart'; -import 'common.dart'; import 'environment.dart'; import 'exceptions.dart'; import 'utils.dart'; diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 2d3cc23c3e641..bcb07cd6273a9 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -64,7 +64,7 @@ class TestCommand extends Command with ArgUtils { 'can save internet bandwidth. However use with caution. Note that ' 'since flutter repo is always synced to youngest commit older than ' 'the engine commit for the tests running in CI, the tests results ' - 'won\'t be consistent with CIs when this flag ise set. flutter ' + 'won\'t be consistent with CIs when this flag is set. flutter ' 'command should be set in the PATH for this flag to be useful.' ) ..addFlag( From cf34de4f5b8b3648a90297aaad4bf5d12e2cac57 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 10 Apr 2020 15:34:16 -0700 Subject: [PATCH 14/15] fix directory error --- lib/web_ui/dev/integration_tests_manager.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index 8db8da7a6c568..f63794b1c07f1 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -81,10 +81,10 @@ class IntegrationTestsManager { /// TODO(nurhan): Use git pull instead if repo exists. Future _cloneFlutterRepo() async { // Delete directory if exists. - if(environment.cloneFlutterScript.existsSync()) { - environment.cloneFlutterScript.deleteSync(); + if(environment.engineDartToolDir.existsSync()) { + environment.engineDartToolDir.deleteSync(); } - environment.cloneFlutterScript.createSync(); + environment.engineDartToolDir.createSync(); final int exitCode = await runProcess( environment.cloneFlutterScript.path, From 41a7dae066bbf10ab3d77259bd35d7f3d040edb5 Mon Sep 17 00:00:00 2001 From: nturgut Date: Fri, 10 Apr 2020 17:11:26 -0700 Subject: [PATCH 15/15] addressing reviewer comments --- lib/web_ui/dev/README.md | 2 +- lib/web_ui/dev/integration_tests_manager.dart | 43 +++---------------- lib/web_ui/dev/test_runner.dart | 3 +- lib/web_ui/dev/utils.dart | 21 +++++++++ 4 files changed, 31 insertions(+), 38 deletions(-) diff --git a/lib/web_ui/dev/README.md b/lib/web_ui/dev/README.md index 6d52e10640065..dba654e49c54e 100644 --- a/lib/web_ui/dev/README.md +++ b/lib/web_ui/dev/README.md @@ -49,7 +49,7 @@ To run integration tests only. For now these tests are only available on Chrome felt test --integration-tests-only ``` -To skip cloning the flutter repository use the following flag. This flag can save internet bandwidth. However use with caution. Note the tests results will not be consistent with CIs when this flag is set. flutter command should be set in the PATH for this flag to be useful +To skip cloning the flutter repository use the following flag. This flag can save internet bandwidth. However use with caution. Note the tests results will not be consistent with CIs when this flag is set. flutter command should be set in the PATH for this flag to be useful. This flag can also be used to test local Flutter changes. ``` felt test --integration-tests-only --use-system-flutter diff --git a/lib/web_ui/dev/integration_tests_manager.dart b/lib/web_ui/dev/integration_tests_manager.dart index f63794b1c07f1..ec3595dedf268 100644 --- a/lib/web_ui/dev/integration_tests_manager.dart +++ b/lib/web_ui/dev/integration_tests_manager.dart @@ -48,30 +48,13 @@ class IntegrationTestsManager { } } - Future _runPubGet(String workingDirectory) async { + Future _runPubGet(String workingDirectory) async { if (!_useSystemFlutter) { await _cloneFlutterRepo(); await _enableWeb(workingDirectory); } - final String executable = - _useSystemFlutter ? 'flutter' : environment.flutterCommand.path; - final int exitCode = await runProcess( - executable, - [ - 'pub', - 'get', - '--local-engine=host_debug_unopt', - ], - workingDirectory: workingDirectory, - ); - - if (exitCode != 0) { - io.stderr.writeln( - 'ERROR: Failed to run pub get. Exited with exit code $exitCode'); - return false; - } else { - return true; - } + await runFlutter(workingDirectory, ['pub', 'get'], + useSystemFlutter: _useSystemFlutter); } /// Clone flutter repository, use the youngest commit older than the engine @@ -81,7 +64,7 @@ class IntegrationTestsManager { /// TODO(nurhan): Use git pull instead if repo exists. Future _cloneFlutterRepo() async { // Delete directory if exists. - if(environment.engineDartToolDir.existsSync()) { + if (environment.engineDartToolDir.existsSync()) { environment.engineDartToolDir.deleteSync(); } environment.engineDartToolDir.createSync(); @@ -101,20 +84,8 @@ class IntegrationTestsManager { } Future _enableWeb(String workingDirectory) async { - final int exitCode = await runProcess( - environment.flutterCommand.path, - [ - 'config', - '--local-engine=host_debug_unopt', - '--enable-web', - ], - workingDirectory: workingDirectory, - ); - - if (exitCode != 0) { - throw ToolException( - 'ERROR: Failed to enable web. Exited with exit code $exitCode'); - } + await runFlutter(workingDirectory, ['config', '--enable-web'], + useSystemFlutter: _useSystemFlutter); } void _runDriver() async { @@ -219,7 +190,7 @@ class IntegrationTestsManager { Future _runTestsInProfileMode( io.Directory directory, String testName) async { final String executable = - _useSystemFlutter ? 'flutter' : environment.flutterCommand.path; + _useSystemFlutter ? 'flutter' : environment.flutterCommand.path; final int exitCode = await runProcess( executable, [ diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index bcb07cd6273a9..598f85d9fb717 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -66,6 +66,7 @@ class TestCommand extends Command with ArgUtils { 'the engine commit for the tests running in CI, the tests results ' 'won\'t be consistent with CIs when this flag is set. flutter ' 'command should be set in the PATH for this flag to be useful.' + 'This flag can also be used to test local Flutter changes.' ) ..addFlag( 'update-screenshot-goldens', @@ -219,7 +220,7 @@ class TestCommand extends Command with ArgUtils { /// Use system flutter instead of cloning the repository. /// - /// Read the flag help for more details. + /// Read the flag help for more details. Uses PATH to locate flutter. bool get useSystemFlutter => boolArg('use-system-flutter'); /// When running screenshot tests writes them to the file system into diff --git a/lib/web_ui/dev/utils.dart b/lib/web_ui/dev/utils.dart index bf3475e8874fc..2ccdb04c5f049 100644 --- a/lib/web_ui/dev/utils.dart +++ b/lib/web_ui/dev/utils.dart @@ -11,6 +11,7 @@ import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; import 'environment.dart'; +import 'exceptions.dart'; class FilePath { FilePath.fromCwd(String relativePath) @@ -107,6 +108,26 @@ Future evalProcess( return result.stdout as String; } +Future runFlutter( + String workingDirectory, + List arguments, { + bool useSystemFlutter = false, +}) async { + final String executable = + useSystemFlutter ? 'flutter' : environment.flutterCommand.path; + arguments.add('--local-engine=host_debug_unopt'); + final int exitCode = await runProcess( + executable, + arguments, + workingDirectory: workingDirectory, + ); + + if (exitCode != 0) { + throw ToolException('ERROR: Failed to run $executable with ' + 'arguments ${arguments.toString()}. Exited with exit code $exitCode'); + } +} + @immutable class ProcessException implements Exception { ProcessException({