Move markNarrowAsRead to new "actions" file; port most of its tests #827
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnprice
commented
Jul 19, 2024
Comment on lines
+42
to
+47
| await tester.pumpWidget(Builder(builder: (context) => | ||
| MaterialApp( | ||
| theme: zulipThemeData(context), | ||
| localizationsDelegates: ZulipLocalizations.localizationsDelegates, | ||
| supportedLocales: ZulipLocalizations.supportedLocales, | ||
| home: GlobalStoreWidget( |
There was a problem hiding this comment.
This makes another spot that could benefit from TestZulipApp, introduced in #805. So whichever of these PRs lands first, the other can update this spot after rebasing.
chrisbobbe
reviewed
Jul 19, 2024
| check(isMarkAsReadButtonVisible(tester)).isTrue(); | ||
|
|
||
| // Might as well test with oldUnreadsMissing: true. | ||
| store.unreads.oldUnreadsMissing = true; |
There was a problem hiding this comment.
The analyzer is flagging an unused import '../model/unreads_checks.dart'.
This function isn't quite part of the UI -- it's mostly a driver around some API requests. But it doesn't belong in the API code either, because it interacts with UI widgets in order to give progress and failure feedback to the user. Because there's nontrivial logic in here and it isn't really specific to this particular piece of UI, we'd like to reuse much of this code, in particular for zulip#131 marking as unread (as discussed at zulip#779). This new file provides an appropriate home for that reusable code. Before we can actually reuse this for marking as unread, it'll need some refactoring. Before we undertake refactoring it, it should have tests that operate at the layer of this function itself, so that they can easily be adapted to cover the mark-unread case too. There are existing tests that cover this pretty thoroughly, but they act on the UI in the message list that drives it. So we'll port those tests over in the next couple of commits.
I think the old name here was used for this function in an early draft of the PR that added it. In any case it doesn't refer to anything now.
This sets us up to be able to refactor this function for other use cases that aren't exercised by MarkAsReadWidget, and have good tests for those cases too. There are a couple of pieces of functionality of the widget's _handlePress method that probably should be within the function markNarrowAsRead instead, so that they can be reused together. For the tests for those, we port them too but mark them as skipped, with TODO comments. After refactoring to move that functionality inside, we can enable the tests. The new markNarrowAsRead unit tests do most of the work of tests for MarkAsReadWidget. We leave behind a couple of smoke tests that mostly just show it is indeed using markNarrowAsRead, plus the tests whose ported versions are skipped.
|
Ah indeed, thanks. Fixed and merged. |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refactoring we'll want for #131 / #779, as discussed at #779 (comment) .
This doesn't cover actually refactoring
markNarrowAsReadto be reusable for marking as unread — @Khader-1 that'll be for you to do as part of #779. But this sets up the framework for doing that refactor.The two main commits' messages:
actions [nfc]: Move markNarrowAsRead to new file
This function isn't quite part of the UI -- it's mostly a driver
around some API requests. But it doesn't belong in the API code
either, because it interacts with UI widgets in order to give
progress and failure feedback to the user.
Because there's nontrivial logic in here and it isn't really specific
to this particular piece of UI, we'd like to reuse much of this code,
in particular for #131 marking as unread (as discussed at #779).
This new file provides an appropriate home for that reusable code.
Before we can actually reuse this for marking as unread, it'll need
some refactoring. Before we undertake refactoring it, it should have
tests that operate at the layer of this function itself, so that they
can easily be adapted to cover the mark-unread case too. There are
existing tests that cover this pretty thoroughly, but they act on the
UI in the message list that drives it. So we'll port those tests over
in the next couple of commits.
actions test: Port most MarkAsReadWidget tests to markNarrowAsRead
This sets us up to be able to refactor this function for other
use cases that aren't exercised by MarkAsReadWidget, and have good
tests for those cases too.
There are a couple of pieces of functionality of the widget's
_handlePress method that probably should be within the function
markNarrowAsRead instead, so that they can be reused together.
For the tests for those, we port them too but mark them as
skipped, with TODO comments. After refactoring to move that
functionality inside, we can enable the tests.
The new markNarrowAsRead unit tests do most of the work of tests
for MarkAsReadWidget. We leave behind a couple of smoke tests
that mostly just show it is indeed using markNarrowAsRead, plus
the tests whose ported versions are skipped.