-
Notifications
You must be signed in to change notification settings - Fork 464
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 tests for Proxy semantics change #2143
Conversation
f51de43
to
f5cd4f9
Compare
104e595
to
f150ac5
Compare
f150ac5
to
9fa9fc0
Compare
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 PR is good, I just left a few comments with minor things that can be addressed here and there.
test/built-ins/Proxy/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.js
Outdated
Show resolved
Hide resolved
test/built-ins/Proxy/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.js
Show resolved
Hide resolved
test/built-ins/Proxy/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.js
Outdated
Show resolved
Hide resolved
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js
Outdated
Show resolved
Hide resolved
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js
Outdated
Show resolved
Hide resolved
...tOwnPropertyDescriptor/resultdesc-is-not-configurable-not-writable-targetdesc-is-writable.js
Outdated
Show resolved
Hide resolved
...tOwnPropertyDescriptor/resultdesc-is-not-configurable-not-writable-targetdesc-is-writable.js
Outdated
Show resolved
Hide resolved
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js
Show resolved
Hide resolved
63026bb
to
17ed315
Compare
Thanks for detailed feedback, @leobalter. Instead of duplicating tests, I've moved property definitions to traps just like we have in some other tests. Except for asserting that |
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 tests are looking great, I left 2 comments that should be easy to address. Thanks for the hard work here!
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js
Outdated
Show resolved
Hide resolved
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js
Outdated
Show resolved
Hide resolved
These tests are great! Thanks! |
Spec PR: tc39/ecma262#666.
Closes #2142.