-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add promise rejection tracking tests #2388
Conversation
Reviewers for this pull request are: @w3c/html-reviewers. |
Critic review: https://critic.hoppipolla.co.uk/r/6015 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
4700911
to
f86b4b7
Compare
p.catch(function() {}); | ||
} catch (e) { | ||
assert_unreached('attaching a handler should not throw an error'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use t.step(function() { p.catch(function() {}); }
instead of try/catch
I pushed a fix for @zcorpan's review and also one for the troublesome issue I mentioned in the OP. The only remaining potentially major item I can see is a global replacement of setTimeout with t.step_timeout. I'm happy to do that if people advise me it's a good idea. My experience is that they have strangely different semantics which scares me a bit. |
Maybe best to ask @jgraham about step_timeout |
Why close and delete my PR? |
|
…and then I screw up and press "close and comment". This isn't our day. :\ |
f9b2eca
to
60cb726
Compare
Chrome (unstable channel)Testing web-platform-tests at revision aeb2f0a891a2904186c614df750478fa9d7643c2 All results9 tests ran/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/allow-crossorigin.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/disallow-crossorigin.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.serviceworker.https.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.dedicatedworker.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-event-constructor.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events-onerror.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.sharedworker.html
/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events-attached-in-event.html
|
Firefox (nightly channel)Testing web-platform-tests at revision aeb2f0a891a2904186c614df750478fa9d7643c2 |
@jeisinger I realize we never landed this... do you have any idea why Chrome does not pass allow-crossorigin.html ? |
|
||
(function() { | ||
var scriptEl = document.createElement('script'); | ||
scriptEl.src = CROSSDOMAIN + 'support/promise-access-control.py?allow=true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add scriptEl.crossOrigin = 'anonymous';
- otherwise CORS won't be used.
|
||
var scriptEl = document.createElement('script'); | ||
scriptEl.src = CROSSDOMAIN + 'support/promise-access-control.py?allow=false'; | ||
scriptEl.onload = resolveLoaded; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
These tests were upstreamed to web-platform-tests in web-platform-tests/wpt#2388. Several Chromium-specific tests remain but the ones deleted in this commit are redundant with the versions now found in third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/. BUG=702025 Review-Url: https://codereview.chromium.org/2798273002 Cr-Commit-Position: refs/heads/master@{#462422}
For whatwg/html#224
Need a few pieces of help:
Running the allow-crossorigin and disallow-crossorigin tests fails for me locally because theworks now, I forgot ?pipe=sub{{ports[html][0]}}
syntax from /cors/support.js is not getting preprocessed. Any ideas?I tried replacing setTimeout withFixedt.step_timeout
in disallow-crossorigin, but this caused the test to pass even when I changed the URL to be same-origin. I think that section of code probably just needs to be done better; review would be helpful.I'd appreciate if review was done on GitHub, not Critic.
These tests were done in collaboration with @jeisinger. Apart from the abovementioned issues, they all pass in Chrome Canary with experimental web platform features enabled.