Skip to content

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

Merged
domenic merged 25 commits intoweb-platform-tests:masterfrom
MattiasBuelens:rs-byob-read-fully
Nov 13, 2023
Merged

Streams: add tests for ReadableStreamBYOBReader.read(view, { min })#29723
domenic merged 25 commits intoweb-platform-tests:masterfrom
MattiasBuelens:rs-byob-read-fully

Conversation

@MattiasBuelens
Copy link
Copy Markdown
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

Comment thread streams/resources/test-utils.js
Comment thread streams/piping/general.any.js
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread streams/readable-byte-streams/read-fully.any.js Outdated
Comment thread streams/readable-byte-streams/read-fully.any.js Outdated
Comment thread streams/readable-byte-streams/read-fully.any.js Outdated
Comment thread streams/readable-byte-streams/read-fully.any.js Outdated
Comment thread streams/readable-byte-streams/read-fully.any.js Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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