Skip to content

Fix self-comparison and assert typos in TestSemantics - #191938

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
Sanaullah49:fix/185900-test-semantics-typos
Sep 1, 2026
Merged

Fix self-comparison and assert typos in TestSemantics#191938
auto-submit[bot] merged 4 commits into
flutter:masterfrom
Sanaullah49:fix/185900-test-semantics-typos

Conversation

@Sanaullah49

@Sanaullah49 Sanaullah49 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

This replaces #185975, which the stale bot auto-closed on Jul 1. The requested tests had been pushed on May 21 (commit "Add semantics tester regression tests"), but the waiting for response label was never cleared, so the timer kept running. #185975 could not be reopened, hence this PR.

@chunhtai you'd reviewed #185975 and said it lgtm pending a test. That test is here — three regression tests, one per fix:

  • Semantics tester compares controlsNodes
  • Semantics tester compares attributed string locales
  • includesNodeWith accepts minValue and maxValue

The bugs

Rebased onto current master, where all three are still present:

  • semantics_tester.dart:511 — controlsNodes != controlsNodes compares the field to itself, so it is always false and the setEquals check is unreachable. The controlsNodes expectation is silently never enforced. Fixed to controlsNodes != null.
  • semantics_tester.dart:715 — the locale comparison reads (second[i] as LocaleStringAttribute).locale != (second[i] as LocaleStringAttribute).locale, comparing second against itself rather than against first. Always false, so mismatched locales compare equal. Fixed to read first[i] on the left.
  • semantics_tester.dart:1184 — the minValue != null || maxValue != null checks sit in assert()'s message slot rather than its condition, so they never gate anything and includesNodeWith(minValue: ...) alone trips the assert. Folded into the condition, with a descriptive message added.

Testing

flutter test packages/flutter/test/widgets/semantics_tester_test.dart — 7/7 passing locally against current master.

The competing PR #185921 was also closed without merging, so there is no longer any overlap. PTAL when you have a moment.

