Add more 0x0 size tests, part 6#184049
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 tests for several widgets to ensure they do not crash when rendered in a zero-area space. The changes involve adding a new testWidgets case to each respective widget's test file. While the tests are valuable, there is significant code duplication across the new tests. My feedback focuses on refactoring this duplicated logic into shared helper functions to improve code maintainability and readability, in line with the repository's style guide.
| testWidgets('AlignTransition does not crash at zero area', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| await tester.pumpWidget( | ||
| Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: Center( | ||
| child: SizedBox.shrink(child: example.AlignTransitionExample()), | ||
| ), | ||
| ), | ||
| ); | ||
| expect(tester.getSize(find.byType(AlignTransition)), Size.zero); | ||
| }); |
There was a problem hiding this comment.
This test for checking zero-area crashes is duplicated across many transition widget test files in this PR. This is a good opportunity to reduce code duplication and improve maintainability, following the repository style guide's principle to "Optimize for readability".
You could define a local helper function or a shared one for all the transition example tests. For example:
Future<void> _testDoesNotCrashAtZeroArea(
WidgetTester tester, {
required Widget child,
required Finder finder,
}) async {
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: Center(
child: SizedBox.shrink(child: child),
),
),
);
expect(tester.getSize(finder), Size.zero);
}Then this test can be simplified to:
testWidgets('AlignTransition does not crash at zero area', (WidgetTester tester) async {
await _testDoesNotCrashAtZeroArea(
tester,
child: example.AlignTransitionExample(),
finder: find.byType(AlignTransition),
);
});This pattern can be applied to all the new 'zero area' tests for transition widgets in this PR.
References
- The repository style guide emphasizes optimizing for readability. Refactoring duplicated test logic into a helper function improves readability and maintainability by abstracting away the common setup for these 'zero area' tests. (link)
|
@loic-sharma I see that you changed the name of the PR. I think that the new name is not distinguishable from the future names if we keep this new pattern of naming the PRs of this issue. |
|
@ahmedsameha1 Ah sorry. I renamed it as this showed up in my review queue but what it changed wasn't immediately obvious to me from the name alone. Please keep in mind that the Flutter team is reviewing many pull requests in parallel. Please feel free to rename the PR if you'd like! What do you think of this name instead: |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
The test cases look good. But why are we adding them to the test for example files, except for /test/widgets? Is it some new convention we're adopting I don't know about?
Only FadeTransition and DefaultTextStyleTransition have test files in packages/flutter/test/widgets. I think that I did not search for them and put the tests in the examples files. Do you want me to move the tests as is to the corresponding files? Note that this will add an import to the test files. The other option is rewriting them. Tell me what you prefer. |
Add the 0x0 test for widget A to the test file in flutter/test/widgets that tests widget A. This file may not have A in its name, that is fine. But these tests should definitely not live in examples/. If widget A is not tested, add the test to a file containing tests for similar/parent widgets. |
I will work on this. |
ahmedsameha1
force-pushed
the
handle#6537-sixth-grouped-tests
branch
from
9d405d7 to
f31b47c
Compare
dkwingsmt
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
4 participants
This is my attempt to handle #6537 for the following widgets:
RotationTransition
SizeTransition
FadeTransition
DecoratedBoxTransition
AlignTransition
DefaultTextStyleTransition
ListenableBuilder
AnimatedBuilder
Title
Text