Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Even more streams tests #461

Merged
merged 1 commit into from
Mar 27, 2023
Merged

Even more streams tests #461

merged 1 commit into from
Mar 27, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 20, 2023

No description provided.

@jasnell jasnell requested review from mikea and harrishancock March 21, 2023 14:07
@jasnell jasnell force-pushed the jsnell/nodejs-streams-tests-moar branch from e5028ea to deb214e Compare March 23, 2023 23:11
@jasnell jasnell force-pushed the jsnell/nodejs-streams-tests-moar branch from 13e16a6 to 5bfc864 Compare March 23, 2023 23:57
@jasnell jasnell merged commit f84e794 into main Mar 27, 2023
Copy link
Contributor

@Warfields Warfields left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of the way done yet, but here are a couple things to look at

}
return true;
})
await rejects(stream.map((x) => x + x).toArray(), /boom/);
Copy link
Contributor

@Warfields Warfields Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure on the expected behavior of streams, do we want to check if the error occurs on the third stream object to make sure that things happen in the right order? Similarly, on the async case.

{
// Test result is a Readable
const stream = Readable.from([1, 2, 3, 4, 5]).filter((x) => true);
strictEqual(stream.readable, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing the negative case may be valuable here. Ex. testing strictEqual(stream.readable, false) when it is not safe to call read()

strictEqual(source._readableState.pipes[1], dest2);
strictEqual(source._readableState.pipes.length, 2);

// Should be able to unpipe them in the reverse order that they were piped.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should happen if we don't unpipe them in order?

}
};

export const pipeMultiplePipes = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this test is doing the following

              ┌────────┐
    ┌────────►│Writable│
    │         └────────┘
    │
┌───┴─────┐   ┌────────┐
│Readable ├──►│Writable│
└───┬─────┘   └────────┘
    │
    │         ┌────────┐
    └────────►│Writable│
              └────────┘

Still going through this PR, but it so far does not have a test for long chains of piped streams.

┌─────────┐  ┌──────┐  ┌──────┐ ...  ┌──────┐
│Readable ├─►│Duplex├─►│Duplex├─────►│Duplex│
└─────────┘  └──────┘  └──────┘      └──────┘

@fhanau fhanau deleted the jsnell/nodejs-streams-tests-moar branch December 4, 2023 22:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants