Skip to content

Restore the default surface size in TestWidgetsFlutterBinding.reset#87226

Closed
jason-simmons wants to merge 1 commit intoflutter:masterfrom
jason-simmons:bug_87194_b
Closed

Restore the default surface size in TestWidgetsFlutterBinding.reset#87226
jason-simmons wants to merge 1 commit intoflutter:masterfrom
jason-simmons:bug_87194_b

Conversation

@jason-simmons
Copy link
Member

@jason-simmons jason-simmons commented Jul 28, 2021

See #87194

@jason-simmons jason-simmons requested review from dkwingsmt and flar July 28, 2021 21:16
@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 28, 2021
@google-cla google-cla bot added the cla: yes label Jul 28, 2021
flar
flar approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I didn't look too closely at the test change as I'm assuming that the change was a simple revert to the previous version without the try/finally.

LGTM.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jul 28, 2021

I'm concerned about the side effect introduced by setSurfaceSize. Maybe we just need _surfaceSize = null? It might be the cause of the current red checks.

@jason-simmons
Copy link
Member Author

jason-simmons commented Jul 28, 2021

Updated this to set _surfaceSize = null followed by a synchronous call to handleMetricsChanged

@flar flar mentioned this pull request Jul 28, 2021
Merged
8 tasks
@jason-simmons
Copy link
Member Author

jason-simmons commented Jul 28, 2021

Just doing _surfaceSize = null is insufficient to update binding state that is derived from the surface size.

But calling handleMetricsChanged in reset() is apparently breaking other tests (e.g. flutter_driver tests that are affected by task scheduling behavior)

Not sure if there is any safe way to restore this state during reset()

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jul 29, 2021

I made another attempt of this PR at #87240 and it passes. Apparently the problem is that renderView was updated with the new surface size.

@dkwingsmt dkwingsmt removed their request for review September 10, 2021 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flar flar flar approved these changes

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jason-simmons @dkwingsmt @flar