Skip to content
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

Merged
merged 14 commits into from
Jan 10, 2024
Merged

More set methods tests #3966

merged 14 commits into from
Jan 10, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Nov 26, 2023

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:

  • difference
  • intersection
  • isDisjointFrom
  • isSubsetOf
  • isSupersetOf
  • symmetricDifference
  • union

@bakkot bakkot requested a review from a team as a code owner November 26, 2023 06:44
@rmahdav
Copy link
Contributor

rmahdav commented Nov 27, 2023

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

Copy link
Contributor

@Ms2ger Ms2ger left a 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.

test/built-ins/Set/prototype/difference/set-like-array.js Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor Author

bakkot commented Nov 30, 2023

One thing I don't think I've seen is a check for the +0/-0 step.

Nice catch, added. I had it for the other three non-predicate methods; don't know why I missed it for difference.

Copy link
Contributor

@ptomato ptomato left a 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/interesection.js Outdated Show resolved Hide resolved

let observedOrder = [];

function observableIterator() {
Copy link
Contributor

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 😄

Copy link
Contributor

@ptomato ptomato left a 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!

Copy link
Member

@ljharb ljharb left a 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.

@bakkot
Copy link
Contributor Author

bakkot commented Dec 18, 2023

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?

That's covered by SetDataHas (invoked in 6.c.ii.2) using SameValueZero.

@ljharb
Copy link
Member

ljharb commented Dec 18, 2023

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 :-)

Copy link
Member

@ljharb ljharb left a 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

@bakkot
Copy link
Contributor Author

bakkot commented Dec 20, 2023

@ljharb Fixed the isSupersetOf/set-like-array.js bug and added the -0 tests for isSupersetOf and isDisjointFrom, thanks for the catches/review.

Copy link
Contributor

@ptomato ptomato left a 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.

@ptomato
Copy link
Contributor

ptomato commented Jan 10, 2024

Well that was a wild ride! Thanks to everyone involved. Based on our chat conversation I'm merging this now.

@ptomato ptomato merged commit c8cd136 into main Jan 10, 2024
8 of 9 checks passed
@ptomato ptomato deleted the more-set-methods branch January 10, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests for Set methods
5 participants