added tests for Array.prototype.group

[polsevev]

Added tests for proposal Array Grouping groupBy

CC @codehag

[rwaldron]

Again, very cool that you jumped right in on these :D we'll also go over this one during our video call on Friday. See you then!!

[ljharb]

This looks great! I compared it against my polyfill's tests, and noticed this PR isn't testing that a non-function callbackfn throws a TypeError, and that the [[Prototype]] of the returned object is null.

[leobalter]

The tests are good, I added a few suggestions only.

You may also wanna test:

• the return type of groupBy
• assert this value inside the callbackfn w/ and w/o thisArg
• non-callback values for callbackfn
• non valid values for the this value
• tainted callbackfns (throwing an exception).
• callbackfn calls with terse arrays like [,,,1,,2]
• array like values for the this value, e.g. [].groupBy.call({length: 2, '1': 'foo'}, callbackfn);

[hemanth]

This should be group instead of groupBy now.

[jridgewell]

#3354 (review)

the return type of groupBy

done.

assert this value inside the callbackfn w/ and w/o thisArg

done.

non-callback values for callbackfn

done.

non valid values for the this value

done.

tainted callbackfns (throwing an exception).

Do we have to?

callbackfn calls with terse arrays like [,,,1,,2]

done.

array like values for the this value, e.g. [].groupBy.call({length: 2, '1': 'foo'}, callbackfn);

Done.

[ptomato]

Thanks for resuming this PR, and apologies that it took some time to get around to it. (I'll start on groupToMap as soon as possible.) I think this is basically ready to merge. Some small suggestions, but keeping in mind how long this has already taken to review, I've made them into GitHub suggestions that you can just click to accept or not, so that the review process don't cause any more delay on this PR.

I have some suggestions for additional things that could be tested, but for the same reason I wouldn't say that they should block this PR. They can be added in a second PR if you are so inclined:

• Test throwing on an arraylike with a value of its length property that can't be converted to an integer, or a getter that throws
• Test throwing in step 6.b if Pk is a property with a getter that throws
• Test throwing if the callback returns a value that can't be converted to a property key (e.g. an object with a toString method that throws)

Thanks again for resuming this work!

[ptomato]

tainted callbackfns (throwing an exception).

Do we have to?

Would be nice, but as with the other suggestions I don't see a need to block this PR on having that.

Another one I thought of: shortening and lengthening the array during the iteration.

[jridgewell]

Test throwing on an arraylike with a value of its length property that can't be converted to an integer, or a getter that throws

Done.

Test throwing in step 6.b if Pk is a property with a getter that throws

Done.

Test throwing if the callback returns a value that can't be converted to a property key (e.g. an object with a toString method that throws)

Done.

tainted callbackfns (throwing an exception).

Done.

[ptomato]

Awesome, thanks!