addressing reviewer comments

Author: Nurhan Turgut

e2etests/web/regular_integration_tests/README.md

@@ -40,44 +40,31 @@ More details for "Running Flutter Driver tests with Web" can be found in [wiki](
 
 ## Adding screenshot tests
 
-In order to test screenshot tests the tests on the driver side needs to call the `integration_test` package with an `onScreenshot` callback which can do a comparison between the `screenshotBytes` taken during the test and a golden file. We added a utility method that can do this comparison by using a golden in `flutter/goldens` repository. In order to use screenshot testing follow these steps:
+In order to test screenshot tests the tests on the driver side needs to call the `integration_test` package with an `onScreenshot` callback which can do a comparison between the `screenshotBytes` taken during the test and a golden file. We added a utility method that can do this comparison by using a golden in `flutter/goldens` repository.
 
-1. Call `screenshot_support.dart` from the driver side test (example: text_editing_integration_test.dart). Default value for `diffRateFailure` is 0.5 .
+In order to use screenshot testing first, import `screenshot_support.dart` from the driver side test (example: `text_editing_integration_test.dart`). Default value for `diffRateFailure` is 0.5.
 
 ```
 import 'package:regular_integration_tests/screenshot_support.dart' as test;
 
 Future<void> main() async {
   final double kMaxDiffRateFailure = 0.1;
-  await test.runTestWithScreenshots(diffRateFailure = kMaxDiffRateFailure );
+  await test.runTestWithScreenshots(diffRateFailure = kMaxDiffRateFailure);
 }
 ```
 
-2. In order to add the screenshot golden initially or to update the existing one, we need to set UPDATE_GOLDENS flag to environment.
+In order to run the tests follow these steps:
 
-```
-export UPDATE_GOLDENS=true
-```
-
-3. Run the specific test or run all integration tests
+1. You can use two different approaches, using [felt](https://github.com/flutter/engine/blob/master/lib/web_ui/dev/README.md) tool will run all the tests, an update all the goldens. For running individual tests, we need to set UPDATE_GOLDENS environment variable.
 
 ```
-flutter drive -v --target=test_driver/text_editing_integration.dart -d web-server --release --local-engine=host_debug_unopt
+felt test --integration-tests-only --update-screenshot-goldens
 ```
 
 ```
-felt test --integration-tests-only
+UPDATE_GOLDENS=true flutter drive -v --target=test_driver/text_editing_integration.dart -d web-server --release --local-engine=host_debug_unopt
 ```
 
-4. The golden will be under `engine/src/flutter/lib/web_ui/.dart_tool/goldens/engine/web/` directory, you should create a PR for that file and merge it to `flutter/goldens`.
-
-5. Get the commit no and replace the `revision` in this file: `engine/src/flutter/lib/web_ui/dev/goldens_lock.yaml`
-
-6. Don't forget to rechange the flag to switch goldens from update mode to comparison mode.
-
-
-```
-export UPDATE_GOLDENS=false
-```
+2. The golden will be under `engine/src/flutter/lib/web_ui/.dart_tool/goldens/engine/web/` directory, you should create a PR for that file and merge it to `flutter/goldens`.
 
-7. Screenshot tests should work after this.
+3. Get the commit SHA and replace the `revision` in this file: `engine/src/flutter/lib/web_ui/dev/goldens_lock.yaml`

e2etests/web/regular_integration_tests/lib/screenshot_support.dart

@@ -22,8 +22,9 @@ import 'package:image/image.dart';
 /// under test ex: a blinking cursor.
 const double kMaxDiffRateFailure = 0.5 / 100; // 0.5%
 
-const int kMaxScreenshotWidth = 1024;
-const int kMaxScreenshotHeight = 1024;
+/// SBrowser screen dimensions for the Flutter Driver test.
+const int _kScreenshotWidth = 1024;
+const int _kScreenshotHeight = 1024;
 
 /// Used for calling `integration_test` package.
 ///
@@ -38,8 +39,8 @@ const int kMaxScreenshotHeight = 1024;
 /// It also includes options for updating the golden files.
 Future<void> runTestWithScreenshots(
     {double diffRateFailure = kMaxDiffRateFailure,
-     int browserWidth = kMaxScreenshotWidth,
-     int browserHeight = kMaxScreenshotHeight}) async {
+    int browserWidth = _kScreenshotWidth,
+    int browserHeight = _kScreenshotHeight}) async {
   final WebFlutterDriver driver =
       await FlutterDriver.connect() as WebFlutterDriver;
 
@@ -50,16 +51,20 @@ Future<void> runTestWithScreenshots(
   window.setSize(Rectangle<int>(0, 0, browserWidth, browserHeight));
 
   bool updateGoldens = false;
+  // We are using an environment variable instead of an argument, since
+  // this code is not invoked from the shell but from the `flutter drive`
+  // tool itself. Therefore we do not have control on the command line
+  // arguments.
+  // Please read the README, further info on how to update the goldens.
   final String updateGoldensFlag = io.Platform.environment['UPDATE_GOLDENS'];
-  if (updateGoldensFlag == null || updateGoldensFlag.toLowerCase() != 'true') {
-    // We are using an environment variable since instead of an argument, since
-    // this code is not invoked from the shell but from the `flutter drive`
-    // tool itself. Therefore we do not have control on the command line
-    // arguments.
-    // Please read the README, further info on how to update the goldens.
-    print('INFO: Goldens will not be updated. Please set `UPDATE_GOLDENS` '
-        'environment variable to `true` for updating them.');
-  } else {
+  // Validate if the environment variable is set correctly.
+  if (updateGoldensFlag != null &&
+      !(updateGoldensFlag.toLowerCase() == 'true' ||
+          updateGoldensFlag.toLowerCase() == 'false')) {
+    throw StateError(
+        'UPDATE_GOLDENS environment variable is not set correctly');
+  }
+  if (updateGoldensFlag != null && updateGoldensFlag.toLowerCase() == 'true') {
     updateGoldens = true;
   }
 
@@ -77,10 +82,12 @@ Future<void> runTestWithScreenshots(
           forIntegrationTests: true,
           write: updateGoldens,
         );
-        if (result != 'OK') {
-          print('ERROR: $result');
+        if (result == 'OK') {
+          return true;
+        } else {
+          io.stderr.writeln('ERROR: $result');
+          return false;
         }
-        return result == 'OK';
       } else {
         return true;
       }

lib/web_ui/dev/goldens_lock.yaml

@@ -1,2 +1,2 @@
-repository: https://github.com/nturgut/goldens.git
-revision: 6163d4e5970d739a413886c7b16136f2c8baf351
+repository: https://github.com/flutter/goldens.git
+revision: 7b9c0fa3b69bcea1501975268e8afa64f07cb313

lib/web_ui/dev/integration_tests_manager.dart

@@ -24,7 +24,10 @@ class IntegrationTestsManager {
 
   final DriverManager _driverManager;
 
-  IntegrationTestsManager(this._browser, this._useSystemFlutter)
+  final bool _doUpdateScreenshotGoldens;
+
+  IntegrationTestsManager(
+      this._browser, this._useSystemFlutter, this._doUpdateScreenshotGoldens)
       : _driverManager = DriverManager.chooseDriver(_browser);
 
   Future<bool> runTests() async {
@@ -159,14 +162,19 @@ class IntegrationTestsManager {
 
   Future<bool> _runTestsInProfileMode(
       io.Directory directory, String testName) async {
-    final String executable =
+    String executable =
         _useSystemFlutter ? 'flutter' : environment.flutterCommand.path;
+    Map<String, String> enviroment = Map<String, String>();
+    if (_doUpdateScreenshotGoldens) {
+      enviroment['UPDATE_GOLDENS'] = 'true';
+    }
     final IntegrationArguments arguments =
         IntegrationArguments.fromBrowser(_browser);
     final int exitCode = await runProcess(
       executable,
       arguments.getTestArguments(testName, 'profile'),
       workingDirectory: directory.path,
+      environment: enviroment,
     );
 
     if (exitCode != 0) {

lib/web_ui/dev/test_runner.dart

@@ -159,7 +159,9 @@ class TestCommand extends Command<bool> with ArgUtils {
 
   Future<bool> runIntegrationTests() async {
     await _prepare();
-    return IntegrationTestsManager(browser, useSystemFlutter).runTests();
+    return IntegrationTestsManager(
+            browser, useSystemFlutter, doUpdateScreenshotGoldens)
+        .runTests();
   }
 
   Future<bool> runUnitTests() async {