`Promise.allKeyed` and `Promise.allSettledKeyed ` testing plan

[acutmore]

Test plan for https://tc39.es/proposal-await-dictionary/ (issue 18)

Boilerplate:

• Property descriptors
• Function lengths
• Function names
• Are not constructors
• prototypes are Function.prototype
• are extensible

Success cases:

• Object with no keys
• Object with no enumerable properties
• Promises already resolved before call
• Promises resolve after call
• Resulting key order matches original order (not the order promises settled)
• Promise.allSettledKeyed
  • All promises reject
  • All promises fullfilled
  • Mix of rejected and fullfilled

Failure cases:

• Testing newPromiseCapability
  • Receiver is not a constructor
  • Constructor throws
  • Constructor does not provide a 'resolve' and 'reject' function
• Testing GetPromiseResolve
  • Receiver does not have a .resolve function
• Not passing an object as the argument
• Promise.allKeyed
  • The first promise to settle is rejected
  • The second promise to settle is rejected
  • The last promise to settle is rejected

Other edge cases:

• Synchronous thenables to cover PerformPromiseAllKeyed step 8
• calling promiseResolve throws
• promiseResolve does not return a thenable
• Exotic object where [[OwnPropertyKeys]] throws
• Exotic object where [[GetOwnProperty]] throws
• Exotic object where [[GetOwnProperty]] returns undefined

[ptomato]

I had a quick look and I think most of the tests under all and allSettled are going to be equally relevant, and easily adaptable, to allKeyed and allSettledKeyed. I would suggest starting from there, and deleting any tests that have to do specifically with iterating over the array provided to all and allSettled. There are a bunch of checks that the iterator is either closed or not closed under certain conditions, which won't be relevant.

(Note that you can probably simplify a lot of those tests using async helpers)

I'd also like to see failure cases like

• Argument not an object
• Object with accessor properties that throw when called

There are a lot of edge cases that can be built around the idea of passing an object with an accessor key that tries to sabotage something else. For example

• if the accessor key tries to delete Promise.resolve, that should still succeed, because Promise.resolve is grabbed and saved eagerly
• an accessor key could set a flag that changes the behaviour of the saved Promise.resolve
• an accessor key could delete subsequent properties, which should then be skipped if I'm reading the spec correctly
• an exotic object could make [[GetOwnProperty]] return a property descriptor but then delete the property so the subsequent Get would get the property off the prototype
• probably a lot more of this kind of thing!

[acutmore]

There are a lot of edge cases that can be built around the idea of passing an object with an accessor key that tries to sabotage something else. ... probably a lot more of this kind of thing!

Absolutely. I wasn't sure how in-depth to go here.

I suspect almost all interesting exotic behaviors the object argument could exhibit would likely also apply to Iterator.zipKeyed too - would it be worth trying to share a harness/utility across the two? My gut is that the extra indirection wouldn't be worth it against just adding similar cases for both APIs as tests are added.

[ptomato]

Yeah, in-depth is welcomed - this (non-local user code sabotage) is the kind of thing that implementations, polyfills, and even proposal authors often get wrong, so it's good to cover it.

Historically the preference has been to avoid adding harness files that are used in just a few places. I personally don't have much of a preference, and honestly whatever makes it easier and more pleasant for contributors to actually write these tests has my support! Maybe the kind of utility you're thinking of might have more applications beyond zipKeyed, maybe with options bags?

[bakkot]

We did end up adding a harness file just for use in zip/zipKeyed in #4609, fwiw.

(From that harness, _assertIsNullProtoMutableObject is probably useful here, but nothing else; that one is only used internally to the harness file so splitting it out would be moderately annoying unless it went into propertyHelper.js which is already required for use of the new harness.)