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: update tests for Web IDL conversion #22982

Merged
merged 10 commits into from
Jun 11, 2020
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 15, 2020

Follows whatwg/streams#1035. Notable changes:

  • Updates for the normative changes listed there.
  • Introduce an idlharness test
  • Remove various tests for things that are covered by idlharness, such as brand checks, the order of getting values from dictionaries, etc.
  • Updated for the fact that everything is now globally exposed, so some roundabout code to get constructors can be removed.
  • Slight timing updates: the pattern of returning a promise from an underlying sink/source/transformer start() function, then waiting on that before doing asserts, does not work with Web IDL's "promise resolved with". So instead we use flushAsyncEvents() to wait a little longer.
  • Consolidated queuing strategy tests into a single file, since after deleting the things covered by idlharness, most of the tests are shared.
  • Added tests that underlyingSink/underlyingSource are converted after queuingStrategy, since those dictionary conversions are done in prose.
  • Added a test for various updates to the Web IDL async iterators specification

@domenic domenic requested a review from ricea April 15, 2020 22:37
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 15, 2020 22:44 Inactive
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

The removed tests will be done by idlharness.js, so we definitely don't want to keep them.

As you say, it would be good to verify that the second argument is unpacked before the second, but we don't need all this infrastructure to do it, just something like

assert_throws_exact(error2, 
    () => new ReadableStream({ get start() { throw error1; } }, { get size() { throw error2; } }));

should do it.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 16, 2020 15:45 Inactive
@MattiasBuelens MattiasBuelens changed the title Steams: update tests for Web IDL conversion Streams: update tests for Web IDL conversion Apr 17, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 21, 2020 17:48 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 22, 2020 16:56 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 22, 2020 17:14 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 April 29, 2020 19:22 Inactive
@ricea
Copy link
Contributor

ricea commented May 1, 2020

Everything still lgtm. I don't know what to do about ReadableStreamBYOBRequest. Could we just call idl_test() asynchronously after grabbing one?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 May 1, 2020 09:40 Inactive
@domenic domenic force-pushed the streams-webidl-updates branch from 7677c83 to 2c5ebb9 Compare May 5, 2020 18:57
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 May 5, 2020 19:03 Inactive
@domenic domenic force-pushed the streams-webidl-updates branch from 2c5ebb9 to 3008aae Compare May 5, 2020 19:22
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 May 5, 2020 19:29 Inactive
@LukeZielinski
Copy link
Contributor

Looks like TaskCluster didn't run here, retrying.

@stephenmcgruer
Copy link
Contributor

Looks like TaskCluster didn't run here, retrying.

This is a draft PR, TaskCluster doesn't run on drafts.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 May 18, 2020 22:31 Inactive
@domenic domenic marked this pull request as ready for review May 18, 2020 22:48
@foolip foolip closed this May 19, 2020
@foolip foolip reopened this May 19, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 May 26, 2020 15:37 Inactive
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.

I re-reviewed everything. Still lgtm.

@ricea
Copy link
Contributor

ricea commented Jun 3, 2020

Updates lgtm.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 June 5, 2020 21:14 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-22982 June 5, 2020 21:48 Inactive
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

The recent additions around next/return interactions look good, modulo one nit. Also, would it be a good idea to have an assert_iteration_result(), perhaps for future?

streams/readable-streams/async-iterator.any.js Outdated Show resolved Hide resolved
@domenic domenic merged commit 887350c into master Jun 11, 2020
@domenic domenic deleted the streams-webidl-updates branch June 11, 2020 15:27
domenic pushed a commit that referenced this pull request Jun 23, 2020
The new idlharness tests for Streams (#22982) did not correctly instantiate all tested interfaces. This PR fixes them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants