Add a FlutterDriver screenshot test to device lab. by cyanglaz · Pull Request #45411 · flutter/flutter

cyanglaz · 2019-11-22T18:43:37Z

Description

Created a new app under dev/integration_test, can be used to run screenshot tests with FlutterDriver. More details can be found in the README.md in this PR.

Also added the test to the device lab. We currently only add this test for mac/ios(iphone 6s). We can enable running this test on other devices later.

This test can be used to catch regressions like #45098

Related Issues

#45348

Tests

I added the following tests:

dev/devicelab/bin/tasks/flutter_driver_screenshot_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

tvolkert · 2019-11-22T19:55:43Z

Looks like bots are unhappy

tvolkert · 2019-12-02T03:32:15Z

Bots are still unhappy.

cyanglaz · 2019-12-02T20:51:07Z

@tvolkert It's green now! (except the flutter-build tree)

tvolkert · 2019-12-04T20:43:22Z

    required_agent_capabilities: ["mac/ios"]
    timeout_in_minutes: 20

+  flutter_driver_screenshot_test:


Let's call this flutter_driver_screenshot_test_ios

tvolkert · 2019-12-04T20:56:46Z

+
+  tearDownAll(() => driver.close());
+
+  group('image rendering ', () {


Nit: no need for a group if there's only 1 test.

tvolkert · 2019-12-04T20:59:01Z

+    test('A page with an image screenshot', () async {
+
+      await driver.waitFor(find.byValueKey('image_page'));
+      final SerializableFinder imagePageListTile =


If you put this line first, then you can await driver.waitFor(imagePageListTile);

tvolkert · 2019-12-04T21:01:45Z

+  DriverScreenShotTester(
+      {@required this.testName,
+      @required this.driver,
+      @required this.deviceModel})


Nit: the formatting will be a little nicer if you put a trailing comma after deviceModel

tvolkert · 2019-12-04T21:02:20Z

+  ///
+  /// All the parameters are required and must not be null.
+  // ignore: always_require_non_null_named_parameters
+  DriverScreenShotTester(


Nit: this could be const

tvolkert · 2019-12-04T21:02:35Z

+const String _kPathParent = 'test_driver/goldens/';
+
+/// The utility class that helps test cases to tests screenshots with a [FlutterDriver].
+class DriverScreenShotTester {


@immutable

tvolkert · 2019-12-04T21:06:07Z

+  /// Constructs a [DriverScreenShotTester].
+  ///
+  /// All the parameters are required and must not be null.
+  // ignore: always_require_non_null_named_parameters


This ignore should not be needed

tvolkert · 2019-12-04T21:08:40Z

+  /// prior to this call.
+  Future<bool> compareScreenshots(List<int> screenshot) async {
+    final File file = File(_getImageFilePath());
+    final List<int> matcher = file.readAsBytesSync();


Might as well use the async version since you're already in an async method.

tvolkert · 2019-12-04T21:09:45Z

+  /// `test_driver/goldens/<testName>/<deviceModel>.png`
+  ///
+  /// Can be used when recording the golden for the first time.
+  Future<void> saveScreenshot(List<int> screenshot) async {


When is this used?

This is not used. It serves as a utility method to save the golden when setting up the test for the first time.

So when we want to add another screenshot test case, we can use this to save the golden image.

Maybe there is a better way to set up golden in flutter driver test?

tvolkert · 2019-12-04T21:09:56Z

+  }
+
+  /// Check if the golden exists.
+  bool goldenExists() {


this is not used, removing

cyanglaz

@tvolkert Thanks for the quick turnaround. The PR is updated with suggestions and I left one comment on the saveScreenshot.

cyanglaz · 2019-12-04T23:11:39Z

    required_agent_capabilities: ["mac/ios"]
    timeout_in_minutes: 20

+  flutter_driver_screenshot_test:


cyanglaz · 2019-12-04T23:12:39Z

+  /// Constructs a [DriverScreenShotTester].
+  ///
+  /// All the parameters are required and must not be null.
+  // ignore: always_require_non_null_named_parameters


cyanglaz · 2019-12-04T23:13:38Z

+const String _kPathParent = 'test_driver/goldens/';
+
+/// The utility class that helps test cases to tests screenshots with a [FlutterDriver].
+class DriverScreenShotTester {


cyanglaz · 2019-12-04T23:13:44Z

+  ///
+  /// All the parameters are required and must not be null.
+  // ignore: always_require_non_null_named_parameters
+  DriverScreenShotTester(


cyanglaz · 2019-12-04T23:13:49Z

+  DriverScreenShotTester(
+      {@required this.testName,
+      @required this.driver,
+      @required this.deviceModel})


cyanglaz · 2019-12-04T23:14:48Z

+  /// prior to this call.
+  Future<bool> compareScreenshots(List<int> screenshot) async {
+    final File file = File(_getImageFilePath());
+    final List<int> matcher = file.readAsBytesSync();


cyanglaz · 2019-12-04T23:18:00Z

+  /// `test_driver/goldens/<testName>/<deviceModel>.png`
+  ///
+  /// Can be used when recording the golden for the first time.
+  Future<void> saveScreenshot(List<int> screenshot) async {


This is not used. It serves as a utility method to save the golden when setting up the test for the first time.

So when we want to add another screenshot test case, we can use this to save the golden image.

Maybe there is a better way to set up golden in flutter driver test?

cyanglaz · 2019-12-04T23:18:49Z

+  }
+
+  /// Check if the golden exists.
+  bool goldenExists() {


this is not used, removing

cyanglaz · 2019-12-04T23:18:56Z

+
+  tearDownAll(() => driver.close());
+
+  group('image rendering ', () {


cyanglaz · 2019-12-04T23:19:01Z

+    test('A page with an image screenshot', () async {
+
+      await driver.waitFor(find.byValueKey('image_page'));
+      final SerializableFinder imagePageListTile =


tvolkert

LGTM

This reverts commit adec697.

…46281) This reverts commit adec697.

…45411)

…46620)

googlebot added the cla: yes label Nov 22, 2019

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Nov 22, 2019

cyanglaz requested a review from tvolkert November 22, 2019 18:44

cyanglaz force-pushed the screenshot_integration_tests branch from 6fb0c78 to 18b0319 Compare November 22, 2019 18:49

screenshot test

7cba69d

cyanglaz force-pushed the screenshot_integration_tests branch from 1b579a7 to 7cba69d Compare December 2, 2019 19:38

This was referenced Dec 4, 2019

Make sure platform views are isolated from the engine's GL context #41620

Closed

[webview_flutter] Fix iOS WebView ignoring SafeArea widget and iOS 13 WebView scroll indicator showing at random location. flutter/plugins#2343

Merged

tvolkert reviewed Dec 4, 2019

View reviewed changes

cyanglaz commented Dec 4, 2019

View reviewed changes

review fixees

642b175

tvolkert approved these changes Dec 5, 2019

View reviewed changes

Merge branch 'master' into screenshot_integration_tests

b8e00d4

cyanglaz merged commit adec697 into flutter:master Dec 6, 2019

cyanglaz deleted the screenshot_integration_tests branch December 6, 2019 22:17

cyanglaz pushed a commit that referenced this pull request Dec 6, 2019

Revert "Add a FlutterDriver screenshot test to device lab. (#45411)"

aeb970a

This reverts commit adec697.

cyanglaz mentioned this pull request Dec 6, 2019

Revert "Add a FlutterDriver screenshot test to device lab." #46259

Closed

jmagman mentioned this pull request Dec 7, 2019

Revert "Add a FlutterDriver screenshot test to device lab." #46281

Merged

jmagman added a commit that referenced this pull request Dec 7, 2019

Revert "Add a FlutterDriver screenshot test to device lab. (#45411)" (#…

e58dc16

…46281) This reverts commit adec697.

cyanglaz pushed a commit to cyanglaz/flutter that referenced this pull request Dec 9, 2019

Recommit Add a FlutterDriver screenshot test to device lab. (flutter#…

2306700

…45411)

cyanglaz mentioned this pull request Dec 9, 2019

Reland "Add a FlutterDriver screenshot test to device lab. #45411" #46620

Merged

13 tasks

cyanglaz pushed a commit that referenced this pull request Dec 10, 2019

Reland "Add a FlutterDriver screenshot test to device lab. #45411" (#…

4a00496

…46620)

github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 2021


		tearDownAll(() => driver.close());

		group('image rendering ', () {

Conversation

cyanglaz commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

tvolkert commented Nov 22, 2019

Uh oh!

tvolkert commented Dec 2, 2019

Uh oh!

cyanglaz commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert left a comment

cyanglaz commented Nov 22, 2019 •

edited

Loading

cyanglaz commented Dec 2, 2019 •

edited

Loading