Remove semantics_tester cross-import from segmented_button_test.dart#185250
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces the semantics_tester.dart utility to the material package and updates segmented_button_test.dart to use it. The review identified several critical logical errors, including self-comparison typos in controlsNodes and LocaleStringAttribute checks, and an incorrectly structured assertion in the _IncludesNodeWith matcher. Additionally, feedback was provided to correct a documentation inconsistency regarding rootRect dimensions and a typo in a semantic error message.
| ); | ||
| } | ||
|
|
||
| if (controlsNodes != controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) { |
There was a problem hiding this comment.
The condition controlsNodes != controlsNodes is always false, which means the controlsNodes check is effectively skipped. This is likely a typo and should be changed to controlsNodes != null or simply removed if setEquals handles nulls as expected for your test logic.
| if (controlsNodes != controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) { | |
| if (controlsNodes != null && !setEquals(controlsNodes, node.controlsNodes)) { |
| (second[i] as LocaleStringAttribute).locale != | ||
| (second[i] as LocaleStringAttribute).locale)) { |
There was a problem hiding this comment.
The comparison (second[i] as LocaleStringAttribute).locale != (second[i] as LocaleStringAttribute).locale is always false because it compares the same object's property to itself. It should compare the locale of the first attribute with the second attribute.
| (second[i] as LocaleStringAttribute).locale != | |
| (second[i] as LocaleStringAttribute).locale)) { | |
| (first[i] as LocaleStringAttribute).locale != | |
| (second[i] as LocaleStringAttribute).locale)) { |
| final double? scrollPosition; | ||
| final double? scrollExtentMax; |
There was a problem hiding this comment.
The assert in the _IncludesNodeWith constructor incorrectly passes minValue != null || maxValue != null as the second argument to the assert function, which is treated as the error message. These conditions should be part of the first argument (the condition) to ensure that the matcher is not created without at least one property to check.
inputType != null ||
minValue != null ||
maxValue != null,| /// The [rect] field is required and has no default. Convenient values are | ||
| /// available: | ||
| /// | ||
| /// * [TestSemantics.rootRect]: 2400x1600, the test screen's size in physical |
There was a problem hiding this comment.
The documentation states that rootRect is 2400x1600, but the actual constant value defined on line 252 is 2400x1800. This should be updated for consistency.
| /// * [TestSemantics.rootRect]: 2400x1600, the test screen's size in physical | |
| /// * [TestSemantics.rootRect]: 2400x1800, the test screen's size in physical |
| } | ||
| if (scrollChildren != null && scrollChildren != nodeData.scrollChildCount) { | ||
| return fail( | ||
| 'expected node id $id to have scrollIndex $scrollChildren but found scrollIndex ${nodeData.scrollChildCount}.', |
There was a problem hiding this comment.
The error message incorrectly refers to scrollIndex instead of scrollChildren (or scrollChildCount).
| 'expected node id $id to have scrollIndex $scrollChildren but found scrollIndex ${nodeData.scrollChildCount}.', | |
| 'expected node id $id to have scrollChildren $scrollChildren but found scrollChildren ${nodeData.scrollChildCount}.', |
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
Recently there was a discussion about what to do with TestSemantics and the conclusion was that duplicating it is what we want, because we don't want to encourage its use in user tests by putting it in flutter_test publicly.
e17daca to
7a4e8c2
Compare
|
autosubmit label was removed for flutter/flutter/185250, because - The status or check suite Check Code Freeze has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
|
The code freeze override has been buggy recently. I pushed a merge commit to try to fix it. If not then a rebase should work (I should have done that but misclicked). |
Removes the cross-import of
semantics_tester.dartfrommaterial/segmented_button_test.dart.Duplicated
semantics_tester.dartlocally into the material test folder to remove the dependency on widgets test utilities. This is part of the effort to decouple Material and Cupertino from the widgets library.Fixes part of #182636