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
Fix tests for error type of __proto__ on cross-domain objects #5163
Conversation
This is a follow-up to #5015 which corrects a problem noted by Boris in https://bugzilla.mozilla.org/show_bug.cgi?id=1347706#c2.
Notifying @ayg, @deniak, @dontcallmedom, @gsnedders, @jdm, @jgraham, @zcorpan, and @zqzhang. (Learn how reviewing works.) |
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.
LGTM with the nit.
test(() => { | ||
assert_throws(new TypeError, () => { | ||
Object.setPrototypeOf(target, newValue); | ||
}); | ||
}, `${prefix}: setting the prototype to ${newValueString} via Object.setPrototypeOf should throw a TypeError`); | ||
|
||
const dunderProtoError = isSameOriginDomain ? { name: "TypeError" } : { name: "SecurityError" }; |
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.
This won't check for the "SecurityError" being a DOMException. How about:
const dunderProtoError = isSameOriginDomain ? new TypeError() : "SecurityError";
?
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.
Ah I didn't realize. Fixed, PTAL.
target.__proto__ = newValue; | ||
}); | ||
}, `${prefix}: setting the prototype to ${newValueString} via __proto__ should throw a TypeError`); | ||
}, `${prefix}: setting the prototype to ${newValueString} via __proto__ should throw a ${dunderProtoError.name}`); |
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.
I guess that makes this last bit harder, but we could just stash the string we care about in another variable to use here.
Firefox (nightly channel)Testing web-platform-tests at revision 1213ada All results10 tests ran/html/browsers/history/the-location-interface/location-prototype-setting-cross-origin-domain.sub.html
/html/browsers/history/the-location-interface/location-prototype-setting-cross-origin.sub.html
/html/browsers/history/the-location-interface/location-prototype-setting-goes-cross-origin-domain.sub.html
/html/browsers/history/the-location-interface/location-prototype-setting-same-origin-domain.sub.html
/html/browsers/history/the-location-interface/location-prototype-setting-same-origin.html
/html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-cross-origin-domain.sub.html
/html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-cross-origin.sub.html
/html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-goes-cross-origin-domain.sub.html
/html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-same-origin-domain.sub.html
/html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-same-origin.html
|
Chrome (unstable channel)Testing web-platform-tests at revision 1213ada |
These tests are now available on w3c-test.org |
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.
Looks great, thank you!
This is a follow-up to #5015 which corrects a problem noted by Boris in https://bugzilla.mozilla.org/show_bug.cgi?id=1347706#c2.