fix: another attempt at fixing the flaky BulkWriter test #1228

thebrianchen · 2020-06-18T01:56:39Z

No description provided.

codecov · 2020-06-18T01:59:05Z

Codecov Report

Merging #1228 into node10 will decrease coverage by 0.17%.
The diff coverage is 93.06%.

@@            Coverage Diff             @@
##           node10    #1228      +/-   ##
==========================================
- Coverage   98.59%   98.42%   -0.18%     
==========================================
  Files          28       30       +2     
  Lines       18301    18474     +173     
  Branches     1403     1423      +20     
==========================================
+ Hits        18044    18183     +139     
- Misses        254      288      +34     
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/index.ts	`97.51% <71.05%> (-0.67%)`	⬇️
dev/src/util.ts	`96.85% <91.42%> (-1.32%)`	⬇️
dev/src/bulk-writer.ts	`98.86% <100.00%> (+0.43%)`	⬆️
dev/src/document.ts	`98.77% <100.00%> (-0.01%)`	⬇️
dev/src/path.ts	`98.55% <100.00%> (-0.01%)`	⬇️
dev/src/reference.ts	`99.88% <100.00%> (+<0.01%)`	⬆️
dev/src/transaction.ts	`97.03% <100.00%> (ø)`
dev/src/write-batch.ts	`100.00% <100.00%> (ø)`
.prettierrc.js	`0.00% <0.00%> (ø)`
... and 3 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 13871d9...43a0fac. Read the comment docs.

schmidt-sebastian · 2020-06-18T16:45:05Z

dev/test/bulk-writer.ts

+        // there are floating promises, not waiting here can cause a race
+        // condition where the first batch's request has not been made by the
+        // time the second request is sent.
+        await requestMadeDeferred.promise;


I am a little confused here... isn't the test meant to send off a lot of writes as fast as possible, and as such it would be beneficial not to wait?

It seems like what we should do is:

