Skip to content

[cupertino_ui] Re-enable dialog_test.dart - #12057

Merged
auto-submit[bot] merged 7 commits into
flutter:mainfrom
justinmc:cross-imports-dialog
Jul 1, 2026
Merged

[cupertino_ui] Re-enable dialog_test.dart#12057
auto-submit[bot] merged 7 commits into
flutter:mainfrom
justinmc:cross-imports-dialog

Conversation

@justinmc

Copy link
Copy Markdown
Contributor

This file used TestSemantics from the Widgets library in flutter/flutter. That approach to semantics testing is not advised, as explained in flutter/flutter#184367. I migrated to the recommended tester.semantics.find.

I also added unawaited around some futures that needed it because that lint doesn't exist in flutter/flutter.

TestSemantics to tester.semantics.find.

@gemini-code-assistgemini-code-assistBot 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 removes the skip annotation from dialog_test.dart, wraps asynchronous showCupertinoDialog calls with unawaited, and migrates semantics tests from SemanticsTester to tester.semantics.find. Feedback on the changes points out that the migration omitted assertions for the hasImplicitScrolling flag, which reduces test coverage, and recommends restoring these assertions while also reusing the defined dialogFinder.

Comment on lines +699 to +718
final Finder dialogFinder = find.bySemanticsLabel('Alert');
final SemanticsNode dialog = tester.semantics.find(find.bySemanticsLabel('Alert'));
expect(dialog.role, SemanticsRole.alertDialog);
expect(dialog, isSemantics(namesRoute: true, scopesRoute: true));
expect(
semantics,
hasSemantics(
TestSemantics.root(
children: <TestSemantics>[
TestSemantics(
children: <TestSemantics>[
TestSemantics(
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.scopesRoute],
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[
SemanticsFlag.scopesRoute,
SemanticsFlag.namesRoute,
],
role: SemanticsRole.alertDialog,
label: 'Alert',
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.hasImplicitScrolling],
children: <TestSemantics>[
TestSemantics(label: 'The Title'),
TestSemantics(label: 'Content'),
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.hasImplicitScrolling],
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isButton],
label: 'Cancel',
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isButton],
label: 'OK',
),
],
),
],
),
],
),
],
),
],
),
],
),
ignoreId: true,
ignoreRect: true,
ignoreTransform: true,
),
find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('The Title')),
findsOneWidget,
);

semantics.dispose();
expect(
find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('Content')),
findsOneWidget,
);
final SemanticsNode buttonOK = tester.semantics.find(
find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('OK')),
);
expect(buttonOK, isSemantics(isButton: true));
final SemanticsNode buttonCancel = tester.semantics.find(
find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('Cancel')),
);
expect(buttonCancel, isSemantics(isButton: true));

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.

medium

During the migration from SemanticsTester to tester.semantics.find, the assertions verifying the hasImplicitScrolling flag on the scrollable containers (the title/content section and the actions section) were lost. This reduces the test coverage for critical accessibility behaviors of the dialog.\n\nAdditionally, dialogFinder is defined but not reused when retrieving the dialog semantics node.\n\nThis suggestion reuses dialogFinder, extracts other finders for readability, and restores the assertions for hasImplicitScrolling by verifying the parent nodes of the title and buttons.

 final Finder dialogFinder = find.bySemanticsLabel('Alert');\n final SemanticsNode dialog = tester.semantics.find(dialogFinder);\n expect(dialog.role, SemanticsRole.alertDialog);\n expect(dialog, isSemantics(namesRoute: true, scopesRoute: true));\n\n final Finder titleFinder = find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('The Title'));\n expect(titleFinder, findsOneWidget);\n final SemanticsNode titleNode = tester.semantics.find(titleFinder);\n expect(titleNode.parent, isSemantics(hasImplicitScrolling: true));\n\n expect(\n find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('Content')),\n findsOneWidget,\n );\n\n final Finder okFinder = find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('OK'));\n final SemanticsNode buttonOK = tester.semantics.find(okFinder);\n expect(buttonOK, isSemantics(isButton: true));\n expect(buttonOK.parent, isSemantics(hasImplicitScrolling: true));\n\n final Finder cancelFinder = find.descendant(of: dialogFinder, matching: find.bySemanticsLabel('Cancel'));\n final SemanticsNode buttonCancel = tester.semantics.find(cancelFinder);\n expect(buttonCancel, isSemantics(isButton: true));

});

testWidgets('Has semantic annotations', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

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.

nit: Should we use SemanticsHandle to ensure semantics is initialized like below (plus dispose at the end of the test)? I'm not sure how necessary this is but I've been doing it in the migrations I've done.

finalSemanticsHandle handle = tester.ensureSemantics();

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done in ab8a7ce.

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.

semantics is enabled by default, I think this is not needed

});

testWidgets('showCupertinoDialog - custom barrierLabel', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

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.

semantics,
isNot(
includesNodeWith(label: 'Custom label', flags: <SemanticsFlag>[SemanticsFlag.namesRoute]),
find.semantics.byPredicate(

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.

I think this can just be

expect(find.semantics.byLabel('Custom label'), findsNothing);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I guess the flags part is not important here. That sure simplifies things.

@justinmcjustinmc added the CICD Run CI/CD label Jun 30, 2026

@QuncCcccccQuncCccccc 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.

LGTM!

),
);

final Finder dialogFinder = find.bySemanticsLabel('Alert');

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.

this can use the isSemantics(children:) to assert the subtree

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done in 4ee13ef

I was hoping to use your new isSemantics role parameter too, but I guess we'll have to wait until we bump the SDK.

@github-actionsgithub-actionsBot removed the CICD Run CI/CD label Jul 1, 2026
@justinmcjustinmc added the CICD Run CI/CD label Jul 1, 2026

@Renzo-OlivaresRenzo-Olivares 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.

LGTM

@justinmcjustinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 1, 2026
@auto-submit
auto-submitBot merged commit 48e670c into flutter:mainJul 1, 2026
13 checks passed
@justinmc
justinmc deleted the cross-imports-dialog branch July 1, 2026 17:46
pullBot pushed a commit to Klomgor/flutter that referenced this pull request Jul 2, 2026
…er#188916)
flutter/packages@e742106...420e135
2026-07-02 dkwingsmt@users.noreply.github.com [material_ui] Migrate api
sample code to @example dartdoc directive (flutter/packages#12078)
2026-07-02 dkwingsmt@users.noreply.github.com [material_ui] Move over
more API samples (flutter/packages#12092)
2026-07-01 katelovett@google.com [cupertino_ui] Migrate api sample code
to @example dartdoc directive (flutter/packages#12063)
2026-07-01 katelovett@google.com [cupertino_ui] Move over more API
samples (flutter/packages#12086)
2026-07-01 48776784+mackings@users.noreply.github.com
[two_dimensional_scrollables] Fix TreeView horizontal hit testing
(flutter/packages#11859)
2026-07-01 1063596+reidbaker@users.noreply.github.com
[camera_android_camerax][tool] Integrate dart_code_linter for cyclomatic
complexity checks (flutter/packages#11999)
2026-07-01 dinurymomshad.dev@gmail.com [cross_file] Add runnable example
with main() (flutter/packages#11527)
2026-07-01 36861262+QuncCccccc@users.noreply.github.com [cupertino_ui]
Enable `switch_test.dart` (flutter/packages#12076)
2026-07-01 jmccandless@google.com [cupertino_ui] Re-enable
dialog_test.dart (flutter/packages#12057)
2026-07-01 jmccandless@google.com [cupertino_ui] Re-enable
action_sheet_test.dart (flutter/packages#12055)
2026-07-01 rmolivares@renzo-olivares.dev [cupertino_ui] Migrate
`bottom_tab_bar_test.dart` to `SemanticsHandle` (flutter/packages#12012)
2026-07-01 1063596+reidbaker@users.noreply.github.com [repo] Add comment
style guideline to AGENTS.md (flutter/packages#12077)
2026-07-01 43054281+camsim99@users.noreply.github.com Adds pre-push
readiness skill (flutter/packages#11935)
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-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.
To file a bug in Flutter:
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmitMerge PR when tree becomes green via auto submit AppCICDRun CI/CDp: cupertino_uitriage-frameworkShould be looked at in framework triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@justinmc@Renzo-Olivares@QuncCccccc@chunhtai