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

fix: added revertFlutterImage call in integration_tests #142166

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

YukiAttano
Copy link

@YukiAttano YukiAttano commented Jan 24, 2024

Implemented the Integration test function "revertFlutterImage()* that is missing for 3 years.

Edit:
To add some context:
The "IntegrationTestWidgetsFlutterBinding" Class provides a function called "takeScreenshot".
This function requires to call "convertFlutterSurfaceToImage" on Android devices to work.
That function states in it's documentation that you have to call "revertFlutterImage", after taking a screenshot, to further use your integration test.
The mentioned function was never publicly available but used only in the "addTeadDown" callback.

This PR adds the missing "revertFlutterImage" as a call to the API for people to use in their integration tests.

Issue List

Pre-launch Checklist

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

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. f: integration_test The flutter/packages/integration_test plugin labels Jan 24, 2024
@HansMuller HansMuller added the platform-android Android applications specifically label Jan 26, 2024
@goderbauer goderbauer added the team-android Owned by Android platform team label Jan 30, 2024
@goderbauer goderbauer requested a review from a team February 15, 2024 19:45
@johnmccutchan
Copy link
Contributor

I think you should fix takeScreenshot to do the right thing and not extend the API surface area.

@YukiAttano
Copy link
Author

YukiAttano commented Feb 16, 2024

@johnmccutchan that is indeed a point. It would be far simpler for any user if that function would do everything that is required to get that screenshot.
For now i have only provided the missing function that was mentioned years ago and people expect to be there.
Because everything here has to have an Issue first, i think your idea is then more of a feature request that needs to be addressed separately.
I'll try to follow the guidelines here, as my implemented function is already mentioned by the api, i wouldn't consider this an extension of the api surface whereas your good idea would be a "breaking change".

According to the existing function documentation, it is not possible to do it like you suggested.
From IntegrationTestWidgetsFlutterBinding.takeScreenshot()

On Android, you need to call convertFlutterSurfaceToImage(), and pump a frame before taking a screenshot.

That pumping is made in the WidgetTester class which seems to be out of scope of the Binding class, as i suppose.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. platform-android Android applications specifically team-android Owned by Android platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants