Fix self-comparison and assert typos in TestSemantics by Sanaullah49 · Pull Request #185975 · flutter/flutter

Sanaullah49 · 2026-05-04T10:16:49Z

Summary

Three small bugs in packages/flutter/test/widgets/semantics_tester.dart where the code accidentally compared a value to itself or shifted part of a logical expression into an assert message.

What changed

controlsNodes self-comparison (line 511). The guard controlsNodes != controlsNodes is always false, so the setEquals(...) mismatch path was unreachable. Replaced with controlsNodes != null, matching the null-guard style used by neighbouring optional fields (e.g. headingLevel, tags, scrollPosition).
LocaleStringAttribute.locale self-comparison (line 718). (second[i] as LocaleStringAttribute).locale != (second[i] as LocaleStringAttribute).locale was always false. Changed the left-hand side to first[i] to actually compare the two attributes' locales, mirroring the existing second[i].range != first[i].range check immediately above.
Stray comma in _IncludesNodeWith assert (line 1183). The comma after inputType != null made minValue != null || maxValue != null the assert message argument (a bool rather than a String). Replaced the comma with || so minValue / maxValue count as valid constructor arguments, matching the matcher's other named parameters.

No tests pass controlsNodes or LocaleStringAttribute to these matchers today, so these fixes only restore intended behaviour without disturbing existing assertions.

Testing

../../bin/flutter analyze test/widgets/semantics_tester.dart
../../bin/flutter test test/widgets/semantics_tester_test.dart
../../bin/flutter test test/widgets/semantics_tester_generate_test_semantics_expression_for_current_semantics_tree_test.dart
../../bin/flutter test test/widgets/semantics_test.dart

Fixes flutter#185900 ## Summary Three small bugs in `packages/flutter/test/widgets/semantics_tester.dart` where the code accidentally compared a value to itself or shifted part of a logical expression into an assert message. ## What changed 1. **`controlsNodes` self-comparison** (line 511). The guard `controlsNodes != controlsNodes` is always false, so the `setEquals(...)` mismatch path was unreachable. Replaced with `controlsNodes != null`, matching the null-guard style used by neighbouring optional fields (e.g. `headingLevel`, `tags`, `scrollPosition`). 2. **`LocaleStringAttribute.locale` self-comparison** (line 718). `(second[i] as LocaleStringAttribute).locale != (second[i] as LocaleStringAttribute).locale` was always false. Changed the left-hand side to `first[i]` to actually compare the two attributes' locales, mirroring the existing `second[i].range != first[i].range` check immediately above. 3. **Stray comma in `_IncludesNodeWith` assert** (line 1183). The comma after `inputType != null` made `minValue != null || maxValue != null` the assert message argument (a `bool` rather than a `String`). Replaced the comma with `||` so `minValue` / `maxValue` count as valid constructor arguments, matching the matcher's other named parameters. No tests pass `controlsNodes` or `LocaleStringAttribute` to these matchers today, so these fixes only restore intended behaviour without disturbing existing assertions. ## Testing * `../../bin/flutter analyze test/widgets/semantics_tester.dart` * `../../bin/flutter test test/widgets/semantics_tester_test.dart` * `../../bin/flutter test test/widgets/semantics_tester_generate_test_semantics_expression_for_current_semantics_tree_test.dart` * `../../bin/flutter test test/widgets/semantics_test.dart`

flutter-dashboard · 2026-05-04T10:16:53Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

gemini-code-assist

Code Review

This pull request corrects logic errors in TestSemantics and SemanticsTester where variables were incorrectly compared to themselves. It also refactors an assertion in the _IncludesNodeWith constructor to consolidate multiple null checks. Feedback suggests adding a descriptive error message to this assertion to comply with the style guide and verifying that the parameters are correctly stored as fields.

gemini-code-assist · 2026-05-04T10:19:10Z

+             inputType != null ||
+             minValue != null ||
+             maxValue != null,


There are two points to consider regarding this change:

Missing Assert Message: The assertion is missing a descriptive error message. According to the Flutter style guide (line 37), error messages should be useful and provide context. Adding a message like 'At least one property must be provided.' would improve the developer experience when the assertion fails.

Potential Missing Fields: The parameters minValue and maxValue (and others like scrollExtentMin) are used in this assertion, but they do not appear to be stored as fields in the _IncludesNodeWith class (only attributedLabel and attributedValue are visible at lines 1187-1188). If these parameters are not stored in fields, the matcher will not be able to verify them during the matching process, rendering them ineffective. Please ensure they are properly stored and utilized in the matching logic.

Suggested change

inputType != null ||

minValue != null ||

maxValue != null,

inputType != null ||

minValue != null ||

maxValue != null, 'At least one property must be provided.',

References

Error messages should be useful: Every error message is an opportunity to make someone love our product. ^(link)

Looks like the bot is correct. There should have a message for the assertion failure. Developer should be notified what's wrong.

Add a descriptive error message to the `_IncludesNodeWith` constructor assert so it complies with the Flutter style guide's requirement that asserts include a useful message.

Istiak-Ahmed78 · 2026-05-04T15:48:15Z

    }

