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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Mar 20, 2018
Merged

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

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
559b827
Readable byte streams: fix test that allows too much pulling
MattiasBuelens Mar 8, 2018
9dc1d0d
Readable byte streams: test that byte stream pulls after enqueue
MattiasBuelens Mar 8, 2018
0518cdc
Readable byte streams: autoAllocateChunkSize test must not ignore sec…
MattiasBuelens Mar 8, 2018
0d05e1e
Use const
MattiasBuelens Mar 15, 2018
b582b51
Extract variable
MattiasBuelens Mar 15, 2018
fbd0b72
Add assert messages
MattiasBuelens Mar 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
121 changes: 99 additions & 22 deletions streams/readable-byte-streams/general.js
View file
Open in desktop
Expand Up @@ -300,22 +300,28 @@ function extractViewInfo(view) {
promise_test(() => {
let pullCount = 0;
let controller;
let byobRequest;
let viewDefined = false;
let viewInfo;
let byobRequests = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


const stream = new ReadableStream({
start(c) {
controller = c;
},
pull() {
byobRequest = controller.byobRequest;
const byobRequest = controller.byobRequest;
const view = byobRequest.view;
viewDefined = view !== undefined;
viewInfo = extractViewInfo(view);

view[0] = 0x01;
byobRequest.respond(1);
byobRequests[pullCount] = {
defined: byobRequest !== undefined,
viewDefined: view !== undefined,
viewInfo: extractViewInfo(view)
};
if (pullCount === 0) {
view[0] = 0x01;
byobRequest.respond(1);
} else if (pullCount === 1) {
view[0] = 0x02;
view[1] = 0x03;
byobRequest.respond(2);
}

++pullCount;
},
Expand All @@ -326,28 +332,50 @@ promise_test(() => {
});

const reader = stream.getReader();
const readPromise = reader.read();
const ignoredReadPromise = reader.read();
const p0 = reader.read();
const p1 = reader.read();

assert_equals(pullCount, 0, 'No pull() as start() just finished and is not yet reflected to the state of the stream');

return Promise.resolve().then(() => {
assert_equals(pullCount, 1, 'pull() must have been invoked once');
assert_not_equals(byobRequest, undefined, 'byobRequest must not be undefined');
assert_true(viewDefined, 'byobRequest.view must not be undefined');
assert_equals(viewInfo.constructor, Uint8Array, 'view.constructor should be Uint8Array');
assert_equals(viewInfo.bufferByteLength, 16, 'view.buffer.byteLength should be 16');
assert_equals(viewInfo.byteOffset, 0, 'view.byteOffset should be 0');
assert_equals(viewInfo.byteLength, 16, 'view.byteLength should be 16');
return readPromise;
const byobRequest = byobRequests[0];
assert_true(byobRequest.defined, 'first byobRequest must not be undefined');
assert_true(byobRequest.viewDefined, 'first byobRequest.view must not be undefined');
const viewInfo = byobRequest.viewInfo;
assert_equals(viewInfo.constructor, Uint8Array, 'first view.constructor should be Uint8Array');
assert_equals(viewInfo.bufferByteLength, 16, 'first view.buffer.byteLength should be 16');
assert_equals(viewInfo.byteOffset, 0, 'first view.byteOffset should be 0');
assert_equals(viewInfo.byteLength, 16, 'first view.byteLength should be 16');

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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');

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

assert_equals(result.value.constructor, Uint8Array);
assert_equals(result.value.buffer.byteLength, 16);
assert_equals(result.value.byteOffset, 0);
assert_equals(result.value.byteLength, 1);
assert_equals(result.value[0], 0x01);
assert_equals(pullCount, 1, 'pull() should only be invoked once');
const byobRequest = byobRequests[1];
assert_true(byobRequest.defined, 'second byobRequest must not be undefined');
assert_true(byobRequest.viewDefined, 'second byobRequest.view must not be undefined');
const viewInfo = byobRequest.viewInfo;
assert_equals(viewInfo.constructor, Uint8Array, 'second view.constructor should be Uint8Array');
assert_equals(viewInfo.bufferByteLength, 16, 'second view.buffer.byteLength should be 16');
assert_equals(viewInfo.byteOffset, 0, 'second view.byteOffset should be 0');
assert_equals(viewInfo.byteLength, 16, 'second view.byteLength should be 16');

return p1;
}).then(result => {
assert_equals(pullCount, 2, 'pull() should only be invoked twice');
assert_not_equals(result.value, undefined);
assert_equals(result.value.constructor, Uint8Array);
assert_equals(result.value.buffer.byteLength, 16);
assert_equals(result.value.byteOffset, 0);
assert_equals(result.value.byteLength, 2);
assert_equals(result.value[0], 0x02);
assert_equals(result.value[1], 0x03);
});
}, 'ReadableStream with byte source: autoAllocateChunkSize');

Expand Down Expand Up @@ -693,7 +721,7 @@ promise_test(() => {
},
type: 'bytes'
}, {
highWaterMark: 256
highWaterMark: 0
});

const reader = stream.getReader();
Expand All @@ -710,6 +738,55 @@ promise_test(() => {
return Promise.all([p0, p1, p2]).then(result => {
assert_equals(pullCount, 1, 'pullCount after completion of all read()s');

assert_equals(result[0].done, false, 'result[0].done');
assert_equals(result[0].value.byteLength, 1, 'result[0].value.byteLength');
assert_equals(result[1].done, false, 'result[1].done');
assert_equals(result[1].value.byteLength, 1, 'result[1].value.byteLength');
assert_equals(result[2].done, false, 'result[2].done');
assert_equals(result[2].value.byteLength, 1, 'result[2].value.byteLength');
assert_equals(byobRequest, undefined, 'byobRequest should be undefined');
assert_equals(desiredSizes[0], 0, 'desiredSize on pull should be 0');
assert_equals(desiredSizes[1], 0, 'desiredSize after 1st enqueue() should be 0');
assert_equals(desiredSizes[2], 0, 'desiredSize after 2nd enqueue() should be 0');
assert_equals(pullCount, 1, 'pull() should only be called once');
});
}, 'ReadableStream with byte source: Respond to pull() by enqueue() asynchronously');

promise_test(() => {
let pullCount = 0;

let byobRequest;
const desiredSizes = [];

const stream = new ReadableStream({
pull(c) {
byobRequest = c.byobRequest;
desiredSizes.push(c.desiredSize);

if (pullCount < 3) {
c.enqueue(new Uint8Array(1));
} else {
c.close();
}

++pullCount;
},
type: 'bytes'
}, {
highWaterMark: 256
});

const reader = stream.getReader();

const p0 = reader.read();
const p1 = reader.read();
const p2 = reader.read();

assert_equals(pullCount, 0, 'No pull as start() just finished and is not yet reflected to the state of the stream');

return Promise.all([p0, p1, p2]).then(result => {
assert_equals(pullCount, 4, 'pullCount after completion of all read()s');

assert_equals(result[0].done, false, 'result[0].done');
assert_equals(result[0].value.byteLength, 1, 'result[0].value.byteLength');
assert_equals(result[1].done, false, 'result[1].done');
Expand All @@ -720,9 +797,9 @@ promise_test(() => {
assert_equals(desiredSizes[0], 256, 'desiredSize on pull should be 256');
assert_equals(desiredSizes[1], 256, 'desiredSize after 1st enqueue() should be 256');
assert_equals(desiredSizes[2], 256, 'desiredSize after 2nd enqueue() should be 256');
assert_equals(pullCount, 1, 'pull() should only be called once');
assert_equals(desiredSizes[3], 256, 'desiredSize after 3rd enqueue() should be 256');
});
}, 'ReadableStream with byte source: Respond to pull() by enqueue() asynchronously');
}, 'ReadableStream with byte source: Respond to multiple pull() by separate enqueue()');

promise_test(() => {
let controller;
Expand Down