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 Transformer.cancel #40453

Merged

Conversation

lucacasonato
Copy link
Member

@@ -388,9 +388,24 @@ promise_test(t => {
controller.terminate();
return Promise.all([
cancelPromise,
promise_rejects_exactly(t, cancelReason, ts.writable.getWriter().closed, 'closed should reject with cancelReason')
promise_rejects_js(t, TypeError, ts.writable.getWriter().closed, 'closed should reject with TypeError')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see. Since TransformStreamDefaultSourceCancelAlgorithm takes at least one microtask to run transformer.cancel() (even if it does not exist), the TypeError from terminate() now wins the race. 🤔

I suppose that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

hello @MattiasBuelens i am trying to implement this for node this test presently fails my knowledge of WPTs is little limited hence asking here so is cancelPromise also expected to throw in this test? I am seeing that running this test standalone throws TypeError but unable to understand why the WPT says this uncaught

Copy link
Member Author

Choose a reason for hiding this comment

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

This test will fail if you have not implanted the changes from the streams spec PR (yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

This test passes against the reference implementation, so if it fails it may be a bug in your implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a bug no doubt i just wnated to know this test is basically equalt to the following script right:

import { TransformStream } from 'stream/web';

let controller;
const ts = new TransformStream({
  start(c) {
    controller = c;
  }
});
const cancelReason = { name: 'cancelReason' };
const cancelPromise = ts.readable.cancel(cancelReason);
controller.terminate();

try {
  await Promise.all([cancelPromise, ts.writable.getWriter().closed]);
} catch (err) {
  console.log(err); // TypeError!
}

Copy link
Contributor

@MattiasBuelens MattiasBuelens Oct 2, 2023

Choose a reason for hiding this comment

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

@debadree25 Yes, that looks script right. Ideally, you want to assert that the TypeError originates from .closed and not from cancelPromise, so in Node you could use assert.rejects():

import assert from 'node:assert/strict';

await Promise.all([
  cancelPromise,
  assert.rejects(ts.writable.getWriter().closed, TypeError)
]);

so is cancelPromise also expected to throw in this test?

No, cancelPromise should always resolve.

The only change is that writer.closed now becomes errored by controller.terminate(), whereas previously it would be errored by ts.readable.cancel(cancelReason). This is because readable.cancel() now needs to go through the (possibly asynchronous) transformer.cancel() method first before it can error the writable, whereas controller.terminate() always errors the writable synchronously (without going through a transformer method).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much @MattiasBuelens for the detail response i guess i have found the issue with my implementation!

@domenic domenic marked this pull request as ready for review September 29, 2023 04:53
@domenic
Copy link
Member

domenic commented Sep 29, 2023

I guess since this was marked as draft, I shouldn't go ahead and merge it immediately. @lucacasonato, can you confirm this is up-to-date with all the latest changes in whatwg/streams#1283, and is ready to merge?

(I took it out of draft state anyway so we could see CI results. If those fail, the first thing to try is rebasing; WPT CI is very finicky and constantly getting patched.)

@lucacasonato
Copy link
Member Author

@domenic Yes, this is up to date.

@domenic
Copy link
Member

domenic commented Sep 29, 2023

Looks like we have a lint failure due to trailing whitespace.

@lucacasonato lucacasonato merged commit a8872d9 into web-platform-tests:master Sep 29, 2023
19 checks passed
@lucacasonato lucacasonato deleted the TransformStream_cancel branch September 29, 2023 07:35
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
Co-authored-by: Mattias Buelens <mattias@buelens.com>
Co-authored-by: Domenic Denicola <d@domenic.me>
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