Add TestTextField and migrate tests#180494
Conversation
BasicTestTextField and migrate tests
5066aa8 to
13b0bb2
Compare
BasicTestTextField and migrate testsTestTextField and migrate tests
| ); | ||
| // Make sure the GestureDetector is big enough to be easily interactive if | ||
| // the handleRect is not empty. | ||
| final Rect interactiveRect = handleRect.isEmpty |
There was a problem hiding this comment.
This change was needed because
was failing due to the hit rect on the selection handle always expanding even when the rect of the handle is empty.| /// | ||
| /// This wrapper does not provide selection handles or context menus | ||
| /// out of the box. | ||
| class TestTextField extends StatefulWidget { |
There was a problem hiding this comment.
Should this live here/a separate file/or in flutter_test? I mention flutter_test because I don't think we generate dart docs for any classes in our widget tests and those may be helpful.
There was a problem hiding this comment.
Are doc macros usable here?
There was a problem hiding this comment.
I think your current location is a very good starting point. It lets us prove the API, solves our immediate need, and gives us maximum flexibility.
My suspicion is TestTextField in its current shape is most helpful for the widget library's tests, which are weird in that they cannot depend on a design system library. Other design system libraries can use their own text fields in tests directly, and might not need TestTextField at all.
Nonetheless, moving this to flutter_test seems very reasonable, but once we do so the API is locked, and our ability to do breaking changes is limited. With your current approach, we're free to move this to flutter_test whenever we'd like.
What do you think of this: design this as if it's in flutter_test, but for now keep this here?
(I don't a strong preference on whether to split this into its own file, though the current location seems very reasonable to me 👍)
There was a problem hiding this comment.
Good point, I also think TestTextField will be most useful in our own widget tests. Designing as if it's in flutter_test sgtm, these helper widgets should definitely be clearly documented so users are clear what to expect when writing their own tests.
| testWidgets( | ||
| 'Cursor animates on iOS', | ||
| (WidgetTester tester) async { | ||
| await tester.pumpWidget(const MaterialApp(home: Material(child: TextField()))); |
There was a problem hiding this comment.
I wonder if tests like these (lots in this specific file) are good candidates to be duplicated in design libraries since they test the defaults set internally by TextField and not EditableText. or is it enough that we are setting the same defaults in TestTextField. What do others think?
There was a problem hiding this comment.
My immediate though is to lean towards duplicating the tests.
My gut feeling is that we should assume TestTextField might not have high fidelity with TextField/CupertinoTextField, even if it does today. Perhaps it's best to treat TestTextField as if it's a text field from a completely different design system?
I'll defer to @justinmc here though for when they're back :)
There was a problem hiding this comment.
Long term vision: Stuff like this is system UI, and it should be in Widgets where possible. Then it could be tested in Widgets only.
If the long term vision isn't possible right now, then I would move this test to Material, because the logic that it's testing is in Material (for now at least). Any duplication of that logic in TestTextField is coincidental and doesn't need to be tested anyway.
There was a problem hiding this comment.
Yeah I would rope these tests into the bucket of "Tests that rely on defaults set in TextField or CupertinoTextField". For example "cursor animates on iOS", this is not an existing default behavior of EditableText only TextField and CupertinoTextField.
There was a problem hiding this comment.
I ended up creating a text_field_cursor_test.dart in material and cupertino with tests that verify their internal defaults respectively. I also refactored some of the cursor tests in editable_text_cursor_test.dart that were simply testing defaults in TextField, in the widgets layer we now only test that the cursor can animate, and that even when cursorOpacityAnimates is set to true, EditableText.debugDeterministicCursor still takes precedence.
commit: 454838f (#180494)
936c30d to
469162d
Compare
|
This is ready for an initial review. The way I went about this is I went through all the usages of The main indicator to whether I should duplicate a test was whether it was a regression test. There was only a handful I duplicated and added to Another question is whether we'd this There are still ~5 tests that have not been migrated to use |
| skip: kIsWeb, | ||
| ); | ||
|
|
||
| testWidgets('entering text does not scroll a surrounding PageView', (WidgetTester tester) async { |
There was a problem hiding this comment.
It's possible this test will drift over time with the corresponding one in the widgets layer. Should we add comments to keep these tests in sync? I don't feel strongly, just a thought.
There was a problem hiding this comment.
Mulling on this some more. Considering these tests will eventually move to packages in another repository, keeping these in sync will be painful. Perhaps it's not even worth attempting (and thus not worth adding these comments), but I'll leave this open in case others have more thoughts.
There was a problem hiding this comment.
I think in general we shouldn't need duplicate tests like this at all. If someone reports a bug in TextField, and it reproduces in EditableText, I think it's ok to fix it and test it only in EditableText. If the fix requires a change in TextField too, or if there's some other specific concern with TextField, only then we should test that in TextField too, IMO.
In this case I'm fine either way... It kind of makes sense to duplicate this test here because without it, we are losing test coverage that may have had some effect over the years. Keeping it in sync with its Widgets counterpart is probably not necessary, though.
| 1920.0, | ||
| 48.0, | ||
| ); //TODO(Renzo-Olivares): Should a test like this be in material,cupertino, and widgets? The shortWindowSize varies depending | ||
| // on the input field because of additional padding by decorators. |
There was a problem hiding this comment.
Don't forget to remove this TODO depending on the result of this conversation: #180494 (comment)
There was a problem hiding this comment.
Expanding from that conversation, I think here it's probably fine just in Widgets. The slight differences in shortWindowSize are probably irrelevant to the overall test. If there were specific concerns about, say, TextField's behavior in a short window due to its padding, then we could test that specific case in Material too.
There was a problem hiding this comment.
Agreed, this specific test is probably okay only being in the widgets library. The tests in this file are more about the behavior of the autocomplete menu and not the input field it is attached to.
| expect(scrollController.offset, 75.0); | ||
| expect(find.byType(EditableText), findsOneWidget); | ||
| // TODO(Renzo-Olivares): Decide if the below calculation is worthwhile to verify | ||
| // or does simply verifying that EditableText is on screen suffice. |
There was a problem hiding this comment.
I'm good with the keeping the calculations below if that's what you choose to do 👍
There was a problem hiding this comment.
I appreciate all the thorough math here! 👍
| } | ||
| } | ||
|
|
||
| final TextSelectionControls testTextSelectionHandleControls = TestTextSelectionHandleControls(); |
There was a problem hiding this comment.
Just curious, did you consider a static field named instance on TestTextSelectionHandleControls? That might be a little cleaner than a top-level global, but I don't feel strongly.
There was a problem hiding this comment.
I did not consider it, I mainly followed the pattern we use in the framework (top-level global) for other text selection controls like materialTextSelectionHandleControls. It eliminates some verbosity, see below.
TestTextSelectionHandleControls.testTextSelectionHandleControls vs testTextSelectionHandleControls.
loic-sharma
left a comment
There was a problem hiding this comment.
This looks great, excellent work!
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍. Thank you @Renzo-Olivares for doing this, it's really a big help for these tests!
After writing this PR, do you have any more thoughts on whether or not some kind of widget like TestTextField might be useful for developers using only the Widgets library? Talking about outside of tests.
Also just double checking, does this PR remove all uses of TextField from Widgets?
| skip: kIsWeb, | ||
| ); | ||
|
|
||
| testWidgets('entering text does not scroll a surrounding PageView', (WidgetTester tester) async { |
There was a problem hiding this comment.
I think in general we shouldn't need duplicate tests like this at all. If someone reports a bug in TextField, and it reproduces in EditableText, I think it's ok to fix it and test it only in EditableText. If the fix requires a change in TextField too, or if there's some other specific concern with TextField, only then we should test that in TextField too, IMO.
In this case I'm fine either way... It kind of makes sense to duplicate this test here because without it, we are losing test coverage that may have had some effect over the years. Keeping it in sync with its Widgets counterpart is probably not necessary, though.
| testWidgets( | ||
| 'Cursor animates on iOS', | ||
| (WidgetTester tester) async { | ||
| await tester.pumpWidget(const MaterialApp(home: Material(child: TextField()))); |
There was a problem hiding this comment.
Long term vision: Stuff like this is system UI, and it should be in Widgets where possible. Then it could be tested in Widgets only.
If the long term vision isn't possible right now, then I would move this test to Material, because the logic that it's testing is in Material (for now at least). Any duplication of that logic in TestTextField is coincidental and doesn't need to be tested anyway.
| 1920.0, | ||
| 48.0, | ||
| ); //TODO(Renzo-Olivares): Should a test like this be in material,cupertino, and widgets? The shortWindowSize varies depending | ||
| // on the input field because of additional padding by decorators. |
There was a problem hiding this comment.
Expanding from that conversation, I think here it's probably fine just in Widgets. The slight differences in shortWindowSize are probably irrelevant to the overall test. If there were specific concerns about, say, TextField's behavior in a short window due to its padding, then we could test that specific case in Material too.
| expect(scrollController.offset, 75.0); | ||
| expect(find.byType(EditableText), findsOneWidget); | ||
| // TODO(Renzo-Olivares): Decide if the below calculation is worthwhile to verify | ||
| // or does simply verifying that EditableText is on screen suffice. |
There was a problem hiding this comment.
I appreciate all the thorough math here! 👍
I found that in the Widgets library convenient usages of A few tests fell into the category of testing defaults set by In general I didn't find any additional evidence from the convenient usages of
There are still around ~5 tests that need a deeper look when migrating. |
|
Main changes from last review are in |
2e5bbfb to
8f4ce35
Compare
Roll Flutter from da72d5936d69 to 1d9d6a9a5ef6 (33 revisions) flutter/flutter@da72d59...1d9d6a9 2026-01-30 [email protected] enable enhanced debugging for GLES playground (flutter/flutter#181157) 2026-01-30 [email protected] Make the Windows windowing_test in .ci.yaml have bringup as false (flutter/flutter#181664) 2026-01-30 [email protected] Roll Packages from cd4fd61 to 510dd40 (4 revisions) (flutter/flutter#181726) 2026-01-30 [email protected] Roll Skia from edbf7e9eb846 to 4745eb2fe837 (1 revision) (flutter/flutter#181725) 2026-01-30 [email protected] Enhance error handling of WidgetsBindingObserver callbacks (flutter/flutter#181174) 2026-01-30 [email protected] Roll Skia from 05d3cb9d2be9 to edbf7e9eb846 (2 revisions) (flutter/flutter#181715) 2026-01-30 [email protected] Roll Skia from c198e5fa9cd9 to 05d3cb9d2be9 (1 revision) (flutter/flutter#181712) 2026-01-30 [email protected] Roll Dart SDK from 920b7e24583e to 2703fd9733ce (2 revisions) (flutter/flutter#181693) 2026-01-30 [email protected] Roll Skia from b9f40c193e7a to c198e5fa9cd9 (6 revisions) (flutter/flutter#181692) 2026-01-30 [email protected] Roll pub packages (flutter/flutter#181690) 2026-01-30 [email protected] Extend the Windows tool_integration_tests_2_9 shard timeout to 1 hour (flutter/flutter#181678) 2026-01-29 [email protected] Add `android_sdk` dependency to `android_engine_opengles_tests` (flutter/flutter#181681) 2026-01-29 [email protected] Roll Dart SDK from a0685c8e946b to 920b7e24583e (3 revisions) (flutter/flutter#181680) 2026-01-29 [email protected] Roll Skia from 128b5213711e to b9f40c193e7a (14 revisions) (flutter/flutter#181675) 2026-01-29 [email protected] [Impeller] Ensure that HostBuffers/DeviceBuffers allocated by RendererTest tests are valid for the lifetime of the RenderPass (flutter/flutter#181635) 2026-01-29 [email protected] [Impeller] Fix off-by-one indices in the SimilarPointPair/SimilarPointTrio functions used by ShadowPathGeometryTest (flutter/flutter#181623) 2026-01-29 [email protected] 180162 fix radio list tile and switch list tile accept widget states controller (flutter/flutter#180367) 2026-01-29 [email protected] Remove unused test file (flutter/flutter#181671) 2026-01-29 [email protected] Roll libpng to version 1.6.54 (flutter/flutter#181625) 2026-01-29 [email protected] Remove nonstandard ndkpath for `hybrid_android_views` integration test (flutter/flutter#181666) 2026-01-29 [email protected] Add TestWidgetsApp utility and refactor widget tests to use WidgetsApp (flutter/flutter#180456) 2026-01-29 [email protected] Add `TestTextField` and migrate tests (flutter/flutter#180494) 2026-01-29 [email protected] Merge changelog for 3.38.9 (flutter/flutter#181668) 2026-01-29 [email protected] [flutter_tools] Deprecate `plugin_ffi` template (flutter/flutter#181588) 2026-01-29 [email protected] Deprecate onReorder callback (flutter/flutter#178242) 2026-01-29 [email protected] [web] Use defensive null check in text editing placeElement (flutter/flutter#180795) 2026-01-29 [email protected] Roll Packages from 1cb2148 to cd4fd61 (4 revisions) (flutter/flutter#181663) 2026-01-29 [email protected] Roll Skia from 89df65f8324c to 128b5213711e (2 revisions) (flutter/flutter#181651) 2026-01-29 [email protected] [hooks] Don't run build hooks for code assets in `flutter run` (flutter/flutter#181542) 2026-01-29 [email protected] Roll Dart SDK from f10dcbfca98f to a0685c8e946b (5 revisions) (flutter/flutter#181653) 2026-01-29 [email protected] Fixes getUniformX for Vulkan (flutter/flutter#181286) 2026-01-29 [email protected] Roll Fuchsia Linux SDK from adhoq9ouVRh0xzkm3... to isy1ARvK-3bsvtfc-... (flutter/flutter#181641) 2026-01-29 [email protected] Add isDark, isLight, and isSystem getters to ThemeMode (flutter/flutter#181475) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: ...
This PR introduces the convenience widget `TestTextField` in the framework widget tests. This new widget should be used in place of convenient `TextField` or `CupertinoTextField` usages in the widgets library tests. part of: flutter#177415 ## 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] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <[email protected]>
This PR introduces the convenience widget `TestTextField` in the framework widget tests. This new widget should be used in place of convenient `TextField` or `CupertinoTextField` usages in the widgets library tests. part of: flutter#177415 ## 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] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <[email protected]>
This PR introduces the convenience widget `TestTextField` in the framework widget tests. This new widget should be used in place of convenient `TextField` or `CupertinoTextField` usages in the widgets library tests. part of: flutter#177415 ## 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] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <[email protected]>
This PR introduces the convenience widget
TestTextFieldin the framework widget tests. This new widget should be used in place of convenientTextFieldorCupertinoTextFieldusages in the widgets library tests.part of: #177415
Pre-launch Checklist
///).