Skip to content
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

fix: fix flaky BulkWriter test #1115

Merged
merged 2 commits into from
Jun 9, 2020
Merged

fix: fix flaky BulkWriter test #1115

merged 2 commits into from
Jun 9, 2020

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Jun 7, 2020

Fixes #1097.

I couldn't reproduce this locally, but my hypothesis is that the test had floating promises that resulted in race conditions. If the second flush() call was made before the first flush() call sent its request, the rate limit threshold would not have have been reached for the second flush(), which meant the timeout handler that ended the test would never be invoked.

By waiting for promises to complete instead of using done(), the test should now execute in the correct order each time.

@thebrianchen thebrianchen self-assigned this Jun 7, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 7, 2020
@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #1115 into master will increase coverage by 1.20%.
The diff coverage is 98.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1115      +/-   ##
==========================================
+ Coverage   97.40%   98.61%   +1.20%     
==========================================
  Files          27       28       +1     
  Lines       16978    18301    +1323     
  Branches     1271     1405     +134     
==========================================
+ Hits        16538    18047    +1509     
+ Misses        436      251     -185     
+ Partials        4        3       -1     
Impacted Files Coverage Δ
dev/src/field-value.ts 97.06% <92.85%> (+0.01%) ⬆️
dev/src/types.ts 99.39% <93.33%> (-0.61%) ⬇️
dev/src/bundle.ts 93.92% <93.92%> (ø)
dev/src/index.ts 98.17% <95.51%> (+0.42%) ⬆️
dev/src/v1/firestore_client.ts 97.86% <98.85%> (+3.05%) ⬆️
dev/src/document.ts 98.77% <98.92%> (+0.15%) ⬆️
dev/src/v1beta1/firestore_client.ts 97.38% <99.02%> (+2.89%) ⬆️
dev/src/reference.ts 99.88% <99.54%> (+0.07%) ⬆️
dev/src/backoff.ts 100.00% <100.00%> (ø)
dev/src/bulk-writer.ts 98.85% <100.00%> (+0.36%) ⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ffe256...4d063c1. Read the comment docs.

@schmidt-sebastian
Copy link
Contributor

Fix LGTM. I think we should fix in master since #1097 is against master.

@thebrianchen thebrianchen changed the base branch from node10 to master June 8, 2020 00:08
@thebrianchen thebrianchen changed the base branch from master to node10 June 8, 2020 00:08
@thebrianchen thebrianchen changed the base branch from node10 to master June 8, 2020 00:11
@thebrianchen thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
@thebrianchen thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 9, 2020
@thebrianchen thebrianchen merged commit 9a24cc0 into master Jun 9, 2020
@thebrianchen thebrianchen deleted the bc/bulk-flake branch June 9, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mocha Tests: does not send batches if doing so exceeds the rate limit failed
4 participants