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

ReadableStream: Readable byte stream must call pull if needed after receiving new chunk #10018

Conversation

Projects
None yet
4 participants
@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Mar 13, 2018

This adds a test for the new behavior to be introduced by whatwg/streams#904. More tests will follow later.

This also changes two other tests:

  • ReadableStream with byte source: autoAllocateChunkSize
    This test previously expected pull() to only be called once, although read() is called twice. This was incorrect: pull() should have been called again to fulfill the second read(). The fixed test verifies that the second read is correctly fulfilled.
  • ReadableStream with byte source: Respond to pull() by enqueue() asynchronously
    This test previously expected pull() to only be called once, but the amount of data enqueued by one pull() is not sufficient to fill the stream's queue up to its highWaterMark of 256. This is now incorrect: the stream would need to repeatedly call pull until it has reached the highWaterMark. The fixed test sets the highWaterMark to 0 such that pull() does not get called repeatedly.
@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 13, 2018

Build PASSED

Started: 2018-03-15 21:09:33
Finished: 2018-03-15 21:18:11

View more information about this build on:

@ricea

ricea approved these changes Mar 15, 2018

Copy link
Contributor

ricea left a comment

Great quality and coverage, thanks!

let byobRequest;
let viewDefined = false;
let viewInfo;
let byobRequests = [];

This comment has been minimized.

Copy link
@ricea

ricea Mar 15, 2018

Contributor

You can use "const" instead of "let" here.

I write the tests for streams in the same style as the streams repository, as I think it makes them easier to navigate. I have a more relaxed version of the eslint config from whatwg/streams that I use to check my changes. Unfortunately there doesn't seem to be any useful place where I could check that in.

}).then(result => {
assert_equals(pullCount, 2, 'pull() must have been invoked twice');
assert_not_equals(result.value, undefined);

This comment has been minimized.

Copy link
@ricea

ricea Mar 15, 2018

Contributor

If you have the energy, it would be nice to have descriptions for these asserts. But don't worry about it, because it was already like this.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Mar 15, 2018

Author Contributor

Done.


I'm not sure if I like the assert message restating the entire check, as in:

assert_equals(viewInfo.byteOffset, 0, 'first view.byteOffset should be 0');

If the assert fails, you already get the expected value in the error message anyway:

assert_equals: first view.byteOffset should be 0 expected 0 but got 42

Perhaps in these cases, we should just describe the value being tested without repeating the expected value? I kind of like the terseness used in some other tests:

assert_equals(view.byteOffset, 0, 'view.byteOffset');

This comment has been minimized.

Copy link
@ricea

ricea Mar 20, 2018

Contributor

I make a habit of putting the word 'should' in my descriptions as a way of forcing myself to be clear about what I'm asserting. But certainly in cases such as this it doesn't help readability. The important thing is to have a distinct message, so that someone looking at a failure doesn't have to guess which assert failed.

@ricea ricea merged commit 6e1b5f4 into web-platform-tests:master Mar 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MattiasBuelens MattiasBuelens deleted the MattiasBuelens:readable-byte-stream-pull-after-new-chunk branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.