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

Streams: add tests for ReadableStreamBYOBReader.read(view, { min }) #29723

Merged
merged 25 commits into from
Nov 13, 2023

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Jul 20, 2021

Add tests for the new optional min parameter in ReadableStreamBYOBReader.read(view, { min }).

See whatwg/streams#1145 for the accompanying spec change.

To do:

  • Write more tests for 0 < min <= view.length

assert_equals(pullCount, 2, 'pull() must only be called 2 times');
assert_false(result3.done, 'branch2 second read() should not be done');
assert_typed_array_equals(result3.value, new Uint8Array([0x02, 0x03]), 'branch2 second read() value');
}, 'ReadableStream with byte source: tee() with readFully() from branch1 and read() from branch2');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

All tests LGTM, be also need tests for the early-error cases, e.g. detached buffer, 0-length view/buffer, etc.

Maybe run the coverage tool and see what it spits out?

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

Looks good. I can't see any obvious gaps. Just a few nits.

streams/readable-byte-streams/read-fully.any.js Outdated Show resolved Hide resolved
streams/readable-byte-streams/read-fully.any.js Outdated Show resolved Hide resolved
streams/readable-byte-streams/read-fully.any.js Outdated Show resolved Hide resolved
streams/readable-byte-streams/read-fully.any.js Outdated Show resolved Hide resolved
streams/readable-byte-streams/read-fully.any.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

@MattiasBuelens MattiasBuelens marked this pull request as ready for review August 19, 2021 21:42
@MattiasBuelens
Copy link
Contributor Author

Added a couple more tests, coverage should be good now. 🙂

@MattiasBuelens MattiasBuelens changed the title Streams: add tests for ReadableStreamBYOBReader.readFully() Streams: add tests for ReadableStreamBYOBReader.fill(view) Aug 19, 2021
@ricea
Copy link
Contributor

ricea commented Aug 19, 2021

Still lgtm.

@MattiasBuelens MattiasBuelens force-pushed the rs-byob-read-fully branch 2 times, most recently from 060a1ab to a684ebd Compare January 18, 2022 22:28
@MattiasBuelens MattiasBuelens changed the title Streams: add tests for ReadableStreamBYOBReader.fill(view) Streams: add tests for ReadableStreamBYOBReader.read(view, { atLeast }) Apr 12, 2022
@ricea
Copy link
Contributor

ricea commented Apr 13, 2022

Still lgtm.

@MattiasBuelens MattiasBuelens changed the title Streams: add tests for ReadableStreamBYOBReader.read(view, { atLeast }) Streams: add tests for ReadableStreamBYOBReader.read(view, { min }) Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants