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: tests for teeing readable byte streams #29190

Merged
merged 29 commits into from
Jul 8, 2021

Conversation

MattiasBuelens
Copy link
Contributor

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

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.

Generally looks good and quite exhaustive; impressive as always.

Were you able to check coverage on the reference implementation?

streams/readable-byte-streams/tee.any.js Outdated Show resolved Hide resolved
streams/readable-byte-streams/tee.any.js Show resolved Hide resolved
streams/readable-byte-streams/tee.any.js Outdated Show resolved Hide resolved
@MattiasBuelens
Copy link
Contributor Author

Were you able to check coverage on the reference implementation?

Riiiight, we have a script for that! 😅 I haven't checked it yet, I'll have a look.

@MattiasBuelens
Copy link
Contributor Author

Looks like npm run coverage is broken in the reference implementation. 😬

Istanbul works by hooking into require(). Previously, we'd just require() the reference implementation and expose that to jsdom's window in the test runner. But nowadays we bundle the whole thing with Browserify first and then eval the bundle to get it in jsdom's window. Since we're no longer using require(), Istanbul doesn't process any of our code and can't report any coverage.

Not sure if/how we want to fix this? 😕

@MattiasBuelens
Copy link
Contributor Author

Right, managed to fix the coverage script. Some cases aren't tested yet:

  • Pull with a BYOB reader, then pull with a default reader (should swap the internal reader)
  • Pull with a default reader, then close the stream while the other branch has a pending BYOB read (should call respond(0))
  • Pull with a default reader, then error the stream
  • Pull with a BYOB reader, then close the stream while the other branch has a pending BYOB read (should call respond(0))
  • Pull multiple times with one reader (should hit the if (reading === true) check in the pull algorithms)

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.

LGTM. Not sure why the bots are failing; try rebasing?

@ricea feel free to take a look

@domenic domenic merged commit 87a4c80 into web-platform-tests:master Jul 8, 2021
@MattiasBuelens MattiasBuelens deleted the tee-byte-stream branch July 8, 2021 20:53
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.

3 participants