Clean up and tighten UpdateMessageEvent tests #258
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
If the fetch somehow didn't get the right number of messages, most of the subsequent tests couldn't pass anyway.
The variable and the listener that updates it are naturally closely related, so group them together. Then group the subsequent maybeUpdateMessage call together with the checks that inspect that call's behavior.
The subject, or "actual", we're inspecting is the Message that one finds in the data structure. The thing we're checking about it is that it is the same object as the Message we found there before.
This test is for `maybeUpdateMessage`, not `fetch`. So it's not its job to check that `fetch` successfully got the content and flags and isMeMessage that we expected. What is useful for a "before" check here, on the other hand, is to confirm that our "after" checks aren't going to be vacuous, by checking that the message doesn't already have the values we'll be looking for there. So check that instead. Include one for lastEditTimestamp, which was previously left out of the "before". A low-tech way to express this would be to keep the checks like `.equals(oldContent)` and add assertions that oldContent differed from newContent, and so on. But we can do it a bit more neatly using the `not` check.
… point The thing we're checking in "after" is that the message has been updated to reflect the values in the update event. So let's express that directly. This also lets us do away entirely with auxiliary names for various pieces of the update event's data.
These two cases differed only in their name, the `renderingOnly` value in the event, and that one of them used `[0]` instead of `.single`. Fix the latter, parameterize, and dedupe.
Each of the test cases calls setupStore and then uses the resulting store for just one thing, namely passing it to messageListViewWithMessages. So just subsume the former into the latter.
|
Thanks, LGTM! Merged. |
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 makes a number of small improvements to the tests for our recently-added handling of
update_messageevents editing messages (in #238), as discussed at #256 (review) .