Improve test coverage for SnapshotRequest.onConnectionStateChanged
#245
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
alecgibson
force-pushed
the
snapshot-request-test
branch
from
9f022ff to
c3c3c0c
Compare
This change improves `SnapshotRequest` test coverage to 100%. Our current test for checking that a dropped connection works didn't actually fail properly when removing `this.sent = false` from `SnapshotRequest._onConnectionStateChanged`, so an improved test has been written that use middleware to wait until the request has definitely been sent, before dropping the connection and reconnecting. It also adds a previously uncovered test case for checking that the connection cannot send the same request multiple times, by using a similar setup to the rewritten dropped connection case. This change also adds some documentation recording why we might want to reset `this.sent = false`, because just looking at the code it's unclear why we would ever want to do that.
alecgibson
force-pushed
the
snapshot-request-test
branch
from
c3c3c0c to
f7dcb37
Compare
|
Don't know why it claims that coverage has actually fallen 🤷♂️
|
|
Re the coverage drop, Coveralls thinks this PR has one fewer "relevant line" of code compared to the code on master:
Probably due to this on line 61: That's fine. Nothing we can do unless we add coverage for unrelated code. |
ericyhwang
approved these changes
Sep 5, 2018
| if (error) return done(error); | ||
| done(); | ||
| var connectionInterrupted = false; | ||
| backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function (request, callback) { |
There was a problem hiding this comment.
Can you add a comment about why this needs to use the middleware?
|
@ericyhwang I've added the comment you asked for. |
|
Thanks! I'll merge but hold off on publishing until we've got more changes, as this is just changes to the tests (minus the very minor refactor) |
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 change improves
SnapshotRequesttest coverage to 100%.Our current test for checking that a dropped connection works didn't
actually fail properly when removing
this.sent = falsefromSnapshotRequest._onConnectionStateChanged, so an improved test hasbeen written that use middleware to wait until the request has
definitely been sent, before dropping the connection and reconnecting.
It also adds a previously uncovered test case for checking that the
connection cannot send the same request multiple times, by using a
similar setup to the rewritten dropped connection case.
This change also adds some documentation recording why we might want to
reset
this.sent = false, because just looking at the code it'sunclear why we would ever want to do that.