[material_ui] Migrate off of flutter_test's find.byTooltip and on to the local findByTooltip - #12492
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces usages of find.byTooltip with a custom findByTooltip helper function across multiple test files in packages/material_ui. Feedback on the changes suggests addressing the duplication of the findByTooltip helper function in the example test files by extracting it into a shared location. Additionally, the feedback recommends replacing unsafe null assertions (!) on widget.richMessage with optional chaining (?.) and a fallback value to prevent potential runtime exceptions.
| /// Finds [RawTooltip] or [Tooltip] widgets with the given `message`. | ||
| /// | ||
| /// ## Sample code | ||
| /// | ||
| /// ```dart | ||
| /// expect(findByTooltip('Back'), findsOneWidget); | ||
| /// expect(findByTooltip(RegExp('Back.*')), findsNWidgets(2)); | ||
| /// ``` | ||
| /// | ||
| /// If the `skipOffstage` argument is true (the default), then this skips | ||
| /// nodes that are [Offstage] or that are from inactive [Route]s. | ||
| /// | ||
| /// This was copied from flutter_test, which uses flutter/material.dart. | ||
| /// | ||
| // TODO(justinmc): Port flutter_test to material_ui, then delete this method and | ||
| // use that one. See https://github.com/flutter/flutter/issues/186966 | ||
| Finder findByTooltip(Pattern message, {bool skipOffstage = true}) { | ||
| return find.byWidgetPredicate((Widget widget) { | ||
| // Compare RawTooltip's semantics tooltip with the given message. | ||
| // However, Tooltip's message needs to be checked directly if: | ||
| // 1. Tooltip.excludeFromSemantics is true, since in this case Tooltip | ||
| // provides no semantics tooltip to the underlying RawTooltip. | ||
| // 2. Tooltip.message and Tooltip.richMessage are empty, since in this | ||
| // case no RawTooltip is created. | ||
| if (widget is Tooltip) { | ||
| final String tooltipMessage = | ||
| widget.message ?? widget.richMessage!.toPlainText(); | ||
| if ((widget.excludeFromSemantics ?? false) || tooltipMessage.isEmpty) { | ||
| return message is RegExp | ||
| ? message.hasMatch(tooltipMessage) | ||
| : tooltipMessage == message; | ||
| } | ||
| } | ||
| return widget is RawTooltip && | ||
| (message is RegExp | ||
| ? message.hasMatch(widget.semanticsTooltip ?? '') | ||
| : widget.semanticsTooltip == message); | ||
| }, skipOffstage: skipOffstage); | ||
| } |
There was a problem hiding this comment.
The findByTooltip helper function is duplicated across multiple example test files (e.g., bottom_app_bar.2_test.dart and tooltip.3_test.dart). To improve maintainability and adhere to DRY (Don't Repeat Yourself) principles, consider extracting this helper into a shared finders.dart file under packages/material_ui/example/test/ and importing it in the respective test files.
There was a problem hiding this comment.
I'm ok either way here. I could make a utils file in examples/test, though it would be the first one. This is already going to be duplicated with material_ui/test/finders.dart, and we will clean them up after flutter_test migrates to material_ui.
| final String tooltipMessage = | ||
| widget.message ?? widget.richMessage!.toPlainText(); |
There was a problem hiding this comment.
Using the null assertion operator ! on widget.richMessage can lead to a runtime exception if both widget.message and widget.richMessage are null. Use optional chaining ?. and a fallback empty string ?? '' to ensure defensive programming and null safety.
| final String tooltipMessage = | |
| widget.message ?? widget.richMessage!.toPlainText(); | |
| final String tooltipMessage = | |
| widget.message ?? widget.richMessage?.toPlainText() ?? ''; |
There was a problem hiding this comment.
There's an assertion that enforces at least one of these is set.
| // 2. Tooltip.message and Tooltip.richMessage are empty, since in this | ||
| // case no RawTooltip is created. | ||
| if (widget is Tooltip) { | ||
| final String tooltipMessage = widget.message ?? widget.richMessage!.toPlainText(); |
There was a problem hiding this comment.
Using the null assertion operator ! on widget.richMessage can lead to a runtime exception if both widget.message and widget.richMessage are null. Use optional chaining ?. and a fallback empty string ?? '' to ensure defensive programming and null safety.
| final String tooltipMessage = widget.message ?? widget.richMessage!.toPlainText(); | |
| final String tooltipMessage = widget.message ?? widget.richMessage?.toPlainText() ?? ''; |
| /// | ||
| /// This was copied from flutter_test, which uses flutter/material.dart. | ||
| /// | ||
| // TODO(justinmc): Port flutter_test to material_ui, then delete this method and |
There was a problem hiding this comment.
I don't think we want flutter_test to depend on material_ui, I thought we were going to remove any material/cupertino references from flutter_test?
There was a problem hiding this comment.
Yeah, I thought we had already done this.. See https://github.com/flutter/packages/blob/main/packages/material_ui/test/finders.dart
I don't think flutter_test can depend on material_ui. We'd end up in another circular circus.
There was a problem hiding this comment.
You are right, I'll remove these TODOs. The linked issue explains the situation correctlly.
116dcb6 to
530e5bc
Compare
Oh wait! It won't! |
|
autosubmit label was removed for flutter/packages/12492, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
flutter/packages@18fe786...f9b3954 2026-09-03 ashutoshagarwal2014@gmail.com [image_picker] Fix android-16 not picking up file (flutter/packages#11320) 2026-09-02 jmccandless@google.com [material_ui] Migrate off of flutter_test's find.byTooltip and on to the local findByTooltip (flutter/packages#12492) 2026-09-02 32538273+ValentinVignal@users.noreply.github.com [material_ui] Remove no shuffle from text field tests (flutter/packages#12547) 2026-09-02 21270878+elliette@users.noreply.github.com Add Material style variant enum (#12221) (flutter/packages#12717) 2026-09-02 brian.egan@verygood.ventures [go_router_builder] Report duplicate route paths at build time (flutter/packages#12399) 2026-09-02 victor.orozco@cloudsufi.com [google_sign_in] PR 1/4 google_sign_in_ios SPM packaging (flutter/packages#12654) 2026-09-02 55357489+voledyaev@users.noreply.github.com [video_player_android] Report display size for anamorphic video (flutter/packages#12360) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
The find.byTooltip finder in flutter/flutter's flutter_test package is unreliable for material_ui due to using flutter/flutter's Tooltip class. This PR migrates all usages to the existing findByTooltip in material_ui, which does not have this problem.
See flutter/flutter#191063 (comment)
Fixes flutter/flutter#191063