[material_ui] Migrate off of flutter_test's find.byTooltip and on to the local findByTooltip - #12492
[material_ui] Migrate off of flutter_test's find.byTooltip and on to the local findByTooltip#12492justinmc wants to merge 3 commits into
Conversation
github-actions
Bot
added
triage-framework
![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 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() ?? ''; |
1 participant
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