-    if (controlsNodes != controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) {
+    if (controlsNodes != null && !setEquals(controlsNodes, node.controlsNodes)) {


Hi @Sanaullah49 , I'm not from the flutter team. But I've talk about this change. I think, we should not do null check here. Let me help you understand why not.

TestSemantics(controlsNodes: null) SemanticsNode(controlsNodes: {'a'})

what do you expect? the controlsNodes are not same, so the test shouldnot pass, right?
when you run test for this case. according to your code, the left side will be false, consequently, the test will pass event the contrlNodes are not same.

I suggest you to fix it

Suggested change

if (controlsNodes != null && !setEquals(controlsNodes, node.controlsNodes)) {

if (controlsNodes != node.controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) {

The intention is to make this an optional matcher, if not provided, it won't check againt the field. I think this is fine as is since this matcher is already quite noisy and deprecated, we shouldn't make it more noisy

Istiak-Ahmed78 · 2026-05-04T15:58:06Z

I think this changes requires to add tests also. But at the same time a PR already tracking the attached issue #185921

Sanaullah49 · 2026-05-07T10:22:31Z

There's a competing PR #185921 fixing the same three bugs. Flagging the differences in approach for maintainer visibility:

1. controlsNodes fix (line 511)

This PR: controlsNodes != null — guards the setEquals check only when the test expectation actually specifies controlsNodes. Matches how all other nullable fields in TestSemantics work.
fix: resolve TestSemantics comparison and assert issues #185921: controlsNodes != node.controlsNodes — compares set references, not values. Works but the intent is less clear.

2. Assert fix in _IncludesNodeWith (line 1180)

This PR uses || to combine all fields into one condition with a proper string message: any single field satisfies the assert.
fix: resolve TestSemantics comparison and assert issues #185921 uses && between the two groups, which would require minValue != null || maxValue != null to be satisfied on every call — breaking valid usages like includesNodeWith(label: 'foo') that don't provide minValue/maxValue.

3. Tests

fix: resolve TestSemantics comparison and assert issues #185921 adds regression tests for the locale and controlsNodes fixes (which this PR lacks). Happy to add those here if this PR is preferred, or to close in favour of fix: resolve TestSemantics comparison and assert issues #185921 if maintainers prefer that approach — just want to surface the && concern first.

chunhtai · 2026-05-15T17:24:22Z

I think this PR lgtm, just need a test before this can merge.

Since there are three different pr, and this seems to be closest. Let's try to get this to merge

victorsanni

These will need tests to land.

victorsanni · 2026-05-20T22:16:36Z

    }

-    if (controlsNodes != controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) {
+    if (controlsNodes != null && !setEquals(controlsNodes, node.controlsNodes)) {


Should we also apply these changes in material/semantics_tester.dart?

chunhtai

LGTM

victorsanni

https://github.com/flutter/flutter/pull/185975/changes#r3277507799

victorsanni

Should we also apply these changes in material/semantics_tester.dart (or cupertino)?

justinmc

See Victor's comment https://github.com/flutter/flutter/pull/185975/changes#r3277507799, this file has been duplicated for now unfortunately, so we'll have to make these changes twice.

github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels May 4, 2026

gemini-code-assist Bot reviewed May 4, 2026

View reviewed changes

Address gemini review feedback

5a4cdc6

Add a descriptive error message to the `_IncludesNodeWith` constructor assert so it complies with the Flutter style guide's requirement that asserts include a useful message.

Istiak-Ahmed78 reviewed May 4, 2026

View reviewed changes

Piinks requested a review from chunhtai May 12, 2026 22:21

victorsanni reviewed May 20, 2026

View reviewed changes

Add semantics tester regression tests

1ddf620

Piinks requested a review from victorsanni May 26, 2026 22:55

chunhtai mentioned this pull request May 27, 2026

fix: resolve TestSemantics comparison and assert issues #185921

Closed

10 tasks

chunhtai approved these changes May 28, 2026

View reviewed changes

victorsanni reviewed May 28, 2026

View reviewed changes

Piinks requested a review from victorsanni June 2, 2026 22:24

victorsanni requested a review from justinmc June 2, 2026 22:34

victorsanni reviewed Jun 2, 2026

View reviewed changes

victorsanni added the CICD Run CI/CD label Jun 2, 2026

justinmc reviewed Jun 2, 2026

View reviewed changes

hannah-hyj added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Jun 10, 2026

Piinks requested a review from justinmc June 23, 2026 22:16

	if (controlsNodes != null && !setEquals(controlsNodes, node.controlsNodes)) {
	if (controlsNodes != node.controlsNodes && !setEquals(controlsNodes, node.controlsNodes)) {

Uh oh!

Conversation

Sanaullah49 commented May 4, 2026

Summary

What changed

Testing

Uh oh!

flutter-dashboard Bot commented May 4, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Istiak-Ahmed78 May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Istiak-Ahmed78 May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Istiak-Ahmed78 commented May 4, 2026

Uh oh!

Sanaullah49 commented May 7, 2026

Uh oh!

chunhtai commented May 15, 2026

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni May 20, 2026

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Istiak-Ahmed78 May 4, 2026 •

edited

Loading