@Sanaullah49
Fix self-comparison and assert typos in TestSemantics
3ad24e3 
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`
@Sanaullah49
Address gemini review feedback
a4a62d3 
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.
@Sanaullah49
Add semantics tester regression tests
6401245
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Aug 28, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Aug 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes comparison bugs in TestSemantics and SemanticsTester related to controlsNodes and LocaleStringAttribute locales, updates the _IncludesNodeWith matcher to support minValue and maxValue assertions with a descriptive error message, and adds corresponding tests. Feedback suggests expanding the assertion message and condition in _IncludesNodeWith to include attributedLabel and attributedValue.

Comment thread packages/flutter/test/widgets/semantics_tester.dart
@Sanaullah49
Include attributed fields and hint in _IncludesNodeWith assert
cfe7027 
The assert listed neither `attributedLabel`, `attributedValue`,
`attributedHint` nor `hint`, even though all four are constructor
parameters that the matcher compares. Passing any of them on its own
tripped the assert.

Adds them to the condition and the message, plus a regression test.
@Sanaullah49

Sanaullah49 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Good catch. It turned out to be four fields rather than two. attributedLabel, attributedValue, attributedHint and hint are all constructor parameters that the matcher compares, so passing any one of them on its own tripped the assert.

Added all four to the condition and the message, with a regression test covering each.

flutter test packages/flutter/test/widgets/semantics_tester_test.dart passes 8/8 against current master.

@chunhtai
chunhtai self-requested a review August 28, 2026 18:07
chunhtai
chunhtai approved these changes Aug 31, 2026
View reviewed changes

@chunhtai chunhtai left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once the gemini review is addressed

@chunhtai
chunhtai requested a review from hannah-hyj August 31, 2026 20:50
@chunhtai chunhtai added the CICD Run CI/CD label Aug 31, 2026
hannah-hyj
hannah-hyj approved these changes Aug 31, 2026
View reviewed changes

@hannah-hyj hannah-hyj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Sep 1, 2026
Merged via the queue into flutter:master with commit 616b5a5 Sep 1, 2026
29 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2026
Sanaullah49 added a commit to Sanaullah49/flutter that referenced this pull request Sep 2, 2026
@Sanaullah49
Apply TestSemantics fixes to the material copy of semantics_tester
6736a7a 
packages/flutter/test/material/semantics_tester.dart is a copy of
packages/flutter/test/widgets/semantics_tester.dart, kept so material
tests do not cross-import from widgets. It did not receive the fixes
from flutter#191938, so the two copies had drifted:

- `controlsNodes != controlsNodes` compares the field to itself, so the
  setEquals check is unreachable and the expectation never runs
- the locale comparison reads `second[i]` on both sides instead of
  `first[i]`, so mismatched locales compare equal
- the `minValue`/`maxValue` checks sit in assert()'s message slot rather
  than its condition, so they gate nothing
- `attributedLabel`, `attributedValue`, `attributedHint` and `hint` were
  missing from that assert's condition, so passing any one alone tripped it

The two files are now byte-identical again. Adds a test file covering
each fix, mirroring the widgets one.
This was referenced Sep 2, 2026
Closed
Open
This was referenced Sep 2, 2026
Closed
Merged
Closed
Closed
Closed
Closed
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Sep 4, 2026
@engine-flutter-autoroll
Roll Flutter from 0cbd1a4be0a4 to 70797e15325e (27 revisions) (#12727)
1240bb5 
flutter/flutter@0cbd1a4...70797e1

2026-09-02 [email protected] [analysis] Upgrade package:analyzer to 14.3.0 and enable custom plugin compilation (flutter/flutter#191590)
2026-09-02 [email protected] Roll Dart SDK from d8d7869d8031 to 9a8fbde80f6c (1 revision) (flutter/flutter#192179)
2026-09-02 [email protected] Roll Skia from dec311244db4 to ae22da2e8308 (1 revision) (flutter/flutter#192177)
2026-09-02 [email protected] [tool] Migrate DoctorCommand to modular dependency injection (flutter/flutter#190758)
2026-09-02 [email protected] Roll Skia from 482b7e285244 to dec311244db4 (3 revisions) (flutter/flutter#192161)
2026-09-02 [email protected] Roll Skia from 28447e9fb8d5 to 482b7e285244 (3 revisions) (flutter/flutter#192157)
2026-09-02 [email protected] feat: Add placeholder to DecorationImage (flutter/flutter#191528)
2026-09-02 [email protected] [Android] Do not schedule engine frames on a detached FlutterJNI (flutter/flutter#191204)
2026-09-02 [email protected] Roll Skia from 3a242fb7cdb3 to 28447e9fb8d5 (1 revision) (flutter/flutter#192145)
2026-09-02 [email protected] Roll Dart SDK from 9164def35347 to d8d7869d8031 (2 revisions) (flutter/flutter#192144)
2026-09-02 [email protected] Add FadeInImageTransition.fadeInOver to fade the image in over the placeholder (flutter/flutter#186246)
2026-09-02 [email protected] Roll Skia from 63d13ec4df6d to 3a242fb7cdb3 (9 revisions) (flutter/flutter#192138)
2026-09-02 [email protected] [flutter_tools] Add --no-plugins flag and disable plugins for benchmark (flutter/flutter#192129)
2026-09-02 [email protected] [tool] Migrate ChannelCommand to modular dependency injection (flutter/flutter#190751)
2026-09-01 [email protected] Add mouseCursor to RawScrollbar (flutter/flutter#185750)
2026-09-01 [email protected] Fix self-comparison and assert typos in TestSemantics (flutter/flutter#191938)
2026-09-01 [email protected] [Linux] Wait for frames to be rendered before using them in another context (flutter/flutter#192094)
2026-09-01 [email protected] [wimp] Turn on multithreading for wimp. (flutter/flutter#191747)
2026-09-01 [email protected] Roll Skia from 3911a1fe7f7a to 63d13ec4df6d (6 revisions) (flutter/flutter#192125)
2026-09-01 [email protected] Remove the redundant flutter/shell/platform/android:robolectric_tests build target (flutter/flutter#192117)
2026-09-01 [email protected] [iOS] Preserve semantics parents after reparenting (flutter/flutter#189686)
2026-09-01 [email protected] [flutter_tools] Report preview reload timing analytics in LspPreviewDetector (flutter/flutter#192120)
2026-09-01 [email protected] [flutter_tools] Implement Templates slice and flutter create integration (flutter/flutter#191748)
2026-09-01 [email protected] Uiscene migrate add2app hosts (flutter/flutter#191847)
2026-09-01 [email protected] Roll Packages from d642322 to 7a7912f (8 revisions) (flutter/flutter#192119)
2026-09-01 [email protected] [web] Anchor flt-semantics-host at 0,0 to fix WebKit semantics offset (flutter/flutter#190486)
2026-09-01 [email protected] Replace Shell::WaitForFirstFrame with an asynchronous API that matches the Shell threading model (flutter/flutter#191841)

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
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
@engine-flutter-autoroll @victorsanni
Roll Flutter from 0cbd1a4be0a4 to 70797e15325e (27 revisions) (flutte…
2822f8e 
…r#12727)

flutter/flutter@0cbd1a4...70797e1

2026-09-02 [email protected] [analysis] Upgrade package:analyzer to 14.3.0 and enable custom plugin compilation (flutter/flutter#191590)
2026-09-02 [email protected] Roll Dart SDK from d8d7869d8031 to 9a8fbde80f6c (1 revision) (flutter/flutter#192179)
2026-09-02 [email protected] Roll Skia from dec311244db4 to ae22da2e8308 (1 revision) (flutter/flutter#192177)
2026-09-02 [email protected] [tool] Migrate DoctorCommand to modular dependency injection (flutter/flutter#190758)
2026-09-02 [email protected] Roll Skia from 482b7e285244 to dec311244db4 (3 revisions) (flutter/flutter#192161)
2026-09-02 [email protected] Roll Skia from 28447e9fb8d5 to 482b7e285244 (3 revisions) (flutter/flutter#192157)
2026-09-02 [email protected] feat: Add placeholder to DecorationImage (flutter/flutter#191528)
2026-09-02 [email protected] [Android] Do not schedule engine frames on a detached FlutterJNI (flutter/flutter#191204)
2026-09-02 [email protected] Roll Skia from 3a242fb7cdb3 to 28447e9fb8d5 (1 revision) (flutter/flutter#192145)
2026-09-02 [email protected] Roll Dart SDK from 9164def35347 to d8d7869d8031 (2 revisions) (flutter/flutter#192144)
2026-09-02 [email protected] Add FadeInImageTransition.fadeInOver to fade the image in over the placeholder (flutter/flutter#186246)
2026-09-02 [email protected] Roll Skia from 63d13ec4df6d to 3a242fb7cdb3 (9 revisions) (flutter/flutter#192138)
2026-09-02 [email protected] [flutter_tools] Add --no-plugins flag and disable plugins for benchmark (flutter/flutter#192129)
2026-09-02 [email protected] [tool] Migrate ChannelCommand to modular dependency injection (flutter/flutter#190751)
2026-09-01 [email protected] Add mouseCursor to RawScrollbar (flutter/flutter#185750)
2026-09-01 [email protected] Fix self-comparison and assert typos in TestSemantics (flutter/flutter#191938)
2026-09-01 [email protected] [Linux] Wait for frames to be rendered before using them in another context (flutter/flutter#192094)
2026-09-01 [email protected] [wimp] Turn on multithreading for wimp. (flutter/flutter#191747)
2026-09-01 [email protected] Roll Skia from 3911a1fe7f7a to 63d13ec4df6d (6 revisions) (flutter/flutter#192125)
2026-09-01 [email protected] Remove the redundant flutter/shell/platform/android:robolectric_tests build target (flutter/flutter#192117)
2026-09-01 [email protected] [iOS] Preserve semantics parents after reparenting (flutter/flutter#189686)
2026-09-01 [email protected] [flutter_tools] Report preview reload timing analytics in LspPreviewDetector (flutter/flutter#192120)
2026-09-01 [email protected] [flutter_tools] Implement Templates slice and flutter create integration (flutter/flutter#191748)
2026-09-01 [email protected] Uiscene migrate add2app hosts (flutter/flutter#191847)
2026-09-01 [email protected] Roll Packages from d642322 to 7a7912f (8 revisions) (flutter/flutter#192119)
2026-09-01 [email protected] [web] Anchor flt-semantics-host at 0,0 to fix WebKit semantics offset (flutter/flutter#190486)
2026-09-01 [email protected] Replace Shell::WaitForFirstFrame with an asynchronous API that matches the Shell threading model (flutter/flutter#191841)

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
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
@engine-flutter-autoroll @victorsanni
Roll Flutter from 0cbd1a4be0a4 to 70797e15325e (27 revisions) (flutte…
2ee1685 
…r#12727)

flutter/flutter@0cbd1a4...70797e1

2026-09-02 [email protected] [analysis] Upgrade package:analyzer to 14.3.0 and enable custom plugin compilation (flutter/flutter#191590)
2026-09-02 [email protected] Roll Dart SDK from d8d7869d8031 to 9a8fbde80f6c (1 revision) (flutter/flutter#192179)
2026-09-02 [email protected] Roll Skia from dec311244db4 to ae22da2e8308 (1 revision) (flutter/flutter#192177)
2026-09-02 [email protected] [tool] Migrate DoctorCommand to modular dependency injection (flutter/flutter#190758)
2026-09-02 [email protected] Roll Skia from 482b7e285244 to dec311244db4 (3 revisions) (flutter/flutter#192161)
2026-09-02 [email protected] Roll Skia from 28447e9fb8d5 to 482b7e285244 (3 revisions) (flutter/flutter#192157)
2026-09-02 [email protected] feat: Add placeholder to DecorationImage (flutter/flutter#191528)
2026-09-02 [email protected] [Android] Do not schedule engine frames on a detached FlutterJNI (flutter/flutter#191204)
2026-09-02 [email protected] Roll Skia from 3a242fb7cdb3 to 28447e9fb8d5 (1 revision) (flutter/flutter#192145)
2026-09-02 [email protected] Roll Dart SDK from 9164def35347 to d8d7869d8031 (2 revisions) (flutter/flutter#192144)
2026-09-02 [email protected] Add FadeInImageTransition.fadeInOver to fade the image in over the placeholder (flutter/flutter#186246)
2026-09-02 [email protected] Roll Skia from 63d13ec4df6d to 3a242fb7cdb3 (9 revisions) (flutter/flutter#192138)
2026-09-02 [email protected] [flutter_tools] Add --no-plugins flag and disable plugins for benchmark (flutter/flutter#192129)
2026-09-02 [email protected] [tool] Migrate ChannelCommand to modular dependency injection (flutter/flutter#190751)
2026-09-01 [email protected] Add mouseCursor to RawScrollbar (flutter/flutter#185750)
2026-09-01 [email protected] Fix self-comparison and assert typos in TestSemantics (flutter/flutter#191938)
2026-09-01 [email protected] [Linux] Wait for frames to be rendered before using them in another context (flutter/flutter#192094)
2026-09-01 [email protected] [wimp] Turn on multithreading for wimp. (flutter/flutter#191747)
2026-09-01 [email protected] Roll Skia from 3911a1fe7f7a to 63d13ec4df6d (6 revisions) (flutter/flutter#192125)
2026-09-01 [email protected] Remove the redundant flutter/shell/platform/android:robolectric_tests build target (flutter/flutter#192117)
2026-09-01 [email protected] [iOS] Preserve semantics parents after reparenting (flutter/flutter#189686)
2026-09-01 [email protected] [flutter_tools] Report preview reload timing analytics in LspPreviewDetector (flutter/flutter#192120)
2026-09-01 [email protected] [flutter_tools] Implement Templates slice and flutter create integration (flutter/flutter#191748)
2026-09-01 [email protected] Uiscene migrate add2app hosts (flutter/flutter#191847)
2026-09-01 [email protected] Roll Packages from d642322 to 7a7912f (8 revisions) (flutter/flutter#192119)
2026-09-01 [email protected] [web] Anchor flt-semantics-host at 0,0 to fix WebKit semantics offset (flutter/flutter#190486)
2026-09-01 [email protected] Replace Shell::WaitForFirstFrame with an asynchronous API that matches the Shell threading model (flutter/flutter#191841)

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
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chunhtai chunhtai chunhtai approved these changes
@hannah-hyj hannah-hyj hannah-hyj approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Sanaullah49 @chunhtai @hannah-hyj @justinmc