Move heroes_test.dart from widgets to material directory#181266
Move heroes_test.dart from widgets to material directory#181266kazbeksultanov wants to merge 1 commit into
Conversation
Heroes tests test Material-specific hero animation behavior (MaterialApp, Scaffold, MaterialPageRoute, Card). These Material-specific tests belong in the material test directory, not widgets. This change: - Moves packages/flutter/test/widgets/heroes_test.dart to packages/flutter/test/material/ - Removes heroes_test.dart from the knownWidgetsCrossImports list in check_tests_cross_imports.dart This is part of issue flutter#177414 to organize tests in the library they're testing.
There was a problem hiding this comment.
Code Review
This pull request moves the heroes_test.dart file from packages/flutter/test/widgets/ to packages/flutter/test/material/. As a result of this move, the path to this test file is removed from the knownWidgetsCrossImports set in dev/bots/check_tests_cross_imports.dart. The changes are straightforward and correctly implement the file relocation. I have reviewed the changes and have no issues to report.
There was a problem hiding this comment.
Merely "moving" the file does not address this issue. The file also imports Cupertino, which is also not allowed.
A Hero animation is not Material specific, so the proper fix is to replace Material specific widgets in this file with their flutter/widgets counterpart.
There was a problem hiding this comment.
+1. This is a big test file, there may be some tests that truly do test Material or Cupertino explicitly and should be moved, but we should try to keep Hero tests in Widgets if possible.
There was a problem hiding this comment.
Let me start with a smaller file and I will need here TestTextField and TestWidgetsApp. But will try to figure our other way how can we do it
|
Hi @kazbeksultanov, how's this PR going? Are you still interested in finishing this PR? Thanks for the contribution so far! (from triage) |
|
Thanks for your contribution! Since I haven't seen a reply, I'm going to reluctantly close this pull request for now, but if you get time to address the comments, we'd be more than happy to take a look at a new patch. If you do send a new patch, it would be very useful to reference this one in the description in order to help reviewers see the previous context. |
Heroes tests test Material-specific hero animation behavior (MaterialApp, Scaffold, MaterialPageRoute, Card). These Material-specific tests belong in the material test directory, not widgets.
This change:
This is part of issue #177414 to organize tests in the library they're testing.
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.