Skip to content

[cupertino_ui] Re-enable action_sheet_test.dart #12055

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

Merged
auto-submit merged 5 commits into from
Jul 1, 2026
Merged

[cupertino_ui] Re-enable action_sheet_test.dart #12055

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe965a5
Re-enable action_sheet_test.dart.
justinmc Jun 29, 2026
2cca7c1
Test button actions
justinmc Jun 29, 2026
49a84da
SemanticsHandle and formatting
justinmc Jun 29, 2026
507538c
No ensureSemantics needed.
justinmc Jun 30, 2026
9969edc
Test full semantics tree
justinmc Jun 30, 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
114 changes: 42 additions & 72 deletions ...ily_disabled_tests/action_sheet_test.dart → .../cupertino_ui/test/action_sheet_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

@Skip(
'This file is skipped due to a cross-import that needs to be fixed. Tracked in https://github.com/flutter/flutter/issues/177028.',
)
// This file is run as part of a reduced test set in CI on Mac and Windows
// machines.
@Tags(<String>['reduced-test-set'])
Expand All @@ -19,8 +16,6 @@ import 'package:flutter/services.dart';

import 'package:flutter_test/flutter_test.dart';

import '../widgets/semantics_tester.dart';

void main() {
testWidgets('Overall appearance is correct for the light theme', (WidgetTester tester) async {
await tester.pumpWidget(
Expand Down Expand Up @@ -1727,8 +1722,6 @@ void main() {
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001

testWidgets('Action sheet semantics', (WidgetTester tester) async {
final semantics = SemanticsTester(tester);

@Renzo-Olivares Renzo-Olivares Jun 29, 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 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.

final SemanticsHandle handle = tester.ensureSemantics();

@justinmc justinmc Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 49a84da, but maybe @chunhtai can confirm if it's needed?

@chunhtai chunhtai Jun 29, 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.

semantics is enabled by default for testwidget now, so you don't need to initialize the handle in test.


await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
Expand All @@ -1746,74 +1739,51 @@ void main() {
await tester.tap(find.text('Go'));
await tester.pumpAndSettle();

final SemanticsNode sheet = tester.semantics.find(find.bySemanticsLabel('Alert'));
expect(sheet.role, SemanticsRole.dialog);
expect(
semantics,
hasSemantics(
TestSemantics.root(
children: <TestSemantics>[
TestSemantics(
children: <TestSemantics>[
TestSemantics(
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.scopesRoute, SemanticsFlag.namesRoute],
label: 'Alert',
role: SemanticsRole.dialog,
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.hasImplicitScrolling],
children: <TestSemantics>[
TestSemantics(label: 'The title'),
TestSemantics(label: 'The message'),
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.hasImplicitScrolling],

@Renzo-Olivares Renzo-Olivares Jun 29, 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.

I wonder if we should still capture SemanticsFlag.hasImplicitScrolling . And how much of the actual semantics tree shape we want to verify. This test was very strict with regards to the shape of the tree, do we still want to keep it strict?

@justinmc justinmc Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to defer to @chunhtai on this.

I originally thought the hasImplicitScrolling was irrelevant, but maybe you do want to verify that the semantics tree is aware of the scrolling... Maybe we should be verifying everything from the "label: 'Alert'" level and below?

@chunhtai chunhtai Jun 30, 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.

Maybe we should be verifying everything from the "label: 'Alert'" level and below?

yes I think so

@justinmc justinmc Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai I've now updated it to test the whole tree, similar to before except with isSemantics instead of TestSemantics. Is that what you had in mind? Some questions:

  • It seems like it's not possible to test the role inside of isSemantics. Is there a better way to do that or do we need to add it to isSemantics?
  • The error messages are not very good. If I forget a child in children, the error message is type 'Null' is not a subtype of type 'String' in type cast.

Should I instead do a flat list of expects like in the original commit fe965a5? Or is there a better way here?

@Renzo-Olivares Renzo-Olivares Jun 30, 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.

I have been testing role using:

tester.getSemantics(find.byType(CupertinoSlidingSegmentedControl<int>)).role,
      SemanticsRole.radioGroup,
    );

@chunhtai chunhtai Jul 1, 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.

It seems like it's not possible to test the role inside of isSemantics. Is there a better way to do that or do we need to add it to isSemantics?

right we should make sure it support role

The error messages are not very good. If I forget a child in children, the error message is type 'Null' is not a subtype of type 'String' in type cast.

we should improve the error message

@chunhtai chunhtai Jul 1, 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.

pr to add role support flutter/flutter#188825

@chunhtai chunhtai Jul 1, 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.

pr to improve child mismatch flutter/flutter#188827

children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.isFocusable,
],
actions: <SemanticsAction>[
SemanticsAction.tap,
SemanticsAction.focus,
],
label: 'One',
),
TestSemantics(
flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.isFocusable,
],
actions: <SemanticsAction>[
SemanticsAction.tap,
SemanticsAction.focus,
],
label: 'Two',
),
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isButton, SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
label: 'Cancel',
),
],
),
],
),
],
),
],
),
ignoreId: true,
ignoreRect: true,
ignoreTransform: true,
sheet,
isSemantics(
label: 'Alert',
namesRoute: true,
scopesRoute: true,
children: <Matcher>[
isSemantics(
hasImplicitScrolling: true,
children: <Matcher>[
isSemantics(label: 'The title'),
isSemantics(label: 'The message'),
],
),
isSemantics(
hasImplicitScrolling: true,
children: <Matcher>[
isSemantics(
label: 'One',
isButton: true,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
isSemantics(
label: 'Two',
isButton: true,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
],
),
isSemantics(
label: 'Cancel',
isButton: true,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
],
),
);

semantics.dispose();
});

testWidgets('Conflicting scrollbars are not applied by ScrollBehavior to CupertinoActionSheet', (
Expand Down
Loading