-
Notifications
You must be signed in to change notification settings - Fork 244
DRIVERS-2975 Fix logic for populating BulkWriteException.partialResult
#1665
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
Conversation
BulkWriteException.partialResult
newDocument: &newDocument { _id: 2, x: 22 } | ||
|
||
tests: | ||
- description: "partialResult is unset when first operation fails during an ordered bulk write (verbose)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose and summary are both tested here to make sure that drivers aren't determining the index of the write error by its position in the cursor rather than the idx
field. The type of result is not relevant for the unordered logic, so I skipped testing both for the equivalent unordered test.
document: *newDocument | ||
ordered: true | ||
verboseResults: true | ||
expectError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other pieces of BulkWriteException
are tested elsewhere, so I skipped including them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a suggested change to test assertions. With suggested changes, verified tests pass in C driver locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with additional { $$exists: false }
.
insertedId: 2 | ||
updateResults: {} | ||
deleteResults: {} | ||
- description: "partialResult is set when first operation fails during an unordered bulk write (summary)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isabelatkinson: FYI, this is a duplicate test name (also used earlier on line 183). Assuming that was unintentional, what should this test actually be called?
Asking as this came up in mongodb/mongo-php-library#1429 /cc @alcaeus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad copy/paste, fixed here
Please complete the following before merging:
clusters, and serverless).