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

Fetch: Add tests for AbortSignal's abort reason #35374

Merged

Conversation

nidhijaju
Copy link
Member

This PR adds test cases to check the functionality of AbortSignal's abort reason when aborting fetch.

See whatwg/fetch#1343 for accompanying spec changes.

@yutakahirano
Copy link
Contributor

The added cases look good. I think we should test that the service worker can observe the fetch abort and see the contents of the abort reason.

@nidhijaju
Copy link
Member Author

The added cases look good. I think we should test that the service worker can observe the fetch abort and see the contents of the abort reason.

I added a new test for this case, so hopefully it's looking better :) PTAL. Thank you!

Copy link
Contributor

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

fetch/api/resources/sw-intercept-abort.js Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor

noamr commented Aug 15, 2022

I'm missing a test that shows that the serialization happens on abort.
I would expect something like a live JS object that increments a counter every time it's accessed. The reason observed by the SW should show a "frozen" version from when the abort took place while other callers would still get the live object.

@nidhijaju
Copy link
Member Author

I'm missing a test that shows that the serialization happens on abort. I would expect something like a live JS object that increments a counter every time it's accessed. The reason observed by the SW should show a "frozen" version from when the abort took place while other callers would still get the live object.

Apologies for the delay! I added a test, but I'm not sure if it's exactly what you were looking for. Would you be able to take a look? Thank you!

annevk pushed a commit to whatwg/fetch that referenced this pull request Oct 5, 2022
This is a follow-up to whatwg/dom#1027 ensuring that the fetch algorithm forwards the abort reason appropriately.

Tests: web-platform-tests/wpt#35374.
@nidhijaju nidhijaju merged commit 0e5f85c into web-platform-tests:master Oct 6, 2022
@nidhijaju nidhijaju deleted the fetch-abortreason-tests branch October 6, 2022 00:48
const controller = new w.AbortController();
const signal = controller.signal;

const fetchPromise = await w.fetch('data.json', { signal });
Copy link
Contributor

@MayyaSunil MayyaSunil Nov 3, 2022

Choose a reason for hiding this comment

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

The test does not produce the required failure messages in the tests

I think the await here should be removed.
The above code will return a response object and not the promise which we need to pass it to promise_rejects_exactly.
The following code should solve the problem:

    const fetchPromise =  w.fetch('data.json', { signal });

Kindly check, I am not a JS expert :)

Copy link
Member

Choose a reason for hiding this comment

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

That seems correct @MayyaSunil. Could you create a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @annevk. I will submit the fix in mozilla's repo and let the sync bot do the job? Is it acceptable to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that works. It might be worth having another test like this whereby we do wait for the response and then check that response.body was errored with the correct exception if that doesn't exist already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug 1798921 is created to track this

Copy link
Contributor

Choose a reason for hiding this comment

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

If the below patch is fine, I will submit it to our repo.

--- a/testing/web-platform/tests/fetch/api/abort/serviceworker-intercepted.https.html
+++ b/testing/web-platform/tests/fetch/api/abort/serviceworker-intercepted.https.html
@@ -114,13 +114,31 @@
     const controller = new w.AbortController();
     const signal = controller.signal;
 
-    const fetchPromise = await w.fetch('data.json', { signal });
+    const fetchPromise = w.fetch('data.json', { signal });
 
     controller.abort(error1);
 
     await promise_rejects_exactly(t, error1, fetchPromise);
   }, "fetch() rejects with abort reason");
 
+
+  promise_test(async t => {
+    const scope = SCOPE + "?q=aborted-with-abort-reason-in-body";
+    await setupRegistration(t, scope, '../resources/sw-intercept.js');
+    const iframe = await with_iframe(scope);
+    add_completion_callback(_ => iframe.remove());
+    const w = iframe.contentWindow;
+
+    const controller = new w.AbortController();
+    const signal = controller.signal;
+
+    const fetchResponse = await w.fetch('data.json', { signal });
+    const bodyPromise  = fetchResponse.body.getReader().read();
+    controller.abort(error1);
+
+    await promise_rejects_exactly(t, error1, bodyPromise);
+    }, "fetch() response body has abort reason");

Copy link
Member

Choose a reason for hiding this comment

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

That looks correct, yeah. Thanks @MayyaSunil!

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.

6 participants