msglist test: Backfill tests for rest of model; overhaul test setup #260
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
We don't have any need for these here -- the code under test isn't mounting any widgets (let alone GlobalStoreWidget) nor otherwise interacting with the global state. Leave them out, simplifying the tests.
The code under test doesn't actually look at this information -- the only thing it uses the PerAccountStore for is to (a) get an API connection, and (b) unregister itself in `dispose`. So simplify the tests by leaving it out. If later the code under test does start needing this information so that we do need to set it up in the tests, we can revisit then and may find a bit simpler of a way to arrange it.
Now that we don't have setup code trying to coordinate on using the same arbitrary user ID as any other code, the two test cases that happen to need to mention a user ID can just mention one inline. That avoids the appearance of this `userId` constant potentially having subtle wider interactions.
These values are, as the doc on their class says, view-models for message lists. That's a type of view-model, not a type of message list; or more briefly, it's a type of model.
None of these tests actually care what the narrow is. So use the narrow that means we can have any kind of message there without causing any inconsistency.
Now that the narrow is AllMessagesNarrow, there's no need for these test messages to be in any particular stream, so we can simplify a bit further.
All the tests in this file are going to have a common structure to the objects they operate on: chiefly, a MessageListView. So we can hoist those up to the level of `main`, which enables the tests to share helper functions that act on those objects. In principle this means a state leak between tests. We mitigate that by resetting all the shared variables at the same time, in the `prepare` function that each test calls at the outset. We could get a similar effect by putting these shared variables and helpers as fields and methods on a class, and having each test case make its own instance of the class -- its own "tester" object, akin to the WidgetTester object that `testWidgets` provides to its test body. That'd provide more encapsulation, and would be a good strategy if for example this logic were to be shared across several test files. But I think this test file can get away without it. The immediate payoff of the shared helpers is that they let us centralize the logic for checking that the view-model has, or hasn't, notified its listeners. That simplifies each test case, plus it lets us make that logic slightly more sophisticated (at O(1) rather than O(n) cost in the source code): we now detect duplicate calls too. Coming up, we'll add further centralized useful logic to these helpers, and add some tests that benefit from the split between `prepare` and `prepareMessages`.
This takes further advantage of having these centralized helpers.
For the more recently-added functionality, we'd written tests along the way. But we didn't have tests for the logic we built earlier, which is some of the most core functionality. Add tests for all that logic too.
|
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 is part of #78. That issue will involve substantial refactoring on MessageListView, which makes it a good time to have tests in place.
The final commit here adds tests for all the exposed methods of MessageListView that lack them, namely
fetch,maybeAddMessage, andreassemble. That was in fact the origin of this PR — I wrote those tests a few weeks ago as part of my draft branch for #78. (Which I've now picked back up to bring toward merge, after having held off for a while because its changes inlib/conflicted with other PRs and I wanted to handle the conflicts myself rather than pushing them to those PRs' authors to deal with.) The 11 preceding commits convert all the existing tests in this file to work the same way as those new tests.In particular the first 9 of these 12 commits are a sort of followup to #258, making further cleanups and simplifications to those existing tests, compared to the patterns that were set in #238.
The remaining two commits make deeper changes:
msglist test: Overhaul setup of tests, with shared helpers
All the tests in this file are going to have a common structure
to the objects they operate on: chiefly, a MessageListView.
So we can hoist those up to the level of
main, which enablesthe tests to share helper functions that act on those objects.
In principle this means a state leak between tests. We mitigate
that by resetting all the shared variables at the same time,
in the
preparefunction that each test calls at the outset.We could get a similar effect by putting these shared variables and
helpers as fields and methods on a class, and having each test case
make its own instance of the class -- its own "tester" object, akin to
the WidgetTester object that
testWidgetsprovides to its test body.That'd provide more encapsulation, and would be a good strategy if
for example this logic were to be shared across several test files.
But I think this test file can get away without it.
The immediate payoff of the shared helpers is that they let us
centralize the logic for checking that the view-model has, or hasn't,
notified its listeners. That simplifies each test case, plus it
lets us make that logic slightly more sophisticated (at O(1) rather
than O(n) cost in the source code): we now detect duplicate calls too.
Coming up, we'll add further centralized useful logic to these helpers,
and add some tests that benefit from the split between
prepareand
prepareMessages.msglist test: Systematically check invariants of MessageListView
This takes further advantage of having these centralized helpers.