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

Add WPTs for parse error handling in SharedWorkers #22185

Merged
merged 4 commits into from
Mar 23, 2020
+79 −0
Merged

Add WPTs for parse error handling in SharedWorkers #22185

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
482bc63
shared-worker-parse-error-failure.html
Mar 11, 2020
c97fe3d
Parse error handling in shared workers
Mar 13, 2020
06d8c42
separate files
Mar 23, 2020
22d700a
minor change
Mar 23, 2020
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
59 changes: 59 additions & 0 deletions workers/modules/shared-worker-parse-error-failure.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<!DOCTYPE html>
<title>SharedWorker: parse error failure</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>

// Check if module shared worker is supported.
// In this test scope, we only use simple non-nested static import as a feature
// of module shared worker, so we only check if static import is supported.
//
// This check is necessary to appropriately test parse error handling because
// we need to distingusih the parse error invoked by unsupported "import" in
// the top-level script from the parse error invoked by the statically imported
// script which is the one we want to check in this test.
promise_setup(async () => {
const scriptURL = 'resources/static-import-worker.js';
const worker = new SharedWorker(scriptURL, { type: 'module' });
const supportsModuleWorkers = await new Promise((resolve, reject) => {
worker.port.onmessage = e => {
resolve(e.data.length == 1 && e.data[0] == 'export-on-load-script.js');
};
worker.onerror = () => reject(false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
worker.onerror = () => reject(false);
worker.onerror = () => resolve(false);

});
assert_precondition(
supportsModuleWorkers,
"Static import must be supported on module shared worker to run this test."
);
});

const checkArguments = (args) => {
Copy link
Member

Choose a reason for hiding this comment

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

To deduplicate this you may want to move it to a file in workers/resources, change it to window.checkErrorArguments = ..., and then use e.g. <script src="resources/check-error-arguments.js"></script> to get the function in both files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created workers/support/check-error-arguments.js.

assert_equals(args.a.constructor, Event);
assert_true('message' in args.a, 'Event.message');
assert_equals(typeof args.a.message, 'string', 'Event.message');
assert_equals(args.a.filename, location.href + '/resources/syntax-error.js'),
'Event.filename');
assert_equals(args.a.lineno, 1, 'Event.lineno');
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. Event does not have messsage, filename, or lineno properties. Does this test somehow pass in Chrome?

I'd expectt the test to check that these properties do not exist...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't understand the difference between Event and ErrorEvent.
Chromium doesn't have this message, filename or lineno value for shared workers but has on dedicated workers.
I removed it.

assert_equals(args.b, undefined, 'unexpected second argument to onerror');
assert_equals(args.c, undefined, 'unexpected third argument to onerror');
};

promise_test(async () => {
const scriptURL = 'resources/syntax-error.js';
const worker = new SharedWorker(scriptURL, { type: 'module' });
const args = await new Promise(resolve =>
worker.onerror = (a, b, c) => resolve({a, b, c}));
Copy link
Member

Choose a reason for hiding this comment

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

This could be a little simpler as

  const args = await new Promise(resolve => worker.onerror = (...args) => resolve(args));

and then args will be an array, which avoids the awkward a/b/c properties and lets the checkArguments function do something like assert_equals(args.length, 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Modified.

checkArguments(args);
}, 'Module shared worker construction for script with syntax error should ' +
'dispatch an event named error.');

promise_test(async () => {
const scriptURL = 'resources/static-import-syntax-error.js';
const worker = new SharedWorker(scriptURL, { type: 'module' });
const args = await new Promise(resolve =>
worker.onerror = (a, b, c) => resolve({a, b, c}));
checkArguments(args);
}, 'Static import on module shared worker for script with syntax error ' +
'should dispatch an event named error.');

</script>
36 changes: 36 additions & 0 deletions workers/shared-worker-parse-error-failure.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<!DOCTYPE html>
<title>SharedWorker: parse error failure</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>

const checkArguments = (args) => {
assert_equals(args.a.constructor, Event);
assert_true('message' in args.a, 'Event.message');
assert_equals(typeof args.a.message, 'string', 'Event.message');
assert_equals(args.a.filename, location.href + '/resources/syntax-error.js',
'Event.filename');
assert_equals(args.a.lineno, 1, 'Event.lineno');
assert_equals(args.b, undefined, 'unexpected second argument to onerror');
assert_equals(args.c, undefined, 'unexpected third argument to onerror');
};

promise_test(async () => {
domenic marked this conversation as resolved.
Show resolved Hide resolved
const scriptURL = 'modules/resources/syntax-error.js';
const worker = new SharedWorker(scriptURL, { type: 'classic' });
const args = await new Promise(resolve =>
worker.onerror = (a, b, c) => resolve({a, b, c}));
checkArguments(args);
}, 'Classic shared worker construction for script with syntax error should ' +
'dispatch an event named error.');

promise_test(async () => {
const scriptURL = 'modules/resources/static-import-worker.js';
const worker = new SharedWorker(scriptURL, { type: 'classic' });
const args = await new Promise(resolve =>
worker.onerror = (a, b, c) => resolve({a, b, c}));
checkArguments(args);
}, 'Static import on classic shared worker should dispatch an event named ' +
'error.');

</script>