test(crud): resync crud tests and update test runner #2005
Conversation
| expect(result).to.not.be.an('error'); | ||
| } | ||
|
|
||
| // TODO: when we fix our bulk write errors, get rid of this |
There was a problem hiding this comment.
Is there a ticket for this?
There was a problem hiding this comment.
I just made the same comment above at the point of function definition 😄
There was a problem hiding this comment.
I thought there was, b/c it's been a known issue for a while, but I can't seem to find one, so I made https://jira.mongodb.org/browse/NODE-1989
| expect(result).to.not.be.an('error'); | ||
| } | ||
|
|
||
| // TODO: when we fix our bulk write errors, get rid of this |
There was a problem hiding this comment.
I just made the same comment above at the point of function definition 😄
| @@ -162,7 +206,7 @@ describe('CRUD spec', function() { | |||
| .find({}) | |||
| .toArray() | |||
| .then(collectionResults => { | |||
| expect(collectionResults).to.containSubset(outcome.result); | |||
| expect(collectionResults).to.containSubset(outcome.collection.data); | |||
There was a problem hiding this comment.
I think we still need to check this in v1, see this for example
There was a problem hiding this comment.
We are already asserting outcome.results here. This line is not asserting the result of the function call, but rather the state of the collection after the operation has been performed. If we were to use the example you provided above, we would be asserting that the entire collection would equal 3.
There was a problem hiding this comment.
oh wow, yeah that's a bug. Do we need a more rigorous validator than containsSubset?
There was a problem hiding this comment.
containsSubset seems to default to equals for non-objects, so I think we're fine.
test/functional/crud_spec_tests.js
Outdated
| ); | ||
| const asserter = assertWriteExpectations(collection, scenarioTest.outcome); | ||
|
|
||
| return collection[scenarioTest.operation.name](documents, options).then(asserter, asserter); |
There was a problem hiding this comment.
This used to return a Promise with only a then attached to it, allowing errors to propagate back up if something went wrong before the expectations were checked. Now you pass in the "asserter" to both resolve and reject, but don't pass in a method that checks for an error.
To me it seems like it would be easier to read this flow if we did something more like:
const promise = collection[scenarioTest.operation.name](documents, options);
if (scenario.hasError) {
promise.catch(doErrorChecks);
}
return promise.then(asserter);
Files to look at:
lib/bulk/common.js: minor change to exposeBulkWriteErrorfor aninstanceofcheck intest/functional/crud_spec_tests.jstest/functional/crud_spec_tests.js: Updated it to run with some of the new features that were added intov1beforev2was created. Specifically:scenario.datacan be an empty array, which will result in an empty insert attempt / erroroutcome.collection.datainstead ofoutcome.resultsbulkWriteandinsertMany), and added a transform function to massage ourBulkWriteErrorinto the shape it should be.Files to ignore: Any yml/json/rst files