Fix flaky test. #1511
Merged
Fix flaky test. #1511
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
The "can return previous value through consecutive updates" test was flaky because it did two consecutive server timestamp writes to the same field and expected 2 local events. But if you do both writes within 1ms then the snapshots will be indistinguishable (the estimated value for the server timestamps will be the same) and so only 1 event will be raised. I added an additional change to the second write to guarantee it triggers an event. While looking at the test, I also noticed it was slightly broken / incomplete. There was an incorrect line attempting to disable the network and the test was missing the remaining code to re-enable the network and verify the behavior. I fixed that as well.
mikelehen
commented
Feb 1, 2019
| .then(() => { | ||
| // We set up two consecutive writes with server timestamps. | ||
| docRef.update('a', FieldValue.serverTimestamp()); | ||
| docRef.update('a', FieldValue.serverTimestamp()); | ||
| // include b=1 to ensure there's a change resulting in a new snapshot. | ||
| docRef.update('a', FieldValue.serverTimestamp(), 'b', 1); |
There was a problem hiding this comment.
This is the actual fix. The rest is just making the test actually disable/re-enable the network and match the other ports.
|
Er, actually I can fix this differently so that it both fixes the flakiness and is closer to the Android / iOS ports. One sec. |
|
Eh, nevermind. Android is flaky too though it's probably less likely (because it does more work between the subsequent writes), but there could still technically be a timing issue. |
mikelehen
pushed a commit
to firebase/firebase-android-sdk
that referenced
this pull request
Feb 1, 2019
mikelehen
pushed a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Feb 1, 2019
mikelehen
added a commit
to firebase/firebase-android-sdk
that referenced
this pull request
Feb 1, 2019
mikelehen
added a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Feb 1, 2019
Port flaky test fix from web. Port of firebase/firebase-js-sdk#1511
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The "can return previous value through consecutive updates" test was flaky
because it did two consecutive server timestamp writes to the same field and
expected 2 local events. But if you do both writes within 1ms then the
snapshots will be indistinguishable (the estimated value for the server
timestamps will be the same) and so only 1 event will be raised. I added an
additional change to the second write to guarantee it triggers an event.
While looking at the test, I also noticed it was slightly broken / incomplete.
There was an incorrect line attempting to disable the network and the test was
missing the remaining code to re-enable the network and verify the behavior. I
fixed that as well.