Skip to content

Conversation

@ValentinVignal
Copy link
Contributor

Contributes to #130459

It adds a test for

  • examples/api/lib/material/icon_button/icon_button.3.dart

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Jun 10, 2024
@goderbauer goderbauer requested review from TahaTesser and bleroux June 12, 2024 22:07
Comment on lines 16 to 20
for (int i = 0; i <= 6; i += 2) {
expect(tester.widget<IconButton>(unselectedIconButtons.at(i)).onPressed, isA<VoidCallback>());
expect(tester.widget<IconButton>(unselectedIconButtons.at(i + 1)).onPressed, isNull);
}
expect(tester.widgetList<IconButton>(unselectedIconButtons).map((IconButton iconButton) => iconButton.isSelected), everyElement(isFalse));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i <= 6; i += 2) {
expect(tester.widget<IconButton>(unselectedIconButtons.at(i)).onPressed, isA<VoidCallback>());
expect(tester.widget<IconButton>(unselectedIconButtons.at(i + 1)).onPressed, isNull);
}
expect(tester.widgetList<IconButton>(unselectedIconButtons).map((IconButton iconButton) => iconButton.isSelected), everyElement(isFalse));
for (int i = 0; i <= 6; i++) {
final IconButton button = tester.widget<IconButton>(unselectedIconButtons.at(i));
expect(button.onPressed, i.isEven ? isA<VoidCallback>() : isNull);
expect(button.isSelected, isFalse);
}

Do you think this alternative is more readable? I prefer it because it avoids repeating the long conversion to IconButton and the mix of for and map. Up to you to adopt it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it looks simpler, I modified it in refactor: Better readable code

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it looks simpler, I modified it in refactor: Better readable code

Great! Can you apply the same to the other similar sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry, I pushed it in format: Better formatting

}
expect(tester.widgetList<IconButton>(unselectedIconButtons).map((IconButton iconButton) => iconButton.isSelected), everyElement(isFalse));
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing this extra line.

@ValentinVignal ValentinVignal requested a review from bleroux June 13, 2024 08:22
Comment on lines 13 to 15
);
expect(find.widgetWithIcon(IconButton, Icons.settings_outlined), findsExactly(8));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
);
expect(find.widgetWithIcon(IconButton, Icons.settings_outlined), findsExactly(8));
);
expect(find.widgetWithIcon(IconButton, Icons.settings_outlined), findsExactly(8));

Copy link
Member

Choose a reason for hiding this comment

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

Add extra line between pumpWidget and expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM!

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 14, 2024

auto label is removed for flutter/flutter/149988, due to - The status or check suite Mac tool_integration_tests_1_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2024
@bleroux bleroux force-pushed the flutter/api/example/Add-test-for-icon-button-3 branch from 248c614 to 6e26421 Compare June 16, 2024 16:21
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 18, 2024
@auto-submit auto-submit bot merged commit f2c48af into flutter:master Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 20, 2024
Manual roll requested by [email protected]

flutter/flutter@ccf3abe...6c06abb

2024-06-18 [email protected] Add test for engine artifact framework permissions (flutter/flutter#148786)
2024-06-18 [email protected] Add test for icon_button.3.dart (flutter/flutter#149988)
2024-06-18 [email protected] Roll Flutter Engine from 78fdd06af541 to 74f42ca3544c (6 revisions) (flutter/flutter#150421)
2024-06-18 [email protected] Fix transparent `dividerColor` breaks `TabBar.tabAlignment` (flutter/flutter#150350)
2024-06-18 [email protected] Fix scrollable `TabBar` jittering (flutter/flutter#150041)
2024-06-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland 3: [CupertinoActionSheet] Match colors to native (#150386)" (flutter/flutter#150413)
2024-06-18 [email protected] Extend the Windows web_tool_tests_1_2 shard timeout to 45 minutes (flutter/flutter#150393)
2024-06-18 [email protected] Roll Flutter Engine from 1c4e5e230ecb to 78fdd06af541 (3 revisions) (flutter/flutter#150403)
2024-06-18 [email protected] Roll Flutter Engine from a4f266f7eb1a to 1c4e5e230ecb (8 revisions) (flutter/flutter#150399)
2024-06-18 [email protected] Rename doc file to use standard hyphens (flutter/flutter#150314)
2024-06-17 [email protected] Fix typo in `SliverLayoutDimensions.hashCode` where not all properties are used in the hash code. (flutter/flutter#150306)
2024-06-17 [email protected] Fix doc comment references to 'this' (flutter/flutter#150379)
2024-06-17 [email protected] Add 'fail-fast' argument to flutter test (flutter/flutter#149587)
2024-06-17 [email protected] Update matchesGoldenFile documentation reference to goldenFileComparator (flutter/flutter#150343)
2024-06-17 [email protected] Reland 3: [CupertinoActionSheet] Match colors to native (flutter/flutter#150386)
2024-06-17 [email protected] [a11y] Add semantics: button to bottom navigation bar items and dropdown menu items (flutter/flutter#149375)
2024-06-17 [email protected] Reland "sliverGridDelegate mainAxisExtent add assert (#148470)"  (flutter/flutter#149720)
2024-06-17 [email protected] `ScaffoldBackgroundColor` should default to `ColorScheme.surface` (flutter/flutter#149772)
2024-06-17 [email protected] Reland TreeSliver  (flutter/flutter#149839)
2024-06-17 [email protected] Reland: [CupertinoActionSheet] Add sliding tap gesture (flutter/flutter#150219)
2024-06-17 [email protected] Roll Flutter Engine from 5989f0215fed to a4f266f7eb1a (1 revision) (flutter/flutter#150377)

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],[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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants