Add regression test for Slider inside TableRow#180342
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request adds a regression test to prevent a crash when a Slider is used within a TableRow. The added test is clear and correctly reproduces the issue. I've only found a minor formatting and best practice issue that can be easily fixed.
| testWidgets( | ||
| 'Table widget - Slider inside TableRow does not crash', | ||
| (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: Scaffold( | ||
| body: Table( | ||
| children: <TableRow>[ | ||
| TableRow( | ||
| children: <Widget>[ | ||
| Slider( | ||
| value: 0.0, | ||
| onChanged: null, | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
| expect(tester.takeException(), isNull); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The formatting of this test block seems to be off. Running dart format should fix it.
Additionally, the widget tree can be made a const expression. This is a good practice for performance as it allows Flutter to skip unnecessary rebuilds for widgets that do not change.
testWidgets(
'Table widget - Slider inside TableRow does not crash',
(WidgetTester tester) async {
await tester.pumpWidget(
const MaterialApp(
home: Scaffold(
body: Table(
children: <TableRow>[
TableRow(
children: <Widget>[
Slider(
value: 0.0,
onChanged: null,
),
],
),
],
),
),
),
);
await tester.pumpAndSettle();
expect(tester.takeException(), isNull);
},
);References
- All Dart code is formatted using
dart format. This is enforced by CI. (link)
There was a problem hiding this comment.
Formatting fixed using dart format, and widget tree is now const. Thanks!
|
yes, the test currently reproduces the failure on master and would fail until the underlying issue is fixed. Happy to mark this as blocked or adjust the test if you’d prefer it to land alongside the fix instead. The analyzer failure appears unrelated to this change. |
| await tester.pumpAndSettle(); | ||
| expect(tester.takeException(), isNull); |
There was a problem hiding this comment.
I think these two lines are unnecessary.
|
Actually I'm going to close this PR before you spend any more time on it. My opinion is that we shouldn't land skipped tests like this because it adds complexity to the test file. Instead, leave a comment on the issue with your test, and when the issue gets fixed, it can be included in that PR. If anyone strongly agrees with me here let me know and I'll reconsider. Thanks @divyanshyagyik for the work anyway, and if you end up submitting a PR fixing the issue I'd be happy to review that. |
Description
Adds a regression test for a framework assertion that occurs when a Slider
is placed inside a TableRow.
Issue
#180337
Details
Placing a Slider (which uses OverlayPortal) inside a Table triggers a
child.owner == ownerassertion. This test reproduces the failure andguards against future regressions.