-
Notifications
You must be signed in to change notification settings - Fork 468
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
More set methods tests #3966
More set methods tests #3966
Conversation
It would be appreciated if these tests are reviewed and landed as soon as possible, so we will be able to ship set methods on Chrome. Thanks! @tc39/test262-maintainers |
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've reviewed the difference
folder; all seems good in there and I'd be happy to land those. One thing I don't think I've seen is a check for the +0/-0 step.
I don't know when I'll have time for the rest of this PR.
Nice catch, added. I had it for the other three non-predicate methods; don't know why I missed it for |
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've made it most of the way through the intersection/
folder; here are the comments I have so far. Thanks for working on this!
test/built-ins/Set/prototype/intersection/require-internal-slot.js
Outdated
Show resolved
Hide resolved
|
||
let observedOrder = []; | ||
|
||
function observableIterator() { |
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.
Here's where I wish we had already expanded the Observable utilities in harness/temporalHelpers.js
into their own, more general, helper file ... oh well, next time 😄
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.
Finished reviewing the intersection
tests. Thanks for working on this!
test/built-ins/Set/prototype/difference/require-internal-slot.js
Outdated
Show resolved
Hide resolved
test/built-ins/Set/prototype/difference/require-internal-slot.js
Outdated
Show resolved
Hide resolved
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've confirmed all the non-class
tests in my polyfills for:
- difference
- intersection
- isDisjointFrom
- isSubsetOf
Also, isDisjointFrom and isSubsetOf are missing a test of sets with 0
and -0
(the -0 test on other methods caught a bug, so i checked for it). It's also possible that https://tc39.es/proposal-set-methods/#sec-set.prototype.isdisjointfrom itself is missing an explicit If nextValue is -0𝔽, set nextValue to +0𝔽.
step after 6.c.ii.1?
I'll post this review now, and will continue reviewing the remaining 4 methods, but since this might be a spec bug I wanted to submit it asap.
That's covered by SetDataHas (invoked in 6.c.ii.2) using SameValueZero. |
Yep, you're right; it was my incorrect optimization. I think the test cases should still be added but I agree the proposal is correct as-is :-) |
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.
rest of the methods look good
@ljharb Fixed the isSupersetOf/set-like-array.js bug and added the |
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've reviewed all the tests that weren't already reviewed by someone else, specifically *class*.js
.
There are a couple of things in isDisjointFrom/set-like-class-order.js
which I think might be mistakes. Other than that, feel free to ignore the rest of the comments, they're not blocking as far as I'm concerned.
test/built-ins/Set/prototype/isDisjointFrom/set-like-class-order.js
Outdated
Show resolved
Hide resolved
test/built-ins/Set/prototype/isDisjointFrom/set-like-class-order.js
Outdated
Show resolved
Hide resolved
test/built-ins/Set/prototype/isSubsetOf/set-like-class-order.js
Outdated
Show resolved
Hide resolved
test/built-ins/Set/prototype/symmetricDifference/set-like-class-order.js
Show resolved
Hide resolved
Co-authored-by: Jordan Harband <[email protected]>
f071f57
to
30a6a68
Compare
Well that was a wild ride! Thanks to everyone involved. Based on our chat conversation I'm merging this now. |
Followup to #3816. Fixes #3738. cc @syg
Basically just tweaked some of the tests from #3816, copy-pasted them for each of the other methods, and tweaked as appropriate.
Reviewed: