fix: add BulkWriter system tests + pass BE error messages #252
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## bc/bulk-master #252 +/- ##
=================================================
Coverage 73.72% 73.73%
- Complexity 1087 1088 +1
=================================================
Files 67 67
Lines 5866 5868 +2
Branches 642 642
=================================================
+ Hits 4325 4327 +2
Misses 1341 1341
Partials 200 200
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I just noticed that these test don't verify that the BulkWriter makes the requested changes. Can you read the documents back and add asserts that they match the written data.
We also had a bug in our implementation that dropped Preconditions (caught during review). Can we add a test that verifies the we set the "Create" and "Update" preconditions. These should fail if triggered correctly.
There was a problem hiding this comment.
I just noticed that these test don't verify that the BulkWriter makes the requested changes. Can you read the documents back and add asserts that they match the written data.
We also had a bug in our implementation that dropped Preconditions (caught during review). Can we add a test that verifies the we set the "Create" and "Update" preconditions. These should fail if triggered correctly.
| public void bulkWriterCreateAddsPrecondition() throws Exception { | ||
| DocumentReference docRef = randomColl.document(); | ||
| docRef.set(Collections.singletonMap("foo", (Object) "bar")).get(); | ||
| BulkWriter writer = firestore.bulkWriter(); |
There was a problem hiding this comment.
These test could profit from some line breaks. You will want to separate the setup, the action and the verfication:
@Test
public void bulkWriterCreateAddsPrecondition() throws Exception {
DocumentReference docRef = randomColl.document();
docRef.set(Collections.singletonMap("foo", (Object) "bar")).get();
BulkWriter writer = firestore.bulkWriter();
ApiFuture<WriteResult> result = writer.create(docRef, Collections.singletonMap("foo", (Object) "bar"));
writer.close().get();
try {
result.get();
fail("Create operation should have thrown exception");
} catch (Exception e) {
assertTrue(e.getMessage().contains("Document already exists"));
}
}
| @Test | ||
| public void bulkWriterUpdate() throws Exception { | ||
| DocumentReference docRef = randomColl.document(); | ||
| docRef.set(Collections.singletonMap("foo", "bar0")).get(); |
There was a problem hiding this comment.
s/bar0/oldValue
s/bar/newValue
| docRef.set(Collections.singletonMap("foo", "bar0")).get(); | ||
| BulkWriter writer = firestore.bulkWriter(); | ||
| ApiFuture<WriteResult> result = | ||
| writer.update(docRef, Collections.singletonMap("foo", (Object) "bar")); |
There was a problem hiding this comment.
Why not use varargs here? Removes the ugly cast.
Porting integration tests from node.
After running into a few BE contention errors during testing, I also added the error message to
BatchWriteResult, since backend errors were not being returned in a useful manner.