Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support native view screenshot in e2e integration tests #83856

Closed
blasten opened this issue Jun 3, 2021 · 27 comments
Closed

Support native view screenshot in e2e integration tests #83856

blasten opened this issue Jun 3, 2021 · 27 comments
Labels
customer: dream (g3) customer: google Various Google teams customer: money (g3) f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list t: flutter driver "flutter driver", flutter_drive, or a driver test

Comments

@blasten
Copy link

blasten commented Jun 3, 2021

Integration tests currently returns a Skia raster image.
This is fine for Flutter widgets, but it misses platform views.

As a result, customer: money or dream cannot test platform views.
Plugins like google_mobile_ads cannot have their own driver tests
that ensure that the native view is sized properly, etc...

We also happen to have the scenario app in the engine, which has
a workaround for this limitation. This workaround breaks occasionally,
and doesn't work with Skia Gold.

For all of these issues, I propose adding a new API to Flutter drive that takes the
device's screenshot and crops the status or action bars if needed
(This will be required on Android if adb is used).

cc @dnfield, @xster, @jmagman for iOS too.

Related bug: http://b/180715355

@blasten blasten added t: flutter driver "flutter driver", flutter_drive, or a driver test customer: google Various Google teams customer: money (g3) labels Jun 3, 2021
@dnfield
Copy link
Contributor

dnfield commented Jun 3, 2021

How will we deal with synchronization issues?

@blasten
Copy link
Author

blasten commented Jun 3, 2021

what is an example of a "synchronization issue" ?

@dnfield
Copy link
Contributor

dnfield commented Jun 3, 2021

Making sure the platform view has finished loading/animating before taking a screenshot

@blasten
Copy link
Author

blasten commented Jun 3, 2021

oh I see. If SurfaceView is used android.view.Choreographer cannot be used. If FlutterImageView is used, we could schedule the screenshot the next Android frame. Luckily, this is already the case with platform views that use HC.

@blasten blasten added the P1 High-priority issues at the top of the work list label Jun 3, 2021
@blasten
Copy link
Author

blasten commented Jun 3, 2021

Also, at this time we have merged the threads, so the raster and the platform threads are the same.

I was looking at the 2s delay described in the public docs. I think if we get the next frame callback from the platform, and the raster and platform threads are merged, we should be able to remove the 2s if wanted to.

On Android at least, the only issue is that SurfaceView/SurfaceTextureView aren't synchronized to the Android view hierarchy.

@jiahaog
Copy link
Member

jiahaog commented Jun 3, 2021

adding a new API to Flutter drive

Is this going to be an API on the Flutter Driver host tools or within the Flutter Driver extension? The latter would be preferable so that it can be reused for package:integration_test as well.

@jmagman
Copy link
Member

jmagman commented Jun 3, 2021

I added flutter drive --screenshot to the tool in #78822 but it only fires when drive fails. They are uploaded in our LUCI tests.
For example: https://ci.chromium.org/p/flutter/builders/prod/Mac_ios%20integration_ui_ios_textfield/1060
Screen Shot 2021-06-03 at 10 11 28 AM
Shows the device looked like this when it failed:

drive_01-2

@jmagman
Copy link
Member

jmagman commented Jun 4, 2021

I added flutter drive --screenshot to the tool in #78822 but it only fires when drive fails. They are uploaded in our LUCI tests.

@blasten in light of this, what additional functionality is this feature tracking?

@blasten
Copy link
Author

blasten commented Jun 4, 2021

oh flutter drive --screenshot is more for debugging, right? This is tracking an API that can generate images from a driver test to use as baseline for future invocations of the test.

@jmagman
Copy link
Member

jmagman commented Jun 4, 2021

oh flutter drive --screenshot is more for debugging, right? This is tracking an API that can generate images from a driver test to use as baseline for future invocations of the test.

If you want the platform views/keyboards, etc it should work. --screenshot is using the tool's platform-specific flutter screenshot logic, so uses adb, idevicescreenshot, fuchsia's screencap, custom device registered hooks, etc.

It doesn't have your "crops the status or action bars if needed" feature, though (not sure how that would work). As long as the same simulator version is used every time to compare goldens you probably don't need to crop it (that's what the scenario app does). I may be missing some additional features though.

One thing that would be really nice is for the test itself to be able to trigger screenshots or screen recordings (tap button, screenshot, start screen recording, scroll, stop screen recording, tap button, screenshot), which would maybe would involve a VM service hook the tool responds to? Not sure...

@blasten
Copy link
Author

blasten commented Jun 4, 2021

One thing that would be really nice is for the test itself to be able to trigger screenshots or screen recordings (tap button, screenshot, start screen recording, scroll, stop screen recording, tap button, screenshot), which would maybe would involve a VM service hook the tool responds to? Not sure...

Right. I think that is the use case that I had in mind.

@jiahaog could you please describe how this is set up in internally in terms of dependencies? Could the bazel rule trigger flutter drive --screenshot <file>, and send it to scuba?

@blasten
Copy link
Author

blasten commented Jun 4, 2021

We have bugs where we don't show the keyboard when the user focuses a Flutter input, etc. I think that if we can get the keyboard in the actual screenshot, we would have a way of testing that.

@tvolkert
Copy link
Contributor

tvolkert commented Jun 4, 2021

I thought we were getting away from package:flutter_driver towards package:integration_test. Does that affect this at all?

@blasten
Copy link
Author

blasten commented Jun 4, 2021

yep. package:integration_test is used by google3 (@jiahaog to confirm.)

@blasten blasten changed the title Support native view screenshot in Flutter drive Support native view screenshot in e2e integration tests Jun 4, 2021
@blasten
Copy link
Author

blasten commented Jun 4, 2021

There was a feature request filed for web: #51890.

On web, the DevTools protocol can be used, so if we decide to do this, we should ensure that it can work with this protocol too: https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-captureScreenshot. cc @yjbanov

@jiahaog
Copy link
Member

jiahaog commented Jun 5, 2021

In Google3, Integration Tests / Driver tests do not use the Flutter tool at all, so using flutter drive to support this may not be the right approach internally.

  • For Flutter Driver tests, the host script runs as a generic Dart VM test (using host dependencies from package:flutter_driver) which communicates with the device over the observatory
  • For Integration Tests, dependencies from package:flutter_test and package:integration_test are set up to run within the test application on the device. GoldenFileComparator is wired up with generic screenshotting tools.

@tvolkert
Copy link
Contributor

tvolkert commented Jun 7, 2021

yep. package:integration_test is used by google3

Irrespective of google3, we're trying to move away from flutter drive in favor of on-device tests except for the (very few) cases where flutter drive needed. e.g. most of our devicelab tests should move to on-device tests using package:integration_test. So I want to make sure we're not adding features to the thing we're trying to reduce the usage of 🙂

@blasten
Copy link
Author

blasten commented Jun 7, 2021

So I want to make sure we're not adding features to the thing we're trying to reduce the usage of 🙂

Sounds good.

On a separate note, flutter/engine#26602 could use this feature. Once we increase API level coverage, we will be able to ensure that input focus and keyboard interactions work as expected.

@blasten
Copy link
Author

blasten commented Jun 12, 2021

#56591 appears to be a blocker for this. Currently, Gradle test-only dependencies end up in the release APK. cc @jonahwilliams

@jonahwilliams
Copy link
Member

jonahwilliams commented Jun 12, 2021

I don't see why #56591 would be a blocker - assuming you're already using the integrtion_test package then you already have that problem?

@blasten
Copy link
Author

blasten commented Jun 14, 2021

assuming you're already using the integrtion_test package then you already have that problem?

Right. The release APK contains test-only dependencies as seen below.

Screen Shot 2021-06-14 at 10 46 48 AM

AGP has a configuration (androidTestImplementation) that generates different dependency graphs depending on the task, but it requires a different directory layout than the one used by plugins public APIs. e.g. androidTest/java, etc...

@Sunbreak
Copy link
Contributor

Sunbreak commented Aug 6, 2021

A lot of related issues are locked: #25306

Do we have any progress here?

@tvolkert
Copy link
Contributor

tvolkert commented Aug 6, 2021

@Sunbreak this is actively being worked on.

@Pulkit07
Copy link

Hey, we are looking to take screenshots of camera view using screenshot plugin but are unable to do so because of this issue. Are there any updates on this?

Thanks for all the work and amazing framework!

@jmagman
Copy link
Member

jmagman commented Mar 18, 2022

https://github.com/flutter/flutter/tree/master/packages/integration_test#screenshots

@blasten do you know what is still needed for this feature?

@blasten
Copy link
Author

blasten commented Mar 19, 2022

do you know what is still needed for this feature?

This is functional on Android. As far as Android support is concerned, this is done.
Unless there's anything pending from iOS, this issue can be closed.

We are tracking another issue #97853 about adding tests that need this e2e screenshot functionality.

@Pulkit07 what issue did you run into?

@blasten blasten closed this as completed Mar 19, 2022
@github-actions
Copy link

github-actions bot commented Apr 2, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
customer: dream (g3) customer: google Various Google teams customer: money (g3) f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list t: flutter driver "flutter driver", flutter_drive, or a driver test
Development

No branches or pull requests

10 participants