[test262] Add staging tests for isSubsetOf set method

[test262-pr-bot]

This CL adds three tests to staging directory of test262.

Bug: v8:13556
Change-Id: Id6e468a0fa29528350a10c94e9dd19c2835788e8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5055866
Reviewed-by: Liviu Rau <[email protected]>
Commit-Queue: Liviu Rau <[email protected]>
Cr-Commit-Position: refs/heads/main@{#91139}

[ljharb]

These tests pass on my spec-implemented polyfill; LGTM pending comments

[ljharb]

Suggested change

	const SetLike = {
	const setLike = {

capitalized things are constructors

[ljharb]

Suggested change

	assert.sameValue(firstSet.isSubsetOf(SetLike), false);
	assert.sameValue(firstSet.isSubsetOf(setLike), false);

[ljharb]

Suggested change

	const otherSet = new Set();
	otherSet.add(42);
	otherSet.add(43);
	otherSet.add(47);
	const otherSet = new Set([42, 43, 47]);

[ljharb]

Suggested change

	const firstSet = new Set();
	firstSet.add(42);
	firstSet.add(43);
	const firstSet = new Set([42, 43]);

[ljharb]

Suggested change

	const firstSet = new Set();
	firstSet.add(42);
	firstSet.add(43);
	firstSet.add(45);
	const firstSet = new Set([42, 43, 45]);

[ljharb]

Suggested change

	const firstSet = new Set();
	firstSet.add(42);
	firstSet.add(43);
	firstSet.add(44);
	const firstSet = new Set([42, 43, 44]);

[ljharb]

Suggested change

	if (key == 42) {
	if (key === 42) {

[ljharb]

Suggested change

	return this.arr.indexOf(key) != -1;
	return this.arr.indexOf(key) !== -1;

[syg]

@ljharb We're trialing the 2-way sync bot for test262 export, and handling upstream change requests in the auto-generated PR adds a lot of complication that's out of scope.

We'll work on adding some notice that these PRs are auto-generated and to comment on the upstream CL.

In the meantime, I suggest the following workflows:

1. If the changes are necessary before landing (these are staging tests after all), please request them upstream at the CL, by following the Reviewed-on: link.
2. If the changes can wait, please open follow-up PRs afterwards.

WDYT?

[ljharb]

I can certainly try it, but that feels like a bit outsized of an ask. If the bot's PRs aren't meant to be reviewed (and are in fact trusted in that sense), why are they PRs at all as opposed to just pushing commits?

Can't the bot notify the v8 author that there's comments, so they can make the changes in the CL?

[syg]

I can certainly try it, but that feels like a bit outsized of an ask. If the bot's PRs aren't meant to be reviewed (and are in fact trusted in that sense), why are they PRs at all as opposed to just pushing commits?

Indeed they are trusted in that sense: they are merged once the upstream CL is reviewed by an engine implementer. They might be better fit as commits, which is what I think the WPT bot does. We can change it to that to avoid this workflow awkwardness.

Edit: Double checked WPT's commits, and it does look like all the export bots just push (with the right metadata linking back to the original upstream commit with review info, of course).

Edit edit: Actually I was totally wrong, all the export bots do create PRs, see e.g. this one. Though I don't think any of them re-imports review comments. I think consistency here would be good.

Can't the bot notify the v8 author that there's comments, so they can make the changes in the CL?

Yes, though recall the goal here is to simplify V8 developers' test workflows to incentivize writing test262 tests directly instead of our engine-specific tests by making test262 part of the existing feature development workflow. The additional step of "also check and apply GH PR reviews" goes against that goal.

[syg]

Okay, given the nature of all the export bots creating PRs, and that the export workflow is more of a one-way export workflow instead of a bidirectional flow, my suggestion is (2) above: change it in follow-ups.

Since these are staging tests, and are going to be authored by engine implementers, I suspect they will always be stylistically different than the non-staging tests. It's probably more fruitful to review at "graduation" time, when we try to move these into the main trunk, than ad-hoc review at export time.

dismissing the block, since all changes requested are nits that can be done in a followup

[ptomato]

I agree, reviewing at "graduation" time seems more in line with the philosophy we decided on for staging. I also agree with Jordan that it'd be fine for the bot to just push the commit, but opening a PR is fine with me too and maybe is better for visibility.