clean up usages of resetXyz for TestFlutterView #180840
Conversation
| tester.view.physicalSize = const Size(200, 160); | ||
| tester.view.devicePixelRatio = 1.0; | ||
| addTearDown(tester.view.resetPhysicalSize); | ||
| addTearDown(tester.view.reset); |
There was a problem hiding this comment.
This sounds like a bug, since tester.view.devicePixelRatio was not reset?
| // Regression test for https://github.com/flutter/flutter/issues/112163 | ||
|
|
||
| tester.view.physicalSize = const Size(540, 340); | ||
| addTearDown(tester.view.resetPhysicalSize); |
There was a problem hiding this comment.
Moved it upwards, so that it is easy to see that the teardown is there, instead of putting it at the bottom of the test.
There was a problem hiding this comment.
Code Review
This pull request cleans up the usage of resetXyz methods for TestFlutterView in several tests, preferring TestFlutterView.reset() for conciseness and correctness. The changes are generally good, replacing multiple reset calls with a single one and fixing a bug where not all modified view properties were being reset. I've found one place where the cleanup could be improved by using addTearDown for more robust test cleanup, consistent with the changes in other files.
packages/flutter/test/widgets/two_dimensional_viewport_test.dart
Outdated
Show resolved
Hide resolved
huycozy
left a comment
There was a problem hiding this comment.
LGTM with one nit! Thanks for cleaning this!
Can you also make this for
as well?|
@Piinks Turns out that cleaning this up does reveal that usages of Who is in charge of figuring out which manual checks in dev/bots can be converted into lint rules? I'd like to reach out an propose a lint rule that makes this easier to clean up / catch in the future. |
|
Requesting @justinmc review: |
I don't quite follow? TestFlutterView is unrelated to that project. |
It's just my thought. Sorry for not making it clearer. Because I personally think it's better to complete this work before two separate packages are out, rather than 3 PRs in the future (material, cupertino and flutter repo(s)). Does this make sense? |
|
Right, all three do use TestFlutterView for some things in tests. My bad. Although the Material / Cupertino split on its own is going to take a while. I also think that if there is a |
justinmc
left a comment
There was a problem hiding this comment.
Great catches on all of these, thanks for cleaning these up! LGTM 👍
While reviewing #180728 we noticed that usage of
TestFlutterView.resetXyz()was quite liberal and some places benefit from usingTestFlutterView.reset(), to avoid repetition.This PR cleans that up.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.