Remove list_tile_tester and rendering_tester cross-imports from cupertino tests#184588
Remove list_tile_tester and rendering_tester cross-imports from cupertino tests#184588xfce0 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces two new testing utility classes, TestListTile and TestCallbackPainter, and updates import paths in existing test files. The review feedback highlights that both new classes lack required documentation for their public members, which is a violation of the project's style guide.
| const TestListTile({super.key, this.title, this.onTap}); | ||
|
|
||
| final Widget? title; | ||
| final VoidCallback? onTap; |
There was a problem hiding this comment.
According to the Flutter style guide, all public members should have documentation. Please add documentation for the constructor and the public fields of TestListTile.
/// Creates a [TestListTile].
const TestListTile({super.key, this.title, this.onTap});
/// The widget to display in the body of the list tile.
final Widget? title;
/// The callback to call when the list tile is tapped.
final VoidCallback? onTap;References
- All public members should have documentation. (link)
| const TestCallbackPainter({required this.onPaint}); | ||
|
|
||
| final VoidCallback onPaint; |
There was a problem hiding this comment.
According to the Flutter style guide, all public members should have documentation. Please add documentation for the constructor and the public field of TestCallbackPainter.
| const TestCallbackPainter({required this.onPaint}); | |
| final VoidCallback onPaint; | |
| /// Creates a [TestCallbackPainter]. | |
| const TestCallbackPainter({required this.onPaint}); | |
| /// The callback to call when the painter paints. | |
| final VoidCallback onPaint; |
References
- All public members should have documentation. (link)
|
Note: these two test files also cross-import |
@chunhtai What are your thoughts here? 1300 lines does indeed seem like too much logic to duplicate across widgets/material/cupertino libraries. It seems preferable to move this logic to flutter_tester - is the API "good enough" to be public though? |
No, that test is not suitable for public, it is almost the same as semantics scuba diff, but worse. If they really want to assert the entire semantics tree, I would rather use semantics scuba. We also discourage use of TestSemantics #184367 so in theory no new test should be using it, and that code won't change and should slowly be deprecated, I think it is ok to duplicate the tester util to 3 places. |
victorsanni
left a comment
There was a problem hiding this comment.
Hi, can you fix the merge conflict in this PR? Thanks
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
1137133 to
2eb262b
Compare
|
Hi! Sorry for the delay — I had to rebase onto the latest master and resolve a merge conflict in I also added the missing dartdoc to the public members of |
victorsanni
left a comment
There was a problem hiding this comment.
Thanks for this PR. Can you grep all instances of TestCallbackPainter in test/cupertino and replace them with the rendering_test_utils.dart import?
| import 'rendering_test_utils.dart'; | ||
|
|
||
| /// A [CustomPainter] that calls a callback when it paints. | ||
| class TestCallbackPainter extends CustomPainter { |
There was a problem hiding this comment.
Shouldn't this class be deleted if we are now importing from rendering_test_utils.dart?
|
|
||
| /// A [CustomPainter] that invokes a callback when it paints. | ||
| class TestCallbackPainter extends CustomPainter { | ||
| const TestCallbackPainter({required this.onPaint}); |
There was a problem hiding this comment.
Should this public constructor have an API doc comment?
4604e34 to
df15b4f
Compare
victorsanni
left a comment
There was a problem hiding this comment.
Can you fix linux analyze by running dart format packages/flutter/test/cupertino/picker_test.dart?
…tino tests Duplicate TestListTile into test/cupertino/list_tile_tester.dart and TestCallbackPainter into test/cupertino/rendering_test_utils.dart to remove cross-imports from cupertino/switch_test.dart and cupertino/picker_test.dart. Part of flutter#182636.
Use local rendering_test_utils.dart instead of importing from ../rendering/rendering_tester.dart.
df15b4f to
f4fa57d
Compare
Remove cross-directory test imports of
list_tile_tester.dartandrendering_tester.dartfrom cupertino tests by duplicating the relevant trivial test utilities locally:TestListTile(~30 lines) → duplicated intotest/cupertino/list_tile_tester.dart, used bycupertino/switch_test.dartTestCallbackPainter(~10 lines) → duplicated intotest/cupertino/rendering_test_utils.dart, used bycupertino/picker_test.dartandcupertino/tab_scaffold_test.dartBoth utilities are trivial and match the "duplicate the util" criteria from the issue.
Part of #182636.
Pre-launch Checklist
///).