Skip to content

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

Merged

Conversation

schmidt-sebastian
Copy link
Contributor

The current spec tests in multi-tab don't verify the ackWrite/failWrite callbacks by default. This fixes this regression.

@schmidt-sebastian
Copy link
Contributor Author

... and I have also make "keepInQueue" optional. It defaults to false, which reduces the diff of the JSON against master.

@schmidt-sebastian schmidt-sebastian force-pushed the multitab-addbackcallbackassert branch 2 times, most recently from a51fd41 to 2ba8368 Compare August 2, 2018 02:08
@schmidt-sebastian schmidt-sebastian force-pushed the multitab-addbackcallbackassert branch from 2ba8368 to 3f3d7ec Compare August 2, 2018 02:29
@schmidt-sebastian
Copy link
Contributor Author

Copy link
Contributor

@mikelehen mikelehen left a 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 })
Copy link
Contributor

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...

Copy link
Contributor Author

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.

@schmidt-sebastian schmidt-sebastian merged commit 4330039 into firestore-multi-tab Aug 2, 2018
@schmidt-sebastian schmidt-sebastian deleted the multitab-addbackcallbackassert branch August 3, 2018 17:15
@firebase firebase locked and limited conversation to collaborators Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants