fix: add BulkWriter integration tests, java backport changes, delete fix #1117

thebrianchen · 2020-06-08T21:57:35Z

A few things in this PR:

Added integration tests now that the protos are public
Backported some unit tests changes from java port (variable names and non-zero mock response times).
Add temporary fix to delete times (see b/158502664).

codecov · 2020-06-08T22:01:25Z

Codecov Report

Merging #1117 into node10 will decrease coverage by 0.00%.
The diff coverage is 98.79%.

@@            Coverage Diff             @@
##           node10    #1117      +/-   ##
==========================================
- Coverage   98.60%   98.59%   -0.01%     
==========================================
  Files          28       28              
  Lines       18366    18310      -56     
  Branches     1414     1405       -9     
==========================================
- Hits        18109    18053      -56     
  Misses        254      254              
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/v1/firestore_client.ts	`97.86% <98.74%> (ø)`
dev/src/bulk-writer.ts	`98.43% <100.00%> (+<0.01%)`	⬆️
dev/src/write-batch.ts	`100.00% <100.00%> (ø)`
dev/src/index.ts	`98.17% <0.00%> (-0.04%)`	⬇️

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 01b665f...f46b6c1. Read the comment docs.

schmidt-sebastian · 2020-06-09T17:04:57Z

dev/src/write-batch.ts

@@ -575,12 +575,33 @@ export class WriteBatch implements firestore.WriteBatch {
      api.BatchWriteResponse
    >('batchWrite', request, tag);

-    return (response.writeResults || []).map((result, i) => {
+    const writeResults = response.writeResults || [];


Do you mind removing GCF_IDLE_TIMEOUT_MS (the unrelated lint complaint above)?

it's being used further down in _shouldCreateTransaction().

dev/src/write-batch.ts

schmidt-sebastian · 2020-06-09T17:08:09Z

dev/src/write-batch.ts

-    return (response.writeResults || []).map((result, i) => {
+    const writeResults = response.writeResults || [];
+    let latestTimestamp = Timestamp.fromMillis(0);
+    writeResults.forEach(result => {


Nit: Prefer using for ... of loops when possible, as this makes for far easier debugging than inline lambdas.

n/a anymore

schmidt-sebastian · 2020-06-09T17:09:17Z

dev/src/write-batch.ts

      return new BatchWriteResult(
-        result.updateTime ? Timestamp.fromProto(result.updateTime) : null,
+        isSuccessfulDelete


I don't know what is going on here :) Can you rewrite this to make it obvious? At the very least, this needs parentheses.

I copied this style from your Transaction PR b/c I thought it seemed really cool! And then I went back and saw you changed it :(

schmidt-sebastian · 2020-06-09T17:10:07Z

dev/system-test/firestore.ts

-  it.skip('can terminate once BulkWriter is closed', async () => {
+  it('has create() method', async () => {
+    const ref = randomCol.doc('doc1');
+    const promise = writer.create(ref, {foo: 'bar'});


s/promise/singleOp/ ?

schmidt-sebastian · 2020-06-10T03:55:09Z

dev/src/bulk-writer.ts

@@ -113,7 +113,7 @@ class BulkCommitBatch {

  /**
   * Adds a `delete` operation to the WriteBatch. Returns a promise that
-   * resolves with the result of the delete.
+   * resolves with the sentinel value for the delete operation.


Should we point out that we are returning Timestamp(0)?

I'll add that to the description.

schmidt-sebastian · 2020-06-10T03:59:22Z

dev/src/write-batch.ts

+      // sentinel Timestamp value.
+      // TODO(b/158502664): Use actual delete timestamp.
+      const isSuccessfulDelete =
+        result.updateTime === null && error.code === Status.OK;


Suggested change

result.updateTime === null && error.code === Status.OK;

const DELETE_TIMESTAMP_SENTINEL : api.ITimestamp = {}

const updateTime = error.code === Status.OK

? Timestamp.fromProto(result.updateTime || DELETE_TIMESTAMP_SENTINEL)

: null;

For brevity.

so crisp, so clean.

* 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 <[email protected]> Co-authored-by: wu-hui <[email protected]>

Brian Chen added 3 commits June 3, 2020 17:07

backport unit test changes

fd92ddf

WIP: failing delete() test

b792f43

add delete fix

89910a0

thebrianchen self-assigned this Jun 8, 2020

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

cleanup

5193146

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

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

schmidt-sebastian reviewed Jun 9, 2020

View reviewed changes

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

Brian Chen added 2 commits June 9, 2020 17:07

resolve comments, add delete sentinel

eace950

lint

9188e4e

thebrianchen requested a review from schmidt-sebastian June 10, 2020 00:15

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

schmidt-sebastian approved these changes Jun 10, 2020

View reviewed changes

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

resolve comments

d149ee0

thebrianchen force-pushed the bc/bulk-sys branch from 73c30fb to d149ee0 Compare June 10, 2020 18:50

updateTime brevity

f46b6c1

thebrianchen merged commit 9224c0f into node10 Jun 10, 2020

thebrianchen deleted the bc/bulk-sys branch June 10, 2020 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add BulkWriter integration tests, java backport changes, delete fix #1117

fix: add BulkWriter integration tests, java backport changes, delete fix #1117

thebrianchen commented Jun 8, 2020 •

edited

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading

schmidt-sebastian Jun 9, 2020

thebrianchen Jun 9, 2020

schmidt-sebastian Jun 9, 2020

thebrianchen Jun 10, 2020

schmidt-sebastian Jun 9, 2020

thebrianchen Jun 10, 2020

schmidt-sebastian Jun 9, 2020

thebrianchen Jun 10, 2020

schmidt-sebastian Jun 10, 2020

thebrianchen Jun 10, 2020

schmidt-sebastian Jun 10, 2020

thebrianchen Jun 10, 2020

-        result.updateTime === null && error.code === Status.OK;
+        const DELETE_TIMESTAMP_SENTINEL : api.ITimestamp = {}
+        const updateTime = error.code === Status.OK
+            ?  Timestamp.fromProto(result.updateTime || DELETE_TIMESTAMP_SENTINEL)
+             : null;

fix: add BulkWriter integration tests, java backport changes, delete fix #1117

fix: add BulkWriter integration tests, java backport changes, delete fix #1117

Conversation

thebrianchen commented Jun 8, 2020 • edited Loading

codecov bot commented Jun 8, 2020 • edited Loading

Codecov Report

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

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 8, 2020 •

edited

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading