test(NODE-7204): migrate node-specific/cursor_stream tests
#4735
Conversation
| await collection.insertMany(docs, { writeConcern: { w: 1 } }); | ||
| } | ||
|
|
||
| describe('using Async Iterator (for await...of)', function () { |
There was a problem hiding this comment.
I don't think this describe is necessary. When we say "cursor streams", we generally refer to a Nodejs stream created from the cursor using cursor.stream().
Is there a reason you feel we need these tests?
There was a problem hiding this comment.
I might be wrong here, but my intention was the following: there are 2 reading modes in which streams are operating: paused and flowing. So i wanted to test both: in for...of the stream is in paused mode, meaning that we can easily perform any async operations between reads, back pressure should work without any additional actions. But in "flowing" mode (with .on('data')) it's no longer a case, so if we want to do something async we need to pause stream first.
In general, I don't have a strong opinion on this, and happy to simplify tests further.
There was a problem hiding this comment.
Sorry for the ambiguity. What I meant was, the for-await loop does not use a Nodejs stream (this is sort of blurry, because for await (const doc of cursor.stream()) uses both the ReadableCursorStream and the Symbol.asyncIterator implementation on readable streams, but this for-await loop does not). In general, when we say "stream" in relation to cursors, we mean a literal Nodejs readable stream, not the abstract concept of a "stream" and we test for-await loops more like the next() api (internally, for-await is just a wrapper around next()).
These tests are specifically testing AbstractCursor.stream() method, which returns a readable stream. I think this test can probably be removed.
I don't feel strongly about testing paused vs flowing modes, but that does seem like its testing Nodejs' stream internals at that point. Out ReadableCursorStream simply implements the _read() method.
| expect(docCount).to.equal(100); | ||
| }); | ||
|
|
||
| it('should handle pauses for async operations within the loop', async function () { |
There was a problem hiding this comment.
What exactly is this testing? What does "handle pauses" mean? Doesn't error? Doesn't include documents from the update or does include documents from the update?
| // Perform an async operation, then resume | ||
| updateCollection | ||
| .updateOne({ _id: 1 }, { $inc: { count: 1 } }, { upsert: true }) | ||
| // Manually resume | ||
| .then(() => stream.resume()) | ||
| .catch(reject); |
There was a problem hiding this comment.
| // Perform an async operation, then resume | |
| updateCollection | |
| .updateOne({ _id: 1 }, { $inc: { count: 1 } }, { upsert: true }) | |
| // Manually resume | |
| .then(() => stream.resume()) | |
| .catch(reject); | |
| // Perform an async operation, then resume | |
| sleep(100) | |
| // Manually resume | |
| .then(() => stream.resume()) | |
| .catch(reject); |
No reason to invoke a driver operation; suggest just sleeping instead.
| expect(finalUpdate.count).to.equal(10); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Is there a reason should correctly error out stream isn't included in these tests?
There was a problem hiding this comment.
thanks, that was oversight from my side
baileympearson
added
the
Primary Review
2 participants
Description
Summary of Changes
This PR rewrites the integration tests for
node-specific/cursor_stream.Notes for Reviewers
Unfortunately it wasn't possible to simple refactor tests for async/await interface as tests were written quite some time ago, and since nodejs streams support both interfaces (async iterator as well as eventemitter), I decided to rewrite tests. The logic of the tests is the same (insert documents and then stream them back), but organization is different (different describes for both interfaces).
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