settings: Migrate to new RadioGroup API; write widget tests for two just-added settings #1602
Conversation
This tests the observable behavior more directly. RadioListTile.checked has been deprecated (zulip#1545), so we'd like this test to stop relying on that implementation detail.
"Open message feeds at" and "Mark messages as read on scroll". Related: zulip#1571 Related: zulip#1583
This migration is verified by the tests that we touched in the previous commits; they ensure that the buttons' checked state still updates visibly. Fixes: zulip#1545
chrisbobbe
added
the
integration review
There was a problem hiding this comment.
Thanks for taking care of this (including writing those tests)! All looks good; some nits and for-the-future musings below.
This also seems like definitely a good change in the upstream API, too — better structured to match how the UI really works (or at least should work, for radio buttons to be the right fit).
| for (final setting in VisitFirstUnreadSetting.values) { | ||
| final thisSettingTitle = settingTitle(setting); | ||
| checkRadioButtonAppearsChecked<VisitFirstUnreadSetting>(tester, | ||
| thisSettingTitle, thisSettingTitle == settingTitle(expectedSetting)); |
There was a problem hiding this comment.
This comparison can be on the values directly instead of the labels, right?
| for (final setting in VisitFirstUnreadSetting.values) { | |
| final thisSettingTitle = settingTitle(setting); | |
| checkRadioButtonAppearsChecked<VisitFirstUnreadSetting>(tester, | |
| thisSettingTitle, thisSettingTitle == settingTitle(expectedSetting)); | |
| for (final setting in VisitFirstUnreadSetting.values) { | |
| final thisSettingTitle = settingTitle(setting); | |
| checkRadioButtonAppearsChecked<VisitFirstUnreadSetting>(tester, | |
| thisSettingTitle, setting == expectedSetting); |
| testNavObserver = TestNavigatorObserver() | ||
| ..onPushed = ((route, _) => lastPushedRoute = route) | ||
| ..onPopped = ((route, _) => lastPoppedRoute = route); | ||
| lastPushedRoute = null; | ||
| lastPoppedRoute = null; |
There was a problem hiding this comment.
Not for this PR, but: given how much this pattern is recurring now, I'm thinking it'd be good to centralize the code for it.
Perhaps by giving TestNavigatorObserver (or a subclass) some more structure, to track this sort of state itself.
| await tester.pump(); | ||
| check(lastPushedRoute).isA<MaterialWidgetRoute>() | ||
| .page.isA<VisitFirstUnreadSettingPage>(); | ||
| await tester.pump((lastPushedRoute as TransitionRoute).transitionDuration); |
There was a problem hiding this comment.
(again not for this PR)
If the nav observer tracks a log of both pushes and pops in a single list (or I guess even just tracks the last single such action), then this could perhaps even look something like:
| await tester.pump((lastPushedRoute as TransitionRoute).transitionDuration); | |
| await tester.pump(navObserver.lastTransitionDuration); |
and exactly the same in the pop case (when the last thing that happened was a pop, the lastTransitionDuration getter would look at its reverseTransitionDuration).
|
|
||
| await tester.tap(findRadioListTileWithTitle<VisitFirstUnreadSetting>( | ||
| settingTitle(VisitFirstUnreadSetting.always))); | ||
| await tester.pumpAndSettle(); // TODO why doesn't just `pump` work? |
There was a problem hiding this comment.
Hmm, dunno. Does pump(Duration.zero) work?
Looking at the implementation of setThemeSetting, it'll wait for the (simulated) DB write to complete before it updates the within-Dart store. In real life that's some IO that will take a small but nonzero amount of time, so its completion callback won't happen in a microtask, which is what pump() does. So it fits that that also wouldn't suffice in the test.
| await tester.pumpAndSettle(); | ||
| checkPage(tester, expectedSetting: VisitFirstUnreadSetting.never); | ||
|
|
||
| await tester.tap(find.byType(BackButton)); |
There was a problem hiding this comment.
Fun alternative (see its docs) which I happened across recently while browsing other code:
| await tester.tap(find.byType(BackButton)); | |
| await tester.tap(find.backButton()); |
| }); | ||
| }); | ||
|
|
||
| group('MarkReadOnScrollSetting', () { |
There was a problem hiding this comment.
If we add one or two more of these, we'll likely want to try abstracting them out a bit more — the body of the test case is repetitive enough that it risks getting hard to read without glossing over.
But explicit is always a good way to write the first version, and I think it works fine for two of these.
Fixes #1545.
Fixes part of: