-
Notifications
You must be signed in to change notification settings - Fork 353
msglist test: Add test for messagesCloseInTime, and update a related test #1826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gnprice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Generally this revision looks good; just small comments.
Given your timezone you're probably asleep at this hour, and I'll be off on vacation before you start your next workday. So @chrisbobbe please pick up the reviews from here, and go ahead and merge this PR when you consider it ready.
| Message streamMessage(int id, int timestamp, User sender) => | ||
| eg.streamMessage(id: id, sender: sender, | ||
| Message streamMessage(int timestamp, User sender) => | ||
| eg.streamMessage(sender: sender, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
msglist [test]: Avoid specifying explicit message-IDs in `eg.{*}Message()`
"msglist test", rather than "msglist [test]"
| bool forcedShowSender = false; | ||
| final bool showSender; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these two commits should get squashed together:
50c8123 msglist test [nfc]: Refactor forcedShowSender evaluation in checkInvariants
b2e9f4e msglist test [nfc]: Rename forcedShowSender to showSender in checkInvariants
for the same reason that motivated the rename (#1815 (comment)): otherwise the intermediate state is confusing in that the name doesn't match the meaning
test/model/message_list_test.dart
Outdated
| doTest('2022-01-01 00:30:00', time, false); | ||
| doTest('2021-02-01 00:30:00', time, false); | ||
| doTest('2021-01-02 00:30:00', time, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice logical sequence. One more case will complete the sequence:
| doTest('2022-01-01 00:30:00', time, false); | |
| doTest('2021-02-01 00:30:00', time, false); | |
| doTest('2021-01-02 00:30:00', time, false); | |
| doTest('2022-01-01 00:30:00', time, false); | |
| doTest('2021-02-01 00:30:00', time, false); | |
| doTest('2021-01-02 00:30:00', time, false); | |
| doTest('2021-01-01 01:30:00', time, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also have this set of cases exercise the normal situation where the first time is earlier than the second one, instead of the unusual case where it's later. That way these will be exercising cases we expect to happen regularly, rather than cases that can only happen if something's gone wrong somewhere.
(The current implementation will clearly behave the same either way; but that's not something that's evident from looking at the test and the name of the function, and not something that necessarily has to stay true in the future.)
…nvariants` To include the "senders don't match" case, and rename it to `showSender`.
c326945 to
4b9ad17
Compare
|
Thanks for the review @gnprice! @chrisbobbe, pushed an update, PTAL. |
|
Thanks, LGTM! Merged. |
Fixes: #1825