X Tutup
The Wayback Machine - https://web.archive.org/web/20220517174315/https://github.com/nodejs/node/pull/42823
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

test: improve lib/internal/webstreams/readablestream.js coverage #42823

Conversation

@nodejs-github-bot nodejs-github-bot added needs-ci test labels Apr 22, 2022
@@ -209,9 +212,11 @@ class Source {
reader.releaseLock();
assert.rejects(reader.read(new Uint8Array(10)), {
code: 'ERR_INVALID_STATE',
message: 'Invalid state: The reader is not attached to a stream',
Copy link
Contributor

@aduh95 aduh95 Apr 22, 2022

Choose a reason for hiding this comment

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

We should not test error messages, see #42813

Suggested change
message: 'Invalid state: The reader is not attached to a stream',

Copy link
Contributor Author

@fossamagna fossamagna Apr 22, 2022

Choose a reason for hiding this comment

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

@aduh95 Thanks, I change that checking only the code property.

@fossamagna fossamagna requested a review from aduh95 Apr 22, 2022
readableStreamReaderGenericRelease(stream[kState].reader);
assert.rejects(iterable.next(), {
code: 'ERR_INVALID_STATE',
});
Copy link
Contributor

@aduh95 aduh95 Apr 22, 2022

Choose a reason for hiding this comment

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

Suggested change
});
}).then(common.mustCall());

readableStreamReaderGenericRelease(stream[kState].reader);
assert.rejects(iterable.return(), {
code: 'ERR_INVALID_STATE',
});
Copy link
Contributor

@aduh95 aduh95 Apr 22, 2022

Choose a reason for hiding this comment

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

Suggested change
});
}).then(common.mustCall());

@fossamagna fossamagna requested a review from aduh95 Apr 22, 2022
aduh95
aduh95 approved these changes Apr 22, 2022
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@mhdawson mhdawson added request-ci author ready labels May 2, 2022
@github-actions github-actions bot removed the request-ci label May 2, 2022
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 2, 2022

@aduh95 aduh95 added commit-queue commit-queue-squash labels May 5, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue label May 5, 2022
@nodejs-github-bot nodejs-github-bot merged commit 277353c into nodejs:master May 5, 2022
55 checks passed
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 5, 2022

Landed in 277353c

RafaelGSS pushed a commit that referenced this issue May 10, 2022
PR-URL: #42823
Refs: https://coverage.nodejs.org/coverage-3a6b975981092213/lib/internal/webstreams/readablestream.js.html#L421
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@RafaelGSS RafaelGSS mentioned this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready commit-queue-squash needs-ci test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants
X Tutup