Android e2e screenshot by blasten · Pull Request #84472 · flutter/flutter

blasten · 2021-06-11T22:36:39Z

Implement Android view screenshot in integration_tests.

This PR adds support for the existing takeScreenshot(String name) method on Android.

List of changes:

takeScreenshot(String name) return type is now a Future of the byte array. This works with the existing dependencies in g3. Example:

final bytes = binding.takeScreenshot('myApp');
await expectLater(bytes, matchesGoldenFile('screenshot.png'));

The byte array is also sent to the host via the driver script where it can be sent to services like Skia Gold for pixel comparisons. Here's an example:

test_driver/integration_test.dart:

  import 'dart:async';
  import 'package:integration_test/integration_test_driver.dart';
  Future<void> main() async => integrationDriver(onScreenshot: async (String name, List<int> bytes) {
     // name is "myApp" from the example above.
     // Send bytes to Skia Gold. 
  });

Adds binding.convertFlutterSurfaceToImage() as suggested by @dnfield. This is required prior to calling takeScreenshot().

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

blasten · 2021-06-12T00:22:34Z

@jiahaog this is still WIP for #83856, but can you please confirm that this works internally?

jiahaog · 2021-06-15T07:31:59Z

@blasten Please correct me if I'm wrong, I think you mean for the API to be used like so:

final myApp = find.byType(MyApp);

final bytes = binding.deviceScreenshot(myApp);
await expectLater(bytes, matchesGoldenFile('screenshot.png'));

This will work internally. I'm wondering if it would make sense to contain the screenshotting logic within the matcher, for consistency with the same API as flutter test screenshots. Something like:

// [matchesDeviceScreenshotGoldenFile] provided by `package:integration_test`

await expectLater(myApp, deviceScreenshotMatchesGoldenFile('screenshot.png'));

blasten · 2021-06-26T00:28:50Z

@jiahaog, @dnfield

There would be two different ways to get bytes out of the device.

External users

External users will use integration_tests with the flutter drive command to pull in the screenshots from the device.
They will only need to use await binding.takeScreenshot('<screenshot-name>');.

A comparator can be added to the file under test_driver:

integrationDriver(onScreenshot: (List<int> png) { 
  /* do something with it. Like saving to disk or sending them to Skia Gold. */
});

Internal users

Since the internal infra already knows how to reach the service for image comparison. There won't be need for integrationDriver. It could just use the raw bytes from await binding.takeScreenshot('<screenshot-name>');.

final bytes = binding.deviceScreenshot(myApp);
await expectLater(bytes, anyComparator('screenshot.png'));

Any thoughts?

jiahaog · 2021-06-28T11:55:01Z

(for external users) Is there a mechanism for users to identify which screenshot is received? Or are users expected to maintain the ordering between the binding.takeScreenshot calls on the host and device.

I think this works, but it's also worth noting that this approach to use the driver script may be a stopgap solution in the meantime. Since #66264, users can run integration tests with flutter test integration_test, and we would eventually like to converge on an approach where the driver script is obsolete, and where the diffing logic happens entirely on the device. The model internally is that all user initiated screenshots are saved to a known directory where tooling goes to pull them to the host at the end of the tests, which can be considered in the future if / when this feature is added to the Flutter Tool.

(For the internal example) Did you mean to use binding.takeScreenshot instead? We override the goldenFileComparator global at the start of tests, so I'd expect users to use the matchesGoldenFile matcher directly:

final bytes = binding.takeScreenshot('myApp');
await expectLater(bytes, matchesGoldenFile('screenshot.png'));

The string parameter to takeScreenshot will also be unused, which is a little awkward. How about removing the parameter or changing it to take a Finder where it can be used to find the bounds and crop the screenshot to just the widget of interest?

Future<List<int>> takeScreenshot(Finder finder) {
  // Take screenshot of the dimensions bound by [finder].
}

dnfield

My major concern is over synchronization. Right now, as implemented, at least two frames will get pumped after requesting a screenshot, but maybe more.

How will test authors know they're getting the frame they really care about, particularly when native UI might be involved and still animating/loading?

dnfield · 2021-06-29T17:09:07Z

+  /**
+   * Captures a screenshot by drawing the view to a Canvas.
+   *
+   * <p>It also converts {@link FlutterView} to an image view, since {@link FlutterSurfaceView}


This has some pretty serious performance implications right?

IOW, doing this is incompatible with using integration_test for performance testing.

Is there some other way of doing this without impacting global test state for the rest of the test execution?

So the answer seems to be yes. Why would we have this fallback though? Why not just fail if instrumentation is unavailable?

Discussed offline: we should probably make this part of the integration_test API so developers can opt into this if they want. If they ahven't opted in, we should fail if instrumentation isn't available.

dnfield · 2021-06-29T17:11:26Z

+    waitForAndroidFrame(
+        () -> {
+          waitForAndroidFrame(


This seems to mean that we'll pump at least two frames after calling this method, right?

How will this be stable for screenshot tests?

For image views: Thsi should work because we'll know we've had at least one frame drawn by flutter.

For instrumentation tests: This should work because the Dart part of the test should have gotten us to a stable non-animating point in the application, and now should be waiting before tapping or doing anything else.

@blasten - will the new method block if awaited until the screenshot is finished?

Removed support for instrumentation tests until #56591 is addressed.

It rasterizes in a separate background thread, so it doesn't block the main thread.

In that case, do we need to fail if !acquired?

This still seems to be an issue - I'm not sure if I'm just missing somethign here, but it seems like if acquired is false we'll fail to produce a screenshot.

It’s recursive, so it will keep waiting.

Ahh I get it now. Ok.

Will the test just timeout if they failed to convert to imageview first?

blasten · 2021-07-20T01:07:08Z

@dnfield PTAL

skia-gold · 2021-07-20T01:50:34Z

Gold has detected about 4 new digest(s) on patchset 36.
View them at https://flutter-gold.skia.org/cl/github/84472

skia-gold · 2021-07-20T02:45:34Z

Gold has detected about 4 new digest(s) on patchset 37.
View them at https://flutter-gold.skia.org/cl/github/84472

flutter-dashboard · 2021-07-20T03:11:30Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #84472 at sha 5a99c62

google-cla Bot added the cla: yes label Jun 11, 2021

Android e2e screenshot

75e1536

blasten force-pushed the android_e2e_screenshot branch from a419690 to 75e1536 Compare June 11, 2021 22:47

Emmanuel Garcia added 2 commits June 11, 2021 16:28

Crop bitmap

95c928e

Dart API

8bad0e9

blasten requested a review from jiahaog June 12, 2021 00:21

Emmanuel Garcia added 4 commits June 11, 2021 17:24

Typo

518cbe0

Use Uint8List directly instead

0062351

Remove crop function

c1724ae

Merge branch 'master' into android_e2e_screenshot

bcae461

Emmanuel Garcia added 7 commits June 15, 2021 13:28

WIP

c2fbf5c

WIP

f30807d

Remove activity

c919a0f

clean up

ab3ddf3

Clean up

9233b89

clean up

2bb4d87

Merge branch 'master' into android_e2e_screenshot

0c7fb03

blasten force-pushed the android_e2e_screenshot branch from 82714b8 to 0c7fb03 Compare June 26, 2021 00:15

Comment

b6747d0

dnfield reviewed Jun 29, 2021

View reviewed changes

Emmanuel Garcia added 6 commits July 7, 2021 20:05

Pass byte buffer

fbb86a6

clean up

475aafe

Merge branch 'master' into android_e2e_screenshot

0c09ef9

Update lock file

bb53a7b

Example

f0912aa

Remove minSdkVersion

eae1c05

blasten force-pushed the android_e2e_screenshot branch from 9f96994 to 87b53eb Compare July 20, 2021 01:00

Emmanuel Garcia added 2 commits July 19, 2021 18:03

Merge branch 'master' into android_e2e_screenshot

4f79019

Fix readme

612e759

analyzer

9a2c8b0

dnfield approved these changes Jul 20, 2021

View reviewed changes

Fix

5a99c62

blasten added the waiting for tree to go green label Jul 20, 2021

flutter-dashboard Bot added the will affect goldens Changes to golden files label Jul 20, 2021

jiahaog reviewed Jul 20, 2021

View reviewed changes

Emmanuel Garcia added 3 commits July 20, 2021 10:28

fixes

ff9bdf6

test

1e8db29

Merge branch 'master' into android_e2e_screenshot

0d08ce8

flutter deleted a comment from flutter-dashboard Bot Jul 20, 2021

flutter deleted a comment from skia-gold Jul 20, 2021

blasten removed the will affect goldens Changes to golden files label Jul 20, 2021

flutter-dashboard Bot added the will affect goldens Changes to golden files label Jul 20, 2021

Merge branch 'master' into android_e2e_screenshot

754dac8

flutter deleted a comment from skia-gold Jul 20, 2021

flutter deleted a comment from flutter-dashboard Bot Jul 20, 2021

blasten removed the will affect goldens Changes to golden files label Jul 20, 2021

fluttergithubbot merged commit 0504fac into flutter:master Jul 20, 2021

blasten deleted the android_e2e_screenshot branch July 21, 2021 22:08

jmagman mentioned this pull request Jul 22, 2021

Add native iOS screenshots to integration_test #84611

Merged

hpoul mentioned this pull request Aug 5, 2021

Add platform screenshot capability to package:integration_test #51890

Open

nilsreichardt mentioned this pull request May 6, 2022

[integration_test] Missing documentation on how to compare taken screenshots (like in golden tests) #103222

Open

Uh oh!

Conversation

blasten commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

blasten commented Jun 12, 2021

Uh oh!

jiahaog commented Jun 15, 2021

Uh oh!

blasten commented Jun 26, 2021

External users

Internal users

Uh oh!

jiahaog commented Jun 28, 2021

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

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!

Uh oh!

blasten commented Jul 20, 2021

Uh oh!

skia-gold commented Jul 20, 2021

Uh oh!

skia-gold commented Jul 20, 2021

Uh oh!

flutter-dashboard Bot commented Jul 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blasten commented Jun 11, 2021 •

edited

Loading