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

Make LiveTestWidgetsFlutterBinding work with setSurfaceSize and live tests #86449

Merged
merged 3 commits into from Jul 19, 2021

Conversation

dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jul 14, 2021

This PR fixes several bugs related to live tests, adds more tests, and completes the documentation of several methods related to pointer events. More specifically:

The several tests for LiveTestWidgetsFlutterBinding added by #83337 were discovered by #82712 (comment) to fail when setSurfaceSize is called. This PR fixed these problems and added a setSurfaceSize version for these tests.

This PR fixes few problems of live tests:

  • [codelab][flutter_app_testing] Widget test fails when run on device #82542 reported that simulated pointer events do not work on live tests (for example, some code lab tests are broken).
  • Tapping the screen on live tests should print the hit widgets, instead the events are currently forwarded to the app and painted.
  • The painted event positions currently do not match the positions where the user interact.

This PR changed the testing widget used in animation/live_binding_test.dart. The target widget is now an InkWell to clearly ensure that the hitTest occurs at the intended local position.

Finally, this PR updated some documentations for some methods, most importantly, whether a pointer event should be in the global coordinate system or the local one.

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.

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jul 14, 2021
@dkwingsmt dkwingsmt marked this pull request as draft July 14, 2021 22:21
@google-cla google-cla bot added the cla: yes label Jul 14, 2021
@skia-gold
Copy link

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

@skia-gold
Copy link

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

@skia-gold
Copy link

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

@skia-gold
Copy link

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

@dkwingsmt dkwingsmt changed the title Cover setSurfaceSize for live widget test binding Refine tests and docs related to LiveTestWidgetsFlutterBinding Jul 16, 2021
@dkwingsmt dkwingsmt changed the title Refine tests and docs related to LiveTestWidgetsFlutterBinding Make LiveTestWidgetsFlutterBinding work with setSurfaceSize Jul 16, 2021
@dkwingsmt dkwingsmt changed the title Make LiveTestWidgetsFlutterBinding work with setSurfaceSize Make LiveTestWidgetsFlutterBinding work with setSurfaceSize and live tests Jul 16, 2021
@dkwingsmt dkwingsmt marked this pull request as ready for review July 16, 2021 01:35
@skia-gold
Copy link

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

@skia-gold
Copy link

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

@flutter-dashboard
Copy link

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 #86449 at sha b3a4553bcbf9969b44756019a345717b72900693

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Jul 16, 2021
Fix

codelab works

Doc and print

Fix tests
@dkwingsmt dkwingsmt force-pushed the set-surface-size-test-messages branch from b3a4553 to 559b13a Compare July 16, 2021 07:55
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

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 #86449 at sha 559b13a

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for debugging this!

packages/flutter_test/lib/src/binding.dart Outdated Show resolved Hide resolved
packages/flutter_test/lib/src/binding.dart Show resolved Hide resolved
packages/flutter_test/lib/src/binding.dart Show resolved Hide resolved
packages/flutter_test/lib/src/widget_tester.dart Outdated Show resolved Hide resolved
packages/flutter_test/lib/src/widget_tester.dart Outdated Show resolved Hide resolved
packages/flutter_test/lib/src/widget_tester.dart Outdated Show resolved Hide resolved
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

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 #86449 at sha f40be96

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/// * The _global coordinate space_ is the one used by the device.
///
/// Positions can be transformed between coordinate spaces with [localToGlobal]
/// and [globalToLocal].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice explanation! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note that global and local coordinate space are usually identically when run in the headless flutter_tester.

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? I thought they are still 800x600 and 2400x1800 respectively, which still require transforming. Also I worry if that comment suggests people don't need to worry about transforming in normal cases - they always should!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave the comment as is.

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

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 #86449 at sha 8219ea8

@dkwingsmt dkwingsmt merged commit cd78190 into flutter:master Jul 19, 2021
@dkwingsmt dkwingsmt deleted the set-surface-size-test-messages branch July 19, 2021 21:54
@zanderso
Copy link
Member

It looks like some issue with the goldens precipitated by this change is affecting rolls from Engine and Plugins:

See:
#86687
#86690

@Piinks
Copy link
Contributor

Piinks commented Jul 20, 2021

@dkwingsmt please triage images before manually merging PRs. They end up causing errant golden file failures for other PRs.

Is it possible that this change causes an inconsistent image? It look like it has been different for each commit following this change, although it could be caused by the untriaged image errors. They appear to be triaged now, so if they continue to vary for every commit there may be something wrong with the test.

Piinks added a commit that referenced this pull request Jul 20, 2021
Piinks added a commit that referenced this pull request Jul 20, 2021
dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jul 20, 2021
dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jul 22, 2021
…ceSize and live tests (flutter#86449)" (flutter#86730)"

This reverts commit 8802e32.

Fix pointer

Trigger Build

Change test to gesture detector
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 framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants