-
Notifications
You must be signed in to change notification settings - Fork 930
Adding back callback assert #1081
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
Adding back callback assert #1081
Conversation
... and I have also make "keepInQueue" optional. It defaults to false, which reduces the diff of the JSON against master. |
a51fd41
to
2ba8368
Compare
2ba8368
to
3f3d7ec
Compare
Android Port of these changes: https://github.com/FirebasePrivate/firebase-android-sdk/pull/167 |
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.
LGTM I think. FWIW I'm not sure if reducing the json diff is a major goal. But if it makes porting easier or whatever, cool.
@@ -645,7 +645,7 @@ describeSpec('Writes:', [], () => { | |||
expectRequestCount({ handshakes: 2, writes: 2, closes: 1 }) | |||
) | |||
.expectNumOutstandingWrites(1) | |||
.writeAcks('collection/key', 1, { expectUserCallback: 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.
Was this a bug before? Else I'm confused why this changed since the default is true
, right? So omitting it is a change in behavior...
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.
Yeah, this was wrong. Disabling and re-enabling the network should not affect whether we resolve the user callback. This now fails since I am asserting that I didn't get any unexpected write acknowledgments.
The current spec tests in multi-tab don't verify the ackWrite/failWrite callbacks by default. This fixes this regression.