test(NODE-7194): migrate integration/crud/crud_api tests
#4710
Conversation
|
|
||
| it('should correctly execute aggregation method using crud api', function (done) { | ||
| const db = client.db(); | ||
| describe('should correctly execute aggregation method using crud api', () => { |
There was a problem hiding this comment.
This can be a difficult part to review, so I want to share the context and my reasoning behind converting one test in it into separate block describe:
in the previous version we were testing multiple independent cursor methods (forEach, next, etc.) within the same function, and use callbacks chain to run them in series. So the structure looked like:
it(..., (done) => {
function test1() {
testMethod1(() => {
test2();
});
}
function test2() {
testMethod2(() => {
done();
});
}
test1();
});
I decided to split this into multiple independent tests (multiple it) within the same block (one describe). And also converted callbacks into async/await.
Let me know if it works.
|
|
||
| it('should correctly execute insert methods using crud api', function (done) { | ||
| client.connect(function (err, client) { | ||
| describe('should correctly execute insert methods using crud api', function () { |
There was a problem hiding this comment.
Please see my comment from the above with the reasoning behind changing single it into describe.
| // in this case we are setting that node needs to be higher than 0.10.X to run | ||
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| describe( |
There was a problem hiding this comment.
Same here, please see my comment from the above with the reasoning behind changing single it into describe.
| // Execute the command with all steps defined | ||
| // will fail | ||
| const err = await cursor.toArray().catch(err => err); | ||
| expect(err).to.be.instanceof(MongoServerError); |
There was a problem hiding this comment.
Since we now catch error with const e = await p.catch(e => e); we get e initialized in both cases (resolve and reject), so I changed this assertion to instanceof instead of previous (test.ok(err != null);) which worked well with callbacks.
| .collection('t20_1') | ||
| .bulkWrite(ops, { ordered: true, writeConcern: { w: 1 } }) | ||
| .catch(err => err); | ||
| expect(err).to.be.instanceOf(MongoBulkWriteError); |
There was a problem hiding this comment.
The original assertion test.ok(err !== null); won't work well with promises, so I changed it to instanceof.
baileympearson
added
the
Primary Review
| it('allMethods', async function () { | ||
| const cursor = db.collection('t1').aggregate([{ $match: {} }], { | ||
| allowDiskUse: true, | ||
| batchSize: 2, | ||
| maxTimeMS: 50 | ||
| }); | ||
|
|
||
| // | ||
| // Exercise each | ||
| // ------------------------------------------------- | ||
| const testEach = function () { | ||
| let count = 0; | ||
| const cursor = db.collection('t1').aggregate(); | ||
| cursor.match({ a: 1 }); | ||
| cursor.forEach( | ||
| () => { | ||
| count = count + 1; | ||
| }, | ||
| err => { | ||
| expect(err).to.not.exist; | ||
| test.equal(3, count); | ||
| testStream(); | ||
| } | ||
| ); | ||
| }; | ||
| // Exercise all the options | ||
| cursor | ||
| .geoNear({ geo: 1 }) | ||
| .group({ group: 1 }) | ||
| .limit(10) | ||
| .match({ match: 1 }) | ||
| .maxTimeMS(10) | ||
| .out('collection') | ||
| .project({ project: 1 }) | ||
| .redact({ redact: 1 }) | ||
| .skip(1) | ||
| .sort({ sort: 1 }) | ||
| .batchSize(10) | ||
| .unwind('name'); | ||
|
|
||
| // | ||
| // Exercise stream | ||
| // ------------------------------------------------- | ||
| const testStream = function () { | ||
| const cursor = db.collection('t1').aggregate(); | ||
| let count = 0; | ||
| cursor.match({ a: 1 }); | ||
| const stream = cursor.stream(); | ||
| stream.on('data', function () { | ||
| count = count + 1; | ||
| }); | ||
| // Execute the command with all steps defined | ||
| // will fail | ||
| const err = await cursor.toArray().catch(err => err); | ||
| expect(err).to.be.instanceof(MongoServerError); | ||
| }); |
There was a problem hiding this comment.
What is the value of this test? Maybe we can schedule it for removal.
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, | ||
| function () { | ||
| it('legacy update', async function () { |
There was a problem hiding this comment.
We can remove this test, there is no Collection.update method anymore.
| { | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, |
There was a problem hiding this comment.
Suggest removing these entirely; these general CRUD tests should pass on any configuration and topology.
| it('should correctly execute removeMany with no selector', { | ||
| // Add a tag that our runner can trigger on | ||
| // in this case we are setting that node needs to be higher than 0.10.X to run | ||
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, |
There was a problem hiding this comment.
same comment as above about metadata
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, |
There was a problem hiding this comment.
same comment as above about metadata
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, |
There was a problem hiding this comment.
same comment as above about metadata
| } | ||
| }); | ||
|
|
||
| it('should correctly execute crud operations using w:0', { |
There was a problem hiding this comment.
This test is already covered by should correctly execute crud operations with w:0, right? I think we can schedule this one for deletion.
| }); | ||
| }); | ||
|
|
||
| it('should correctly throw error on illegal callback when ordered bulkWrite encounters error', { |
There was a problem hiding this comment.
Can we take this opportunity to update the test title so it accurately reflects that the test is testing?
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, |
There was a problem hiding this comment.
same comment about metadata
| metadata: { | ||
| requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } | ||
| requires: { topology: ['single', 'replicaset', 'sharded'] } | ||
| }, | ||
|
|
There was a problem hiding this comment.
same comment about metadata
Description
Summary of Changes
This PR migrates the integration tests for
crud/crud_api. The changes include:srcfolderitinto separatedescribewith multiple tests in itNotes for Reviewers
The file is huge, and there are many changes in it, but I tried to keep the same structure, I hope it's still manageable to review it. Let me know if you want me to break down PR into pieces somehow.
What is the motivation for this change?
This work is part of a larger, ongoing initiative to convert all tests to use
async/await, with the ultimate goal of removing the legacy driver wrapper.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript