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

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

Merged
merged 11 commits into from
Oct 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d4a347a
add abort reason tests
nidhijaju Aug 8, 2022
75174a9
fix test
nidhijaju Aug 8, 2022
f2f12d1
fix promise rejection
nidhijaju Aug 8, 2022
be63549
add test
nidhijaju Aug 10, 2022
7a3d345
removing logging
nidhijaju Aug 10, 2022
4759700
fix typo
nidhijaju Aug 10, 2022
d02270b
update test
nidhijaju Aug 15, 2022
c52030b
remove waitUntil()
nidhijaju Aug 15, 2022
e26ec20
Merge branch 'web-platform-tests:master' into fetch-abortreason-tests
nidhijaju Aug 24, 2022
acb5c9c
add serialization test
nidhijaju Aug 24, 2022
52f8b23
mutate error
nidhijaju Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions fetch/api/abort/general.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

const BODY_METHODS = ['arrayBuffer', 'blob', 'formData', 'json', 'text'];

const error1 = new Error('error1');
error1.name = 'error1';

// This is used to close connections that weren't correctly closed during the tests,
// otherwise you can end up running out of HTTP connections.
let requestAbortKeys = [];
Expand All @@ -31,6 +34,16 @@ promise_test(async t => {
await promise_rejects_dom(t, "AbortError", fetchPromise);
}, "Aborting rejects with AbortError");

promise_test(async t => {
const controller = new AbortController();
const signal = controller.signal;
controller.abort(error1);

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

await promise_rejects_exactly(t, error1, fetchPromise, 'fetch() should reject with abort reason');
}, "Aborting rejects with abort reason");

promise_test(async t => {
const controller = new AbortController();
const signal = controller.signal;
Expand Down Expand Up @@ -91,6 +104,22 @@ promise_test(async t => {
await promise_rejects_dom(t, "AbortError", fetchPromise);
}, "Signal on request object");

promise_test(async t => {
const controller = new AbortController();
const signal = controller.signal;
controller.abort(error1);

const request = new Request('../resources/data.json', { signal });

assert_not_equals(request.signal, signal, 'Request has a new signal, not a reference');
assert_true(request.signal.aborted, `Request's signal has aborted`);
assert_equals(request.signal.reason, error1, `Request's signal's abort reason is error1`);

const fetchPromise = fetch(request);

await promise_rejects_exactly(t, error1, fetchPromise, "fetch() should reject with abort reason");
}, "Signal on request object should also have abort reason");

promise_test(async t => {
const controller = new AbortController();
const signal = controller.signal;
Expand Down
99 changes: 94 additions & 5 deletions fetch/api/abort/serviceworker-intercepted.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
const SCOPE = '../resources/basic.html';
const BODY_METHODS = ['arrayBuffer', 'blob', 'formData', 'json', 'text'];

async function setupRegistration(t, scope) {
const reg = await navigator.serviceWorker.register('../resources/sw-intercept.js', { scope });
const error1 = new Error('error1');
error1.name = 'error1';

async function setupRegistration(t, scope, service_worker) {
const reg = await navigator.serviceWorker.register(service_worker, { scope });
await wait_for_state(t, reg.installing, 'activated');
add_completion_callback(_ => reg.unregister());
return reg;
Expand All @@ -23,7 +26,7 @@
promise_test(async t => {
const suffix = "?q=aborted-not-intercepted";
const scope = SCOPE + suffix;
await setupRegistration(t, scope);
await setupRegistration(t, scope, '../resources/sw-intercept.js');
const iframe = await with_iframe(scope);
add_completion_callback(_ => iframe.remove());
const w = iframe.contentWindow;
Expand Down Expand Up @@ -56,7 +59,7 @@
for (const bodyMethod of BODY_METHODS) {
promise_test(async t => {
const scope = SCOPE + "?q=aborted-" + bodyMethod + "-rejects";
await setupRegistration(t, scope);
await setupRegistration(t, scope, '../resources/sw-intercept.js');
const iframe = await with_iframe(scope);
add_completion_callback(_ => iframe.remove());
const w = iframe.contentWindow;
Expand Down Expand Up @@ -84,7 +87,7 @@

promise_test(async t => {
const scope = SCOPE + "?q=aborted-stream-errors";
await setupRegistration(t, scope);
await setupRegistration(t, scope, '../resources/sw-intercept.js');
const iframe = await with_iframe(scope);
add_completion_callback(_ => iframe.remove());
const w = iframe.contentWindow;
Expand All @@ -100,6 +103,92 @@
await promise_rejects_dom(t, "AbortError", w.DOMException, reader.read());
await promise_rejects_dom(t, "AbortError", w.DOMException, reader.closed);
}, "Stream errors once aborted.");

promise_test(async t => {
const scope = SCOPE + "?q=aborted-with-abort-reason";
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 fetchPromise = await w.fetch('data.json', { signal });
Copy link
Contributor

@MayyaSunil MayyaSunil Nov 3, 2022 •
edited
Loading

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!


controller.abort(error1);

await promise_rejects_exactly(t, error1, fetchPromise);
}, "fetch() rejects with abort reason");

promise_test(async t => {
const scope = SCOPE + "?q=service-worker-observes-abort-reason";
await setupRegistration(t, scope, '../resources/sw-intercept-abort.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 fetchPromise = w.fetch('data.json', { signal });

await new Promise(resolve => {
w.navigator.serviceWorker.addEventListener('message', t.step_func(event => {
assert_equals(event.data, "fetch event has arrived");
resolve();
}), {once: true});
});

controller.abort(error1);

await new Promise(resolve => {
w.navigator.serviceWorker.addEventListener('message', t.step_func(event => {
assert_equals(event.data.message, error1.message);
resolve();
}), {once: true});
});

await promise_rejects_exactly(t, error1, fetchPromise);
}, "Service Worker can observe the fetch abort and associated abort reason");

promise_test(async t => {
let incrementing_error = new Error('error1');
incrementing_error.name = 'error1';

const scope = SCOPE + "?q=serialization-on-abort";
await setupRegistration(t, scope, '../resources/sw-intercept-abort.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 fetchPromise = w.fetch('data.json', { signal });

await new Promise(resolve => {
w.navigator.serviceWorker.addEventListener('message', t.step_func(event => {
assert_equals(event.data, "fetch event has arrived");
resolve();
}), {once: true});
});

controller.abort(incrementing_error);

const original_error_name = incrementing_error.name;

incrementing_error.name = 'error2';

await new Promise(resolve => {
w.navigator.serviceWorker.addEventListener('message', t.step_func(event => {
assert_equals(event.data.name, original_error_name);
resolve();
}), {once: true});
});

await promise_rejects_exactly(t, incrementing_error, fetchPromise);
}, "Abort reason serialization happens on abort");
</script>
</body>
</html>
19 changes: 19 additions & 0 deletions fetch/api/resources/sw-intercept-abort.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
async function messageClient(clientId, message) {
const client = await clients.get(clientId);
client.postMessage(message);
}

addEventListener('fetch', event => {
let resolve;
const promise = new Promise(r => resolve = r);

function onAborted() {
messageClient(event.clientId, event.request.signal.reason);
resolve();
}

messageClient(event.clientId, 'fetch event has arrived');

event.respondWith(promise.then(() => new Response('hello')));
event.request.signal.addEventListener('abort', onAborted);
});