setTimeoutHandler((_, timeout) => { if (!timeoutCalled) { timeoutCalled = true; // expect(requestCounter).to.equal(0); This is racy without your // deferred promise, but not needed for the purpose of the test expect(timeout).to.be.greaterThan(0); // done(); Maybe remove this so we don't have any outstanding // work when the test completes. } }); for (let i = 0; i < 600; i++) { bulkWriter.set(firestore.doc('collectionId/doc' + i), {foo: 'bar'}); } await bulkWriter.close(); expect(timeoutCalled).to.be.true;

Post conversation:
The original issue with this simplified test was that it still passes even if the timeout handler is called for the first batch. To fix this, the BulkWriter tests now check that the timeout handler wasn't called after each individual test.

I tried removing the done(), but since it's based on the actual clock, the timeout handler will be called multiple times. If we just call the timeout function directly in the overridden handler, we will loop until enough time passes for the rate limiter to allow the request. Though there could still be outstanding work once the test completes, it shouldn't affect other tests since each test creates a new BulkWriter instance.

thebrianchen · 2020-06-18T22:44:31Z

I also removed the response generation logic since the rate limiter test isn't testing the request/response logic. This also allows the test to pass independently of what the maximum batch size is.

schmidt-sebastian · 2020-06-18T22:57:54Z

dev/test/bulk-writer.ts

+          const responses = mergeResponses(
+            Array.from(new Array(requestLength), (_, i) => successResponse(i))
+          );
+          return response({


This is a pretty great simplification.

schmidt-sebastian · 2020-06-18T22:59:37Z

dev/test/bulk-writer.ts

-    it.skip('does not send batches if doing so exceeds the rate limit', async () => {
-      // The test is considered a success if BulkWriter tries to send the second
-      // batch again after a timeout.
+    afterEach(() => setTimeoutHandler(setTimeout));


I think we probably don't need this anymore since this is called above.

schmidt-sebastian · 2020-06-18T23:00:32Z

dev/test/bulk-writer.ts

+        }
+        await bulkWriter.close();
+        if (!timeoutCalled) {
+          done(new Error('Expected the timeout handler to be called'));


It's actually an error to invoke done() twice. This should show up in the console.

Either the done() in the timeout handler will be called, setting timeoutCalled to true, or the error done() will be called. Calling done() twice will cause the test to fail.

schmidt-sebastian · 2020-06-18T23:01:41Z

dev/test/bulk-writer.ts

+        for (let i = 0; i < 600; i++) {
+          bulkWriter.set(firestore.doc('collectionId/doc' + i), {foo: 'bar'});
+        }
+        await bulkWriter.close();


If you don't wait for the close to finish (i.e. no dangling promise in this test), then verifyInstance (line 189) might become flaky since it verifies that at the end of each test all operations have finished executing.

talked separately: The 2nd request will never fire since we are not invoking the callback function in the overridden handler. This means that the the handler will be trigger twice: once when the 2nd request is made, and once when the first request is returned.

schmidt-sebastian · 2020-06-19T05:13:12Z

dev/test/bulk-writer.ts

-      const arrayRange2 = [500, 501, 502, 503, 504];
-      const requests2 = arrayRange2.map(i => setOp('doc' + i, 'bar'));
-      const responses2 = arrayRange2.map(i => successResponse(i));
+    it('does not send batches if doing so exceeds the rate limit', done => {


Final nit: This test should get a more obvious name "Uses timeout for batches that exceed the rate limit" for example.

* fix!: mark v1beta1 client as deprecated (#937) * feat!: use QueryDocumentSnapshot in FirestoreDataConverter (#965) * deps: update to gts 2.x (#1013) * chore!: update settings for Node 10 (#1019) * deps: drop through2 (#1014) * feat: support BigInt (#1016) * fix: make update.sh work on Linux (#1043) * fix: only use BigInt in BigInt system test (#1044) * fix: make pbjs compile admin proto again (#1045) * Add BulkWriter (#1055) * docs: Add documentation for FirestoreDataConverter (#1059) * chore: enforce return types (#1065) * fix: add generic to Firestore.getAll() (#1066) * chore: remove internal WriteOp (#1067) * chore: add linter checks for it|describe.only (#1068) * fix: handle terminate in BulkWriter (#1070) * chore: run template copying last in synthtool (#1071) * feat: Firestore Bundles implementation (#1078) * feat: add support for set() with SetOptions when using FirestoreDataConverter (#1087) * feat: Add totalDocuments and totalBytes to bundle metadata. (#1085) * feat: Add totalDocuments and totalBytes to bundle metadata. * fix: Better comment * fix: Better testing. * fix: Improve metadata testing. * fix: incomplete expect in rate-limiter test (#1092) * Remove BatchWrite proto, fix conformance tests * chore: use public API types internally (#1100) * feat: add Partition and BatchWrite protos (#1110) * fix: remove GCF transaction fallback (#1112) * fix: add BulkWriter integration tests, java backport changes, delete fix (#1117) * chore: merge master (#1218) * chore: add eslint check for console.log statements (#1229) * fix: another attempt at fixing the flaky BulkWriter test (#1228) * Fix comment * Renames * Test fix * Fix unit tests Co-authored-by: Brian Chen <chenbrian@google.com> Co-authored-by: wu-hui <53845758+wu-hui@users.noreply.github.com>

thebrianchen self-assigned this Jun 18, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 18, 2020

thebrianchen requested a review from schmidt-sebastian June 18, 2020 02:01

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 18, 2020

schmidt-sebastian reviewed Jun 18, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 18, 2020

fix: another attempt at fixing the flaky BulkWriter test

d17723c

thebrianchen force-pushed the bc/bulk-flake branch from 489260d to 4b5251d Compare June 18, 2020 22:09

simplify tests and add timeout handler checking

b46e184

thebrianchen force-pushed the bc/bulk-flake branch from a3629ae to b46e184 Compare June 18, 2020 22:40

remove unused import

b22c370

thebrianchen requested a review from schmidt-sebastian June 18, 2020 22:45

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 18, 2020

schmidt-sebastian reviewed Jun 18, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 18, 2020

remove await and unneeded afterEach

17f8ba5

thebrianchen unassigned wu-hui Jun 18, 2020

thebrianchen requested a review from schmidt-sebastian June 18, 2020 23:55

thebrianchen assigned schmidt-sebastian Jun 18, 2020

schmidt-sebastian reviewed Jun 19, 2020

View reviewed changes

schmidt-sebastian approved these changes Jun 19, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 19, 2020

rename testg

43a0fac

thebrianchen merged commit 2f417f7 into node10 Jun 19, 2020

thebrianchen deleted the bc/bulk-flake branch June 19, 2020 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: another attempt at fixing the flaky BulkWriter test #1228

fix: another attempt at fixing the flaky BulkWriter test #1228

thebrianchen commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

schmidt-sebastian Jun 18, 2020

thebrianchen Jun 18, 2020

thebrianchen commented Jun 18, 2020

schmidt-sebastian Jun 18, 2020

schmidt-sebastian Jun 18, 2020

thebrianchen Jun 18, 2020

schmidt-sebastian Jun 18, 2020

thebrianchen Jun 18, 2020

schmidt-sebastian Jun 18, 2020

thebrianchen Jun 18, 2020

schmidt-sebastian Jun 19, 2020

fix: another attempt at fixing the flaky BulkWriter test #1228

fix: another attempt at fixing the flaky BulkWriter test #1228

Conversation

thebrianchen commented Jun 18, 2020 • edited Loading

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading