Set union tests #3816

frehner · 2023-04-12T23:24:17Z

Hello! First time code contributor here, so please feel free to correct anything and everything that I'm doing. I appreciate your time. ❤️

This is the very beginnings of helping with #3738 . I'm hoping that I can be helpful here, but there are still quite a bit that I'm missing and unsure on how to do.

If it helps, there's also an associated engine262 PR here engine262/engine262#212 where I've done a basic implementation of the spec and have been using it to validate these tests so far.

In any case, I figured this was a good place to stop and get feedback before moving on further. I want to make sure that I'm not doing something wrong or using patterns that are incorrect.

Thanks again!

ljharb

Looks good to me so far! Feel free to look at https://github.com/es-shims/Set.prototype.union in case there's any tests there you haven't covered.

frehner · 2023-04-17T22:23:42Z

I noticed that the Set.prototype.add test suite has tests on the this value and RequireInternalSlot steps. Should I do the same for union?

e.g.

1. Let O be the this value.
2. Perform ? RequireInternalSlot(O, [[SetData]]).

ptomato · 2023-04-19T16:59:59Z

That's correct, pretty much every public API has a set of boilerplate tests that include tests for those steps. Here's a list that I keep in my notes of boilerplate tests, with links to examples:

Property descriptor of the object/method/whatever
Brand check for methods
Function length property (although note that new tests should use verifyProperty() instead)
Function name property
Function is a constructor (or not a constructor)
Object prototype and extensibility

frehner · 2023-05-08T22:57:56Z

Would these tests need to have a features flag in the frontmatter?

ptomato · 2023-05-09T17:00:31Z

That's right. It looks like there isn't a feature defined in features.txt yet, so that would also need to be added.

frehner · 2023-05-09T17:03:29Z

I'll add it here, then! Should I go for something generic set-methods (so it can be used for the other methods as well) or more specific Set.prototype.union?

ptomato · 2023-05-09T17:26:57Z

Feature flags are generally per-proposal.

frehner · 2023-05-12T20:22:26Z

Ok, I think these tests for Set.prototype.union are ready. How would you like me to proceed? Should I mark this as ready for review, or would you like me to implement tests for the other new methods as part of this PR?

ptomato · 2023-05-18T18:35:30Z

I think that's mainly a question of what you prefer, as the test writer. Certainly it's OK to merge a PR that provides some but not all of the tests listed in #3738, as long as we keep track in that issue what's been done and what's left to do.

frehner · 2023-05-22T20:03:32Z

Left a comment here on what I've implemented and not yet implemented (only 3 bullet points not implemented by this PR).

I think I'll mark this as ready for review and can follow up on those 3 bullet points when I'm a little more clear on what I need to do on them. 👍

jugglinmike · 2023-05-31T15:35:12Z

Hi there, @frehner! I see that you received some direction from Kevin over in gh-3738; did you want to incorporate that here in this patch? There's no rush--I just don't want to merge this prematurely.

frehner · 2023-05-31T17:27:38Z

Hi there, @frehner! I see that you received some direction from Kevin over in gh-3738; did you want to incorporate that here in this patch? There's no rush--I just don't want to merge this prematurely.

Yeah! I'll try to get to it hopefully this weekend, if that's ok.

frehner · 2023-06-04T00:04:26Z

Ok I think I got it all now. 😅

bakkot

LGTM other than nit. Thanks!

test/built-ins/Set/GetSetRecord/keys-is-callable.js

test/built-ins/Set/prototype/union/add-not-called.js

test/built-ins/Set/prototype/union/receiver-not-set.js

frehner · 2023-06-20T21:56:31Z

Not sure what's with the failing CI check graaljs, any pointers there would be helpful.

GraalJS ❯ Downloading https://github.com/oracle/graaljs/releases/download/vm-23.0.0/graaljs-23.0.0-linux-amd64.tar.gz
esvu ✖ Error: Got 404
    at /home/circleci/test262/node_modules/esvu/src/installer.js:69:17
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Function.install (/home/circleci/test262/node_modules/esvu/src/installer.js:66:30)
    at async installEngine (/home/circleci/test262/node_modules/esvu/src/bin.js:121:3)
    at async main (/home/circleci/test262/node_modules/esvu/src/bin.js:168:7)
esvu ✖ Some engines were not installed.

bakkot · 2023-06-20T22:07:43Z

The graal failures are unrelated to this PR - at a glance, it looks like it's caused by that project switching from having their releases tagged like vm-X.X.X to having them tagged like graal-X.X.X in their most recent release (last week).

(Edit: opened esvu issue.)

test/built-ins/Set/prototype/union/subclass-receiver-methods.js

ljharb · 2023-06-21T20:45:48Z

I rebased the PR, and ran it against my set method polyfills, and everything's passing, as expected.

frehner · 2023-07-12T21:19:47Z

Just a followup here: anything I need to do?

ptomato · 2023-07-13T09:36:46Z

Just a followup here: anything I need to do?

Afraid not; the bottleneck at this point is maintainer availability for a review. Thanks @bakkot for also reviewing it, though; that's helpful!

Co-authored-by: Kevin Gibbons <[email protected]>

Co-authored-by: Jordan Harband <[email protected]>

Ms2ger

All requests for more test coverage can be deferred, but if you do, please add a checklist to the tracking issue.

test/built-ins/Set/GetSetRecord/called-with-object.js

test/built-ins/Set/GetSetRecord/has-is-callable.js

test/built-ins/Set/prototype/union/set-like-class-order.js

test/built-ins/Set/prototype/union/subclass-symbol-species.js

test/built-ins/Set/prototype/union/union-function-properties.js

test/built-ins/Set/prototype/union/union-name-properties.js

bakkot · 2023-11-08T22:39:30Z

I can take this over so we can get this landed and engines can start shipping.

frehner · 2023-11-08T22:49:26Z

I can take this over so we can get this landed and engines can start shipping.

Thanks - I currently don't have capacity to work on this at the moment, but would have time in perhaps a couple of months.

bakkot · 2023-11-12T04:27:41Z

Thanks for your thorough review @Ms2ger. I believe I've addressed all your comments.

If this is approved I'll follow up with tests for the other methods.

Ms2ger · 2023-11-15T10:04:36Z

test/built-ins/Set/prototype/union/size-is-a-number.js

+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: sec-getsetrecord
+description: GetSetRecord if the Set-like object has a size of 'undefined' an error is thrown


Suggested change

description: GetSetRecord if the Set-like object has a size of 'undefined' an error is thrown

description: GetSetRecord throws an exception if the Set-like object has a size that is coerced to NaN

Ms2ger · 2023-11-15T10:09:38Z

test/built-ins/Set/prototype/union/has-is-callable.js

+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: sec-getsetrecord
+description: GetSetRecord if the Set-like object's 'has' property is not callable an error is thrown


Suggested change

description: GetSetRecord if the Set-like object's 'has' property is not callable an error is thrown

description: GetSetRecord throws an exception if the Set-like object's 'has' property is not callable

Ms2ger · 2023-11-15T10:09:51Z

test/built-ins/Set/prototype/union/keys-is-callable.js

+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: sec-getsetrecord
+description: GetSetRecord if the Set-like object's 'keys' property is not callable an error is thrown


Suggested change

description: GetSetRecord if the Set-like object's 'keys' property is not callable an error is thrown

description: GetSetRecord throws an exception if the Set-like object's 'keys' property is not callable

Ms2ger · 2023-11-15T10:10:37Z

test/built-ins/Set/prototype/union/has-is-callable.js

+  function () {
+    s1.union(s2);
+  },
+  "GetSetRecord throws an error when has is not callable"


Suggested change

"GetSetRecord throws an error when has is not callable"

"GetSetRecord throws an error when has is undefined"

Ms2ger · 2023-11-15T10:10:48Z

test/built-ins/Set/prototype/union/keys-is-callable.js

