-
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
[webidl] Assert prototype immutability #5788
[webidl] Assert prototype immutability #5788
Conversation
We already have extensive tests for this (which cover more cases). Search for setPrototypeOf across all files to see. |
Firefox (nightly channel)Testing web-platform-tests at revision 7836ffe |
Chrome (unstable channel)Testing web-platform-tests at revision 7836ffe |
It looks like that method is referenced in the following files:
Maybe a little more instructive is searching for the
...but even here, it seems as though we're missing coverage: ServiceWorkerGlobalScope, DedicatedWorkerGlobalScope, SharedWorkerGlobalScope, and WorkerGlobalScope. There may be others today, and there's always the possibility for more being defined in the future. Part of the motivation for maintaining |
Sure, that might work, although my impression was idlharness.js tests didn't work in workers (and that's why service worker tests have their own IDL testing framework). |
I've been vetting this change with |
OK. Well in general I'd prefer you use the same tests as test-setting-immutable-prototype.js, either through refactoring or duplication, instead of the set in this PR (which does strange things like try/catch and then assert the catch wasn't hit, instead of just letting the exception cause test failures). |
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.
Marking as required changes per my last comment
Hi @domenic, I'm just now getting back to this. Although it's my preference to avoid duplication wherever possible, I'm not sure if re-use is an option here. That's because I've taken your second suggestion and duplicated the relevant assertions. What do you think? |
Firefox (nightly)Testing web-platform-tests at revision 89aead7 |
Sauce (safari)Testing web-platform-tests at revision 65218bb |
Chrome (unstable)Testing web-platform-tests at revision 65218bb |
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 65218bb |
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 test name changes
resources/idlharness.js
Outdated
"original value not modified" | ||
); | ||
}.bind(this), this.name + " interface: internal [[SetPrototypeOf]] method " + | ||
"of named properties object - setting to a new value via Object.setPrototypeOf " + |
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 isn't testing named properties objects, just global objects.
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.
The referenced specification text applies only to named properties objects. I think that means the surrounding condition needs to change, not the description. Would you agree?
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.
No... I thought you were referencing https://heycam.github.io/webidl/#platform-object-setprototypeof
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.
Named properties objects only apply to Window, so shouldn't be tested in idlharness.
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.
Got it. Looks like the spec reference needs updating, too
@domenic I've pushed up another commit to address your latest feedback. |
@domenic Would you feel comfortable merging this on my behalf? Or would you like me to get another review first? |
Oh, I thought you had merge privileges. I'm happy to do so. |
This change is