Remove feedback_tester cross-imports from material tests#184279
Conversation
There was a problem hiding this comment.
Code Review
This pull request relocates the FeedbackTester utility to the material test directory and updates import paths in several Material component tests. Feedback includes adding documentation to the FeedbackTester constructor to meet style guide requirements and refactoring the method handler to a switch statement for improved readability.
| /// It replaces the MockMethodCallHandler of [SystemChannels.platform] and | ||
| /// cannot be used in combination with other classes that do the same. | ||
| class FeedbackTester { | ||
| FeedbackTester() { |
There was a problem hiding this comment.
The public constructor is missing a documentation comment. According to the repository style guide, all public members should have documentation.
| FeedbackTester() { | |
| /// Creates a [FeedbackTester] that tracks feedback requests. | |
| /// | |
| /// The [dispose] method must be called when the tester is no longer needed. | |
| FeedbackTester() { |
References
- All public members should have documentation. (link)
| Future<void> _handler(MethodCall methodCall) async { | ||
| if (methodCall.method == 'HapticFeedback.vibrate') { | ||
| _hapticCount++; | ||
| } | ||
| if (methodCall.method == 'SystemSound.play' && | ||
| methodCall.arguments == SystemSoundType.click.toString()) { | ||
| _clickSoundCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
For improved readability and to avoid multiple string comparisons, consider refactoring the if statements into a switch statement on methodCall.method.
Future<void> _handler(MethodCall methodCall) async {
switch (methodCall.method) {
case 'HapticFeedback.vibrate':
_hapticCount++;
break;
case 'SystemSound.play':
if (methodCall.arguments == SystemSoundType.click.toString()) {
_clickSoundCount++;
}
break;
}
}|
Hi @xfce0, this PR is currently targeting |
| /// | ||
| /// It replaces the MockMethodCallHandler of [SystemChannels.platform] and | ||
| /// cannot be used in combination with other classes that do the same. | ||
| class FeedbackTester { |
There was a problem hiding this comment.
Are there tests for this class that need to be duplicated as well?
There was a problem hiding this comment.
Hello, thank you for your reply! No, FeedbackTester doesn't have its own dedicated tests — it's a test utility validated implicitly by the tests that use it. feedback_test.dart in test/widgets/ tests the Feedback widget (forTap, forLongPress), not the FeedbackTester class itself. So no test duplication is needed here
There was a problem hiding this comment.
Looks like that's right, I don't see any tests for FeedbackTester itself.
|
Retargeted to master and rebased onto the latest commit. Thanks for the heads up! |
811a6b1 to
7b5329b
Compare
| /// | ||
| /// It replaces the MockMethodCallHandler of [SystemChannels.platform] and | ||
| /// cannot be used in combination with other classes that do the same. | ||
| class FeedbackTester { |
There was a problem hiding this comment.
Looks like that's right, I don't see any tests for FeedbackTester itself.
|
Hi @xfce0, can you resolve the merge conflict in this PR? |
7b5329b to
6681e01
Compare
|
Rebased onto the latest master — the conflict was in text_field_test.dart due to the merged live_text_utils PR (#184517) |
|
And another analyzer failure @xfce0 |
7e5596e to
7e22ee5
Compare
|
Hi @justinmc @victorsanni — all CI checks are now passing (87 checks green). Could you please re-review when you have a moment? Thanks! |
|
This PR is ready to be ported over to flutter/packages in the new material_ui package! This PR is also necessary for pre-release of the material_ui package. If you are unable to work on this right now, no worries! We’ll go ahead and port it over to get it landed. Your contribution is greatly appreciated. 💙 |
…rt`, `app_bar_sliver_test.dart` and partial fix in `chip_test.dart` (#12004) Part of flutter/flutter#182636 and flutter/flutter#188395 This PR: * Removed the cross-import of `widgets/semantics_tester.dart`. * Removed @Skip tag, all tests in this file has passed. `semantics_tester.dart` has existed in material_ui, so we can directly import `semantics_tester.dart`; * Moved the file to `test/` folder. Note: `chip_test.dart` only partially remove the widgets/ dependency and still stay in `temporarily_disabled_tests` folder. Once flutter/flutter#184279 is ported over, this file can be move out of the folder. ## Pre-Review Checklist
Port over `bottom_navigation_bar_test` from flutter/flutter#184279 * Ports over `feedback_tester.dart` * Ports over `bottom_navigation_bar_test` As part of this change, moved `_findByTooltip` (originally defined in `navigation_bar_test`) into new shared `test/finders.dart` (see flutter/flutter#186966 for details). Work towards flutter/flutter#182636 and flutter/flutter#188395 ## Pre-Review Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran [the auto-formatter]. - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I followed [the version and CHANGELOG instructions], using [semantic versioning] and the [repository CHANGELOG style], or I have commented below to indicate which documented exception this PR falls under[^1]. - [x] I updated/added any relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1]. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. <!-- Links --> [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [the auto-formatter]: https://github.com/flutter/packages/blob/main/script/tool/README.md#format-code [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [linked to at least one issue that this PR fixes]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [the version and CHANGELOG instructions]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version-and-changelog-updates [semantic versioning]: https://dart.dev/tools/pub/versioning#semantic-versions [repository CHANGELOG style]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style [test exemption]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests
Part of #182636.
FeedbackTesteris a small (51-line) test utility with a trivial implementation, so duplication is the appropriate strategy per the issue guidelines. The file is copied intotest/material/, and all17 imports are updated to reference the local copy.
Pre-launch Checklist
///).