Skip to content

Fix self-comparison and assert typos in TestSemantics #185975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sanaullah49 wants to merge 3 commits into
base: master
Choose a base branch
from
+106 −4
Open

Fix self-comparison and assert typos in TestSemantics #185975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
beddaf5
Fix self-comparison and assert typos in TestSemantics
Sanaullah49 May 4, 2026
5a4cdc6
Address gemini review feedback
Sanaullah49 May 4, 2026
1ddf620
Add semantics tester regression tests
Sanaullah49 May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/flutter/test/widgets/semantics_tester.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ class TestSemantics {
);
}

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

@Istiak-Ahmed78 Istiak-Ahmed78 May 4, 2026 •
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.

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)) {

@chunhtai chunhtai May 12, 2026

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.

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

@victorsanni victorsanni May 20, 2026

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.

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

return fail(
'expected node id $id to controls nodes $controlsNodes but found controlling nodes ${node.controlsNodes}',
);
Expand Down Expand Up @@ -715,7 +715,7 @@ class SemanticsTester {
if (first[i] is LocaleStringAttribute &&
(second[i] is! LocaleStringAttribute ||
second[i].range != first[i].range ||
(second[i] as LocaleStringAttribute).locale !=
(first[i] as LocaleStringAttribute).locale !=
(second[i] as LocaleStringAttribute).locale)) {
return false;
}
Expand Down Expand Up @@ -1180,8 +1180,14 @@ class _IncludesNodeWith extends Matcher {
scrollExtentMin != null ||
maxValueLength != null ||
currentValueLength != null ||
inputType != null,
minValue != null || maxValue != null,
inputType != null ||
minValue != null ||
maxValue != null,
Comment on lines +1183 to +1185

@gemini-code-assist gemini-code-assist Bot May 4, 2026

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.

high

There are two points to consider regarding this change:

  1. 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.
  2. 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
  1. Error messages should be useful: Every error message is an opportunity to make someone love our product. (link)

@Istiak-Ahmed78 Istiak-Ahmed78 May 4, 2026

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.

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

'At least one matcher field must be non-null so the matcher checks for at least one '
'property of a semantics node. Pass any of `label`, `value`, `actions`, `flags`, '
'`flagsCollection`, `tags`, `increasedValue`, `decreasedValue`, `scrollPosition`, '
'`scrollExtentMax`, `scrollExtentMin`, `maxValueLength`, `currentValueLength`, '
'`inputType`, `minValue`, or `maxValue`.',
);
final AttributedString? attributedLabel;
final AttributedString? attributedValue;
Expand Down
96 changes: 96 additions & 0 deletions packages/flutter/test/widgets/semantics_tester_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'dart:ui';

import 'package:flutter/gestures.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';

Expand Down Expand Up @@ -149,4 +150,99 @@ void main() {
);
semantics.dispose();
});

testWidgets('Semantics tester compares controlsNodes', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

await tester.pumpWidget(
Semantics(container: true, controlsNodes: const <String>{'actual'}, child: Container()),
);

final expectedSemantics = TestSemantics.root(
children: <TestSemantics>[
TestSemantics.rootChild(
id: 1,
rect: TestSemantics.fullScreen,
controlsNodes: const <String>{'expected'},
),
],
);

expect(semantics, isNot(hasSemantics(expectedSemantics)));
semantics.dispose();
});

testWidgets('Semantics tester compares attributed string locales', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

await tester.pumpWidget(
Semantics(
container: true,
attributedLabel: AttributedString(
'label',
attributes: <StringAttribute>[
LocaleStringAttribute(
locale: const Locale('en'),
range: const TextRange(start: 0, end: 5),
),
],
),
textDirection: TextDirection.ltr,
child: Container(),
),
);

expect(
semantics,
includesNodeWith(
label: 'label',
attributedLabel: AttributedString(
'label',
attributes: <StringAttribute>[
LocaleStringAttribute(
locale: const Locale('en'),
range: const TextRange(start: 0, end: 5),
),
],
),
),
);
expect(
semantics,
isNot(
includesNodeWith(
label: 'label',
attributedLabel: AttributedString(
'label',
attributes: <StringAttribute>[
LocaleStringAttribute(
locale: const Locale('es'),
range: const TextRange(start: 0, end: 5),
),
],
),
),
),
);
semantics.dispose();
});

testWidgets('includesNodeWith accepts minValue and maxValue', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

await tester.pumpWidget(
Semantics(
container: true,
value: '5',
minValue: '0',
maxValue: '10',
textDirection: TextDirection.ltr,
child: Container(),
),
);

expect(semantics, includesNodeWith(minValue: '0'));
expect(semantics, includesNodeWith(maxValue: '10'));
semantics.dispose();
});
}
Loading