test: add test for pipeline on http response stream

[elliot-nelson]

Test a corner case of the internal/streams/end-of-stream module by passing an HTTP response stream into a stream pipeline.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[addaleax]

Writable streams don’t have a 'data' event, so this won’t end up being called? (I’m also not sure that it’s guaranteed that the 10 sent chunks also end up being received as 10 separate chunks – I feel like they might end up being grouped together).

Generally, I’d recommend wrapping all callbacks that are called a fixed number of times with common.mustCall(...) or common.mustCall(..., n), it helps with making sure that the test works like it’s supposed to :)

[elliot-nelson]

Actually, I'll have to noodle on this a bit more... Now that I look closer, the lines I was attempting to cover (in end-of-stream) are related to a request stream that has a .req property. I can't think of any legit reason for such a thing to exist.

(I can craft a test case where I construct a request stream and attach another stream to it on the key req, just to cover the LOC, but I'd rather figure out how such an event could happen in the wild.)

[elliot-nelson]

I got more information from the original author, and it looks like the few uncovered lines in the end-of-stream coverage are to normalize the behavior of the request library.

(In Request, the object returned from making a call is a Request object that is itself a stream, but also has a .req property that represents the underlying stream.)

I'm actually not sure what next steps would be, possibilities:

1. Do nothing, except perhaps add comments inline so people know what they're looking at?
2. This doesn't belong in core, remove request-specific lines.
3. Add a unit test using an actual request object (node core doesn't require npm packages, so this doesn't make much sense).
4. Add a unit test just faking it (hand-craft an object that behaves like request to test with). This satisfies my original goal of adding some code coverage, but it's kind of brittle -- if request ever changes to behave differently, then the test become useless.

[Trott]

I'm actually not sure what next steps would be, possibilities:

1. Do nothing, except perhaps add comments inline so people know what they're looking at?
2. This doesn't belong in core, remove request-specific lines.
3. Add a unit test using an actual request object (node core doesn't require npm packages, so this doesn't make much sense).
4. Add a unit test just faking it (hand-craft an object that behaves like request to test with). This satisfies my original goal of adding some code coverage, but it's kind of brittle -- if request ever changes to behave differently, then the test become useless.

If it helps make the choice easier: request is in maintenance mode.

[ronag]

@elliot-nelson any further progress here?

[ronag]

I'm closing this as it's been stale for a while. Feel free to re-open if work continues.