Mark as resolved/unresolved prep 2 #1306
Merged
Conversation
This file contains 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
chrisbobbe
added
the
integration review
gnprice
reviewed
Jan 25, 2025
test/widgets/action_sheet_test.dart
Outdated
Comment on lines
150
to
153
| final channelIdsWithUnreads = store.unreads.streams.keys; | ||
| final topicWithUnreads = channelIdsWithUnreads.firstWhereOrNull((streamId) => | ||
| store.unreads.countInTopicNarrow(streamId, TopicName(topic)) > 0); | ||
| if (topicWithUnreads == null) { |
There was a problem hiding this comment.
The name topicWithUnreads doesn't seem to match what this does — it ends up taking the value of a channel ID.
Perhaps use any instead of firstWhereOrNull?
There was a problem hiding this comment.
Ah indeed, and hasTopicWithUnreads for the variable name.
test/widgets/action_sheet_test.dart
Outdated
| StreamMessage? message, | ||
| }) async { | ||
| final effectiveMessage = message ?? someMessage; | ||
| assert(effectiveMessage.topic.apiName == topic); |
There was a problem hiding this comment.
Can this function skip taking topic as a parameter, and just get it from the message?
test/widgets/action_sheet_test.dart
Outdated
| Future<void> showFromAppBar(WidgetTester tester, { | ||
| ZulipStream? channel, | ||
| String topic = someTopic, | ||
| Iterable<StreamMessage>? messages, |
There was a problem hiding this comment.
Suggested change
| Iterable<StreamMessage>? messages, | |
| List<StreamMessage>? messages, |
Then we can skip the toList below. The callers are all passing lists anyway.
…pare` To test showing the topic action sheet from the message list (app bar or recipient header), we don't need `store.addMessage`; in those cases, the message data comes via MessageListView's message fetch. To test showing the action sheet from the Inbox, we do need a message ID to be in the Unreads data. So, arrange that, but only in the tests (one test) that use the Inbox. While we're at it, also make this setup represent the common case where we don't have the full Message object because a message list containing the message hasn't been opened yet. When we write tests for the upcoming resolve/unresolve button in the topic action sheet, that's a case we'll want to exercise, in addition to the case where we do have the message details.
This `prepare` function would also be useful for some tests outside the 'showTopicActionSheet' group. Give it these params to start getting it ready to move to the outer scope. We'll add more params in the next few commits.
For the sake of some new callers when we move this function definition to an outer scope.
For the sake of some new callers when we move this function definition to an outer scope.
For the sake of some new callers when we move this function definition to an outer scope.
…nSheet Just to make this logic more similar to a `prepare` function in a different group, to prepare (heh) for moving that `prepare` function out and reusing it here.
Any of this function's new params (isChannelSubscribed, isChannelMuted, visibilityPolicy, and zulipFeatureLevel) might come in useful as we continue to add to the topic action sheet.
Which is given the value `eg.stream()`, too, anyway.
…narrow With zulip#1039 "Fuse recipient header into app bar for topic narrows", this setup will likely become invalid. It assumes that a topic recipient header can be found in a topic-narrow message list. Switching to the combined-feed narrow would break tests that use a muted topic, because the topic's message(s) are excluded in that narrow, so there wouldn't be a recipient header to tap. Instead, trigger the topic action sheet from the topic-narrow app bar.
These are natural places for someone to look for such tests.
Upcoming tests for the resolve/unresolve button can use this to test an edge case where the button isn't offered. While we're at it, replace finder logic in some existing checks, making some a bit more targeted (so a not-NFC change): when finding `UserTopicUpdateButton`s by their label text, we now require the text to be under a BottomSheet widget.
…ssage` When we add the resolve/unresolve button, coming up, we'll use this to test the edge case where the button isn't shown in the message-list app bar if the message list is empty. (For that case we'll just pass an empty Iterable for this.)
Ah indeed! |
chrisbobbe
force-pushed
the
pr-action-sheet-test-refactors
branch
from
e172c58 to
55fdfd3
Compare
|
Thanks for the review, revision pushed! |
|
Thanks! Looks good; merging. |
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.
Here are the commits from #1301 that are just about refactoring the action-sheet tests. I've revised them for Zixuan's review #1301 (review) ; in particular by just removing an
assert(() {}())wrapper (which isn't needed because it's only running in tests).