Make sure that a DesktopTextSelectionToolbarButton doesn't crash in 0… #173827
Conversation
github-actions
bot
added
a: text input
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a regression test to ensure DesktopTextSelectionToolbarButton doesn't crash when rendered in a zero-sized area. This is a valuable addition to prevent future regressions. My review includes a suggestion to make the new test more robust by adding an explicit assertion.
| await tester.pumpWidget( | ||
| const MaterialApp( | ||
| home: SizedBox.shrink( | ||
| child: DesktopTextSelectionToolbarButton(onPressed: null, child: Text('X')), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
This test correctly checks for rendering crashes. To make it more robust, consider adding an assertion to verify that the DesktopTextSelectionToolbarButton is actually rendered in the widget tree.1 This ensures the test is truly exercising the render path and would fail if the widget were, for example, optimized out of the build entirely.
await tester.pumpWidget(
const MaterialApp(
home: SizedBox.shrink(
child: DesktopTextSelectionToolbarButton(onPressed: null, child: Text('X')),
),
),
);
expect(find.byType(DesktopTextSelectionToolbarButton), findsOneWidget);
Style Guide References
Footnotes
|
Could you also do this #173677 (comment), in this PR? |
|
I think there is no need to do this, according to: #6537 (comment) |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 I agree no takeException is needed when it's at the end of a test like this. I typically use it when it would be beneficial for the test to fail before the end of the test if there is an exception.
|
I guess you could expect that the size of DesktopTextSelectionToolbarButton is indeed 0x0? |
Do you want me to add this expectation? |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Yes please add that expectation. I just talked with @LongCatIsLooong about this and we agree.
|
@justinmc Could you resolve this in #6537 because, according to #6537 (comment), there is no need to add that expectation. I think that this expectation is not specific to this widget, or is it? |
ahmedsameha1
force-pushed
the
handle#6537-DesktopTextSelectionToolbarButton
branch
from
f7f602b to
96bea17
Compare
|
I have updated this pull request according to #6537 (comment). |
packages/flutter/test/material/desktop_text_selection_toolbar_button_test.dart
Outdated
Show resolved
Hide resolved
ahmedsameha1
force-pushed
the
handle#6537-DesktopTextSelectionToolbarButton
branch
from
cbad6c1 to
24f659a
Compare
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Checklist:
- The test is in the correct file
- The test name goes “does not crash at zero area”
- The target widget is wrapped by
Center(or is fullscreen) - The target widget does not have an overlay, or the overlay is tested
- The target widget is expected to have a size of exactly
Size.zero
| ), | ||
| ), | ||
| ); | ||
| expect(tester.getSize(find.byType(DesktopTextSelectionToolbarButton)).isEmpty, isTrue); |
There was a problem hiding this comment.
| expect(tester.getSize(find.byType(DesktopTextSelectionToolbarButton)).isEmpty, isTrue); | |
| expect(tester.getSize(find.byType(DesktopTextSelectionToolbarButton)), Size.zero); |
dkwingsmt
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/173827, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
bot
removed
the
autosubmit
flutter#173827) This is my attempt to handle flutter#6537 for the DesktopTextSelectionToolbarButton UI control. --------- Co-authored-by: Tong Mu <[email protected]>
4 participants
This is my attempt to handle #6537 for the DesktopTextSelectionToolbarButton UI control.