fix(Bulk): change BulkWriteError message to first item from writeErrors #2013
Conversation
test/functional/bulk_tests.js
Outdated
| @@ -937,6 +937,69 @@ describe('Bulk', function() { | |||
| } | |||
| }); | |||
|
|
|||
| it( | |||
| 'should provide descriptive error message for Unordered Batch with duplicate key errors on updates', | |||
There was a problem hiding this comment.
nit: we can change this from Unordered Batch to unordered batch (no caps).
test/functional/bulk_tests.js
Outdated
| it( | ||
| 'should provide descriptive error message for Unordered Batch with duplicate key errors on updates', | ||
| { | ||
| metadata: { requires: { topology: ['single', 'replicaset', 'ssl', 'heap', 'wiredtiger'] } }, |
There was a problem hiding this comment.
Since we're allowing every topology here, we can change this test from
it('title', {
metadata: { requires: { topology: ['single', 'replicaset', 'ssl', 'heap', 'wiredtiger'] } },
test: function(done) {
});
to
it('title', function(done) {
});
and it will run on all topology types.
test/functional/bulk_tests.js
Outdated
|
|
||
| test: function(done) { | ||
| const configuration = this.configuration; | ||
| var client = configuration.newClient(configuration.writeConcernMax(), { |
There was a problem hiding this comment.
Can you switch all of these vars to const? const is more modern and we are trying to slowly modernize our tests by making any new tests modern.
test/functional/bulk_tests.js
Outdated
| writeConcern.sparse = false; | ||
|
|
||
| // Add unique index on b field causing all updates to fail | ||
| col.ensureIndex({ b: 1 }, writeConcern, function(err) { |
There was a problem hiding this comment.
Since ensureIndex is deprecated in favor of createIndexes, can we use col.createIndexes instead? Here are the docs for how to use createIndexes.
test/functional/bulk_tests.js
Outdated
| expect(result.nInserted).to.equal(2); | ||
| expect(result.hasWriteErrors()).to.equal(true); | ||
| expect( | ||
| result.getWriteErrorCount() === 4 || result.getWriteErrorCount() === 3 |
There was a problem hiding this comment.
I think we can change this test to ensure our error count is always 3. Maybe we can get rid of some of the batch.inserts.
test/functional/bulk_tests.js
Outdated
| client.close(); | ||
| done(); | ||
| }); | ||
| client.close(); |
There was a problem hiding this comment.
Let's change this to client.close(done); since we're here. This helps us modernize our tests.
test/functional/bulk_tests.js
Outdated
| // Add unique index on b field causing all updates to fail | ||
| col.ensureIndex({ b: 1 }, writeConcern, function(err) { | ||
| expect(err).to.not.exist; | ||
| client.connect(function(err, client) { |
There was a problem hiding this comment.
For all callbacks in this test, let's use (err, client) => { notation instead of function(err, client) {.
lib/bulk/common.js
Outdated
| @@ -1119,7 +1119,7 @@ class BulkOperationBase { | |||
| callback, | |||
| new BulkWriteError( | |||
| toError({ | |||
| message: 'write operation failed', | |||
| message: this.s.bulkResult.writeErrors[0].errmsg, | |||
There was a problem hiding this comment.
What happens if this.s.bulkResult.writeErrors[0] or its errmsg doesn't exist? Let's check that the errmsg exists and use the default write operation failed message if not.
Fixes NODE-1965
Description
An unspecific
BulkWriteError: write operation failedmessage was changed to the first item fromwriteErrors.What changed?
A user requested to make the error more specific and suggested to use the first item from
writeErrors. A change was made to theMongoErrormessage field to reflect this error message.