+  function () {
+    s1.union(s2);
+  },
+  "GetSetRecord throws an error when keys is not callable"


Suggested change

"GetSetRecord throws an error when keys is not callable"

"GetSetRecord throws an error when keys is undefined"

Ms2ger · 2023-11-15T10:13:05Z

test/built-ins/Set/prototype/union/allows-set-like-class.js

+const s2 = {
+  size: 2,
+  has: function () {
+    throw new Test262Error("Set.prototype.union should not invoke .has on its argument");
+  },
+  keys: function* keys() {
+    yield 2;
+    yield 3;
+  },
+};


I'm afraid you went a bit too far here - this is now basically a duplicate of test/built-ins/Set/prototype/union/allows-set-like-object.js, while the file name suggests that it should cover a class

Ms2ger · 2023-11-15T10:36:24Z

test/built-ins/Set/prototype/union/set-like-class-mutation.js

+const evilSetLike = {
+  size: 2,
+  get has() {
+    baseSet.add("q");


Should this be

Suggested change

baseSet.add("q");

baseSet.delete("q");

?

Ah, actually I meant to use add but change the expectations for the resulting sets. Not sure how I made that mistake...

Ms2ger · 2023-11-15T10:44:33Z

test/built-ins/Set/prototype/union/length.js

+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: sec-set.prototype.union
+description: Set.prototype.union function properties


Suggested change

description: Set.prototype.union function properties

description: Set.prototype.union length property

Ms2ger · 2023-11-15T10:44:46Z

test/built-ins/Set/prototype/union/name.js

+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+esid: sec-set.prototype.union
+description: Set.prototype.union name properties


Suggested change

description: Set.prototype.union name properties

description: Set.prototype.union name property

bakkot · 2023-11-15T16:17:59Z

@Ms2ger Done.

ljharb reviewed Apr 13, 2023

View reviewed changes

frehner mentioned this pull request Apr 24, 2023

Tests for Set methods #3738

Closed

18 tasks

frehner marked this pull request as ready for review May 22, 2023 20:03

frehner requested a review from a team as a code owner May 22, 2023 20:03

frehner mentioned this pull request May 22, 2023

Implement Set.union engine262/engine262#212

Open

ptomato added the needs review label May 29, 2023

bakkot approved these changes Jun 11, 2023

View reviewed changes

test/built-ins/Set/GetSetRecord/keys-is-callable.js Outdated Show resolved Hide resolved

test/built-ins/Set/prototype/union/add-not-called.js Outdated Show resolved Hide resolved

ljharb reviewed Jun 11, 2023

View reviewed changes

test/built-ins/Set/prototype/union/receiver-not-set.js Outdated Show resolved Hide resolved

ljharb requested changes Jun 21, 2023

View reviewed changes

test/built-ins/Set/prototype/union/subclass-receiver-methods.js Outdated Show resolved Hide resolved

ljharb force-pushed the set-union-tests branch from 4a821b3 to 9899630 Compare June 21, 2023 20:21

ljharb approved these changes Oct 25, 2023

View reviewed changes

frehner added 2 commits October 25, 2023 08:51

Beginning of adding Set.prototype.union tests

f7bc820

Fix description

0f3df99

frehner and others added 9 commits October 25, 2023 08:51

Set.prototype.union tests for arrays and subclass methods

f6c2a39

Add the Set-methods frontmatter feature flag

5bb5143

Add additional Set.prototype.union test for edge cases

1077c9c

Update test/built-ins/Set/GetSetRecord/keys-is-callable.js

f11475e

Co-authored-by: Kevin Gibbons <[email protected]>

Use compareArray() for assertions

0c5358d

Remove class field syntax

ca63aa0

Remove unused args

c0382d9

Update test/built-ins/Set/prototype/union/subclass-receiver-methods.js

43e6006

Co-authored-by: Jordan Harband <[email protected]>

Return original 'add'

dfed700

ljharb force-pushed the set-union-tests branch from 3f313f7 to dfed700 Compare October 25, 2023 15:51

ljharb requested a review from a team as a code owner October 25, 2023 15:51

Ms2ger requested changes Oct 26, 2023

View reviewed changes

ptomato added awaiting author and removed needs review labels Nov 3, 2023

bakkot added 2 commits November 11, 2023 20:19

address comments

490c71f

add one more mutation in the evil mutating iterator test

5723c5c

Ms2ger requested changes Nov 15, 2023

View reviewed changes

address further comments

5badea8

Ms2ger approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'main' into set-union-tests

1af9b78

Ms2ger enabled auto-merge (squash) November 15, 2023 16:53

Ms2ger merged commit 60310b7 into tc39:main Nov 15, 2023
8 of 9 checks passed

bakkot mentioned this pull request Nov 19, 2023

Consolidate set methods features #3962

Merged

bakkot mentioned this pull request Nov 26, 2023

More set methods tests #3966

Merged

7 tasks

frehner deleted the set-union-tests branch July 17, 2024 21:56

ioannad mentioned this pull request Jul 26, 2024

[explicit-resource-management] some boilerplate tests from PR #3866 #4172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set union tests #3816

Set union tests #3816

frehner commented Apr 12, 2023

ljharb left a comment

frehner commented Apr 17, 2023 •

edited

Loading

ptomato commented Apr 19, 2023

frehner commented May 8, 2023

ptomato commented May 9, 2023

frehner commented May 9, 2023

ptomato commented May 9, 2023

frehner commented May 12, 2023

ptomato commented May 18, 2023

frehner commented May 22, 2023

jugglinmike commented May 31, 2023

frehner commented May 31, 2023

frehner commented Jun 4, 2023

bakkot left a comment

frehner commented Jun 20, 2023

bakkot commented Jun 20, 2023 •

edited

Loading

ljharb commented Jun 21, 2023

frehner commented Jul 12, 2023

ptomato commented Jul 13, 2023

Ms2ger left a comment

bakkot commented Nov 8, 2023

frehner commented Nov 8, 2023

bakkot commented Nov 12, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

bakkot Nov 15, 2023

Ms2ger Nov 15, 2023

Ms2ger Nov 15, 2023

bakkot commented Nov 15, 2023

	description: GetSetRecord if the Set-like object has a size of 'undefined' an error is thrown
	description: GetSetRecord throws an exception if the Set-like object has a size that is coerced to NaN

	"GetSetRecord throws an error when has is not callable"
	"GetSetRecord throws an error when has is undefined"

	"GetSetRecord throws an error when keys is not callable"
	"GetSetRecord throws an error when keys is undefined"

	description: Set.prototype.union function properties
	description: Set.prototype.union length property

	description: Set.prototype.union name properties
	description: Set.prototype.union name property

Set union tests #3816

Set union tests #3816

Conversation

frehner commented Apr 12, 2023

ljharb left a comment

Choose a reason for hiding this comment

frehner commented Apr 17, 2023 • edited Loading

ptomato commented Apr 19, 2023

frehner commented May 8, 2023

ptomato commented May 9, 2023

frehner commented May 9, 2023

ptomato commented May 9, 2023

frehner commented May 12, 2023

ptomato commented May 18, 2023

frehner commented May 22, 2023

jugglinmike commented May 31, 2023

frehner commented May 31, 2023

frehner commented Jun 4, 2023

bakkot left a comment

Choose a reason for hiding this comment

frehner commented Jun 20, 2023

bakkot commented Jun 20, 2023 • edited Loading

ljharb commented Jun 21, 2023

frehner commented Jul 12, 2023

ptomato commented Jul 13, 2023

Ms2ger left a comment

Choose a reason for hiding this comment

bakkot commented Nov 8, 2023

frehner commented Nov 8, 2023

bakkot commented Nov 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakkot commented Nov 15, 2023

frehner commented Apr 17, 2023 •

edited

Loading

bakkot commented Jun 20, 2023 •

edited

Loading