[flutter_test/integration_test] added setSurfaceSize test coverage by prefanatic · Pull Request #82712 · flutter/flutter

prefanatic · 2021-05-17T17:05:37Z

This PR includes an updated test case for integration_test and a new test case for flutter_test to ensure that hit tests transform properly when using setSurfaceSize, for both the IntegrationTestWidgetsFlutterBinding and LiveTestWidgetsFlutterBinding bindings.

Aims to resolve #82557

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.

google-cla · 2021-05-17T17:05:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

prefanatic · 2021-05-17T17:07:13Z

@googlebot I signed it!

goderbauer · 2021-05-20T20:50:50Z

packages/integration_test/test/binding_test.dart

Maybe create a separate hitTesting works when using setSurfaceSize to keep the tests small and focused on one aspect at a time.

Thanks for the review - I've decoupled the focused hitTesting works when using setSurfaceSize out from the existing setSurfaceSize works, and have applied this to both the integration_test suite and flutter_test suite.

goderbauer · 2021-05-20T20:51:11Z

packages/flutter_test/test/live_binding_test.dart

nit: indentation

goderbauer · 2021-05-20T22:03:52Z

packages/flutter_test/lib/src/binding.dart

Removing this seems to break the logic where you can just flutter run a test, tap on the screen and it would tell you potential finders for widgets that are under your finger. (Looks like we don't have a test for that behavior, though 😞.) Maybe the transform logic has to move to a place where it only triggers for real pointer events and not for test-simulated ones?

/cc @dkwingsmt since the PR author bisected that this broke with #64846, which changed how test-simulated and real pointer events are routed in tests.

I'm adding a unit test for the behavior you described and let's see if we can fix this issue without breaking it.

I believe the tests where added in #83337. @prefanatic Can you rebase this PR to the latest master and see if this is breaking anything?

@goderbauer Thanks, I've rebased the PR and pushed, looks green.

dkwingsmt · 2021-06-02T21:45:31Z

I've added some unit tests for the features that we were afraid of breaking. Can you sync with master and see if all tests pass?

prefanatic · 2021-06-30T16:07:13Z

@goderbauer Sorry, just getting back around to this --

I'm hoping you can help clear things up for me, trying to study how these pointer events are dispatched. It's clear that removing hitTest from _LiveTestRenderView may not be the solution, as it does indeed continue to break the "Some possible finders" hints when running tests under flutter run.

The tests added in #83337 cover "Some possible finders" but not under a setSurfaceSize case. When applying setSurfaceSize to these tests, they fail - presumably in a similar fashion to tests added in this PR.

From what I can see, the coordinates from finders determined by getCenter -> _getElementPoint are eagerly transformed with the correct scale in mind, through RenderBox.localToGlobal(). For hit testing under setSurfaceSize, this already transformed Offset eventually passes through _LiveTestRenderView.hitTest which does another transformation. This results in tapping on the wrong spot, and warning about a missed hit.

Now, for "Some possible finders", the pointer event does not seem to be transformed until _LiveTestRenderView.hitTest, thus the breakage with the original fix.

I think my direction here should be to ensure that pointer events that drive "Some possible finders" should not rely on _LiveTestRenderView.hitTest for transformation? In general, _LiveTestRenderView.hitTest also causes problems with warnIfMissed on taps, as a similar double-transform occurs, where the result of localToGlobal is sent to hitTest.

goderbauer · 2021-07-14T23:14:03Z

I think my direction here should be to ensure that pointer events that drive "Some possible finders" should not rely on _LiveTestRenderView.hitTest for transformation?

That sounds like a good direction, yes. Alternatively, we could remove the eager transformation of coordinates from finders, but that would probably mess up running tests under the regular TestWidgetsFlutterBinding and require more adjustments there.

The tests added in #83337 cover "Some possible finders" but not under a setSurfaceSize case. When applying setSurfaceSize to these tests, they fail - presumably in a similar fashion to tests added in this PR.

@dkwingsmt was experimenting with a test for this scenario in #86449 (see "Should show event indicator for pointer events with setSurfaceSize" test) and also discovered that those are failing. Can you add that test case to this PR as well so we can make sure it gets fixed with whatever approach we come up with here? Thanks!

dkwingsmt · 2021-07-16T01:37:13Z

#86449 is almost complete and should fix many problems with live tests and setSurfaceSize. Can you take a look if your original problems persist? Also if you'd like we can still merge this PR for the additional tests.

goderbauer · 2021-07-16T22:25:21Z

Also if you'd like we can still merge this PR for the additional tests.

Yes, let's keep this tests to ensure we have good test coverage.

prefanatic · 2021-07-18T15:22:47Z

Thank you both! With #86449, the original issue is resolved and the test cases in this PR pass.

I've restructured this PR to represent only the added test coverage.

skia-gold · 2021-07-18T15:29:19Z

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

goderbauer

LGTM once #86449 is merged and these tests pass

dkwingsmt

LGTM

dkwingsmt · 2021-07-19T21:56:00Z

Now that #86449 is merged, can you update with master and convert this PR out of draft?

skia-gold · 2021-07-20T14:00:34Z

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

flutter-dashboard · 2021-07-20T14:15:29Z

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 #82712 at sha 3779bf2df80552f26253c4f4d73e141ab8348df2

goderbauer · 2021-07-28T21:57:41Z

@prefanatic Can you try rebasing this PR again to get the gold check to pass? Thanks!

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 May 17, 2021

google-cla bot added the cla: no label May 17, 2021

google-cla bot added cla: yes and removed cla: no labels May 17, 2021

prefanatic mentioned this pull request May 17, 2021

[integration_test] HitTest regression with setSurfaceSize #82557

Closed

goderbauer reviewed May 20, 2021

View reviewed changes

prefanatic force-pushed the integration_test_resize_taps branch from 4c25b84 to 98d3d9c Compare June 7, 2021 14:51

Piinks added the f: integration_test The flutter/packages/integration_test plugin label Jun 10, 2021

prefanatic marked this pull request as ready for review June 13, 2021 16:11

prefanatic marked this pull request as draft June 29, 2021 14:44

goderbauer requested a review from dkwingsmt July 12, 2021 20:50

dkwingsmt mentioned this pull request Jul 16, 2021

Make LiveTestWidgetsFlutterBinding work with setSurfaceSize and live tests #86449

Merged

8 tasks

prefanatic force-pushed the integration_test_resize_taps branch from 98d3d9c to 70cab82 Compare July 18, 2021 15:22

prefanatic changed the title ~~[flutter_test/integration_test] Fix setSurfaceSize regression~~ [flutter_test/integration_test] added setSurfaceSize test coverage Jul 18, 2021

goderbauer approved these changes Jul 19, 2021

View reviewed changes

dkwingsmt approved these changes Jul 19, 2021

View reviewed changes

prefanatic force-pushed the integration_test_resize_taps branch from 70cab82 to 3779bf2 Compare July 20, 2021 13:27

prefanatic marked this pull request as ready for review July 20, 2021 13:43

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

prefanatic force-pushed the integration_test_resize_taps branch from 3779bf2 to 67d9206 Compare July 29, 2021 12:52

prefanatic added 2 commits July 30, 2021 10:09

flutter_test - setSurfaceSize tests

33c35ca

integration_test - setSurfaceSize test

24466a3

prefanatic force-pushed the integration_test_resize_taps branch from 67d9206 to 24466a3 Compare July 30, 2021 14:09

dkwingsmt merged commit ab6ce71 into flutter:master Jul 30, 2021

Conversation

prefanatic commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

google-cla bot commented May 17, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

prefanatic commented May 17, 2021

Uh oh!

goderbauer May 20, 2021

Choose a reason for hiding this comment

Uh oh!

prefanatic May 26, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer May 20, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer May 20, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 25, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

prefanatic Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jun 2, 2021

Uh oh!

prefanatic commented Jun 30, 2021

Uh oh!

goderbauer commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt commented Jul 16, 2021

Uh oh!

goderbauer commented Jul 16, 2021

Uh oh!

prefanatic commented Jul 18, 2021

Uh oh!

skia-gold commented Jul 18, 2021

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jul 19, 2021

Uh oh!

skia-gold commented Jul 20, 2021

Uh oh!

flutter-dashboard bot commented Jul 20, 2021

Uh oh!

goderbauer commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

prefanatic commented May 17, 2021 •

edited

Loading

goderbauer commented Jul 14, 2021 •

edited

Loading