Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow manipulating the generator in Duplex.from() #55096

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matthieusieben
Copy link
Contributor

Fix for #55077

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 24, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm with a few nits, good job

lib/internal/streams/duplexify.js Outdated Show resolved Hide resolved
lib/internal/streams/duplexify.js Outdated Show resolved Hide resolved
@mcollina mcollina requested a review from ronag September 24, 2024 11:12
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

test/parallel/test-stream-duplex-from.js Outdated Show resolved Hide resolved
test/parallel/test-stream-duplex-from.js Outdated Show resolved Hide resolved
lib/internal/streams/duplexify.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added the stream Issues and PRs related to the stream subsystem. label Sep 24, 2024
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.25%. Comparing base (3c5ceff) to head (0ec46d7).
Report is 183 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55096      +/-   ##
==========================================
+ Coverage   88.24%   88.25%   +0.01%     
==========================================
  Files         651      651              
  Lines      183877   183960      +83     
  Branches    35858    35874      +16     
==========================================
+ Hits       162266   162361      +95     
+ Misses      14901    14886      -15     
- Partials     6710     6713       +3     
Files with missing lines Coverage Δ
lib/internal/streams/duplexify.js 97.16% <100.00%> (+0.59%) ⬆️

... and 51 files with indirect coverage changes

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Sep 24, 2024

Not sure how to fix the breaking test(s):

test-stream-compose-operator
=== release test-stream-compose-operator ===
Path: parallel/test-stream-compose-operator
node:internal/process/promises:394
    triggerUncaughtException(err, true /* fromPromise */);
    ^

AssertionError [ERR_ASSERTION]: The input did not match the regular expression /boom/. Input:

'AbortError: The operation was aborted'

    at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: AbortError: The operation was aborted
      at destroyer (node:internal/streams/destroy:328:11)
      at node:internal/streams/duplexify:89:7
      at asyncGenerator.return (node:internal/streams/duplexify:245:9)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js:63:5
      at async nextAsync (node:internal/streams/from:182:33) {
    code: 'ABORT_ERR'
  },
  expected: /boom/,
  operator: 'rejects'
}

Node.js v23.0.0-pre
Command: out/Release/node /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js

Fixed thanks to @jazelly 🙏

@ronag
Copy link
Member

ronag commented Sep 25, 2024

@benjamingr This seems super hacky... wdyt?

Co-authored-by: Jason Zhang <xzha4350@gmail.com>
@matthieusieben
Copy link
Contributor Author

@ronag I agree. I don't usually like to monkey patch stuff but in this case I coudn't think of another way...

I tried using a try/catch/finally block inside the async generator to do the same. But since those block don't get called unless an iteration started (next() called at least once), that didn't work.

@@ -401,3 +402,193 @@ function makeATestWritableStream(writeFunc) {
assert.strictEqual(d.writable, false);
}));
}

{
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd really prefer if we could start adding a bit more documentation to each individual test that briefly describes what is expected and what that test is verifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants