test(NODE-7197): migrate integration/crud/find_cursor_methods tests
#4727
Conversation
baileympearson
added
the
Primary Review
| context('iterator', function () { | ||
| it('should iterate each document in a cursor', async function () { | ||
| const coll = client.db().collection('abstract_cursor'); | ||
| const cursor = coll.find({}, { batchSize: 2 }); | ||
|
|
||
| const bag = []; | ||
| cursor.forEach( | ||
| doc => bag.push(doc), | ||
| err => { | ||
| expect(err).to.not.exist; | ||
| expect(bag).to.have.lengthOf(6); | ||
| done(); | ||
| } | ||
| ); | ||
| for await (const doc of cursor) { | ||
| bag.push(doc); | ||
| } | ||
| expect(bag).to.have.lengthOf(6); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Why was this method changed? We still support AbstractCursor.forEach().
There was a problem hiding this comment.
Do we have adequate coverage elsewhere?
There was a problem hiding this comment.
you are right, sorry for that confusion, I misinterpreted deprecated notation
| let doc: any; | ||
| while ((doc = (await cursor.next()) && doc != null)) { | ||
| /** empty */ | ||
| } |
There was a problem hiding this comment.
instead of casting, why not:
while ((await cursor.next()) != null) {
/** empty */
}There was a problem hiding this comment.
yes, this is much easier, thanks!
| await cursor.next(); | ||
|
|
||
| let doc; | ||
| let doc: any; |
| await cursor.next(); | ||
|
|
||
| let doc; | ||
| let doc: any; |
|
|
||
| const coll = client.db().collection('abstract_cursor'); | ||
| const cursor = coll.find({}, { batchSize: 2 }); | ||
| this.defer(() => cursor.close()); |
There was a problem hiding this comment.
These .close() calls are gone without replacement, right?
I think they were superfluous anyway because the tests exhausted the respective cursors, maybe it's worth and possible to check that they are already closed afterwards?
There was a problem hiding this comment.
Thanks for the question! Yes, we are re-creating client for every test within beforeEach hook and close client in afterEach, something like:
let client: MongoClient;
beforeEach(function () {
client = this.configuration.newClient();
});
afterEach(async function () {
await client.close();
});
And in addition to that we do check for opened clients after tests.
There was a problem hiding this comment.
Right, I wasn't concerned about resource leakage here – I guess what I'm saying is that previously, we verified that it's acceptable to close the cursor after exhausting it, and now we don't do this anymore (at least not in these tests). No strong feelings, but I was wondering if it's something worth keeping in some form, either by verifying that cursor.close() still works (not just client.close()), or by verifying that the cursor is already closed anyway
There was a problem hiding this comment.
Ah, that was a misunderstanding, after all these tests refactoring I think I lost a perspective and misinterpreted cursor with client (as it's also very common in our tests to close client in tests and not hooks). Thanks for paying attention to this.
I don't have opinion on this, down the file we have a dedicated describe('#close', function () { which tests cursor.close() in different contexts (before initialization, after iterations, etc.), so I think it would be ok to remove it from other places and close the entire client in other tests (like this one). What do you think?
There was a problem hiding this comment.
No strong feelings about whether to keep it or drop it – just wanted to make sure this change was intentional 🙂
3 participants
Description
Summary of Changes
This PR migrates the integration tests for
crud/find_cursor_methods. The changes include:srcfolderNotes for Reviewers
To simplify the review process there is a separate commit for .js -> .ts conversion.
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