Add Object.getOwnPropertyDescriptors tests

[ljharb]

Tests for the stage 3 proposal for Object.getOwnPropertyDescriptors.

[jugglinmike]

The G identifier is not relevant for this test

[jugglinmike]

Looking good, @ljharb! I left some in-line notes for minor alterations. One patch-wide suggestion concerns the es7id tag. Given that this is not targeting ES7 (and that we're moving to a version-independent identification scheme), these tests should probably specify esid: pendingid: pending instead.

[jugglinmike]

Oh, and one more test! For

[...]
3. Let descriptors be ! ObjectCreate( %ObjectPrototype%).
[...]
5. Return descriptors. 

We're want something like,

assert.sameValue(
  Object.getPrototypeOf(Object.getOwnPropertyDescriptors({})),
  Object.prototype
);

[ljharb]

Thanks, I'll make the requested changes

[ljharb]

Updated!

[caitp]

here's one you could try:

var i = 0;
var descriptors = [
  { enumerable: false, value: "A1", writable: true, configurable: true },
  { enumerable: true, value: "A2", writable: true, configurable: true }
];
var log = "";
var proxy = new Proxy({}, {
  ownKeys() {
    log += "ownKeys|";
    return ["DUPLICATE", "DUPLICATE", "DUPLICATE"];
  },
  getOwnPropertyDescriptor(t, name) {
    log += "getOwnPropertyDescriptor:" + name + "|";
    return descriptors[i++];
  }
});

var result = Object.getOwnPropertyDescriptors(proxy);
assert.sameValue(result.hasOwnProperty("DUPLICATE"), true);
assert.sameValue(result.DUPLICATE, undefined);
assert.sameValue(log, "ownKeys|getOwnPropertyDescriptor:DUPLICATE|getOwnPropertyDescriptor:DUPLICATE|getOwnPropertyDescriptor:DUPLICATE|");

This exposes some currently broken code in v8, and from IRC discussions, v8 will probably have to fix its bug :d

[ljharb]

@jugglinmike just noticed your edit - IDs updated to "id".

[goyakin]

@ljharb, what do you think about using the constant 2 here instead of Object.getOwnPropertyNames? That way, we won't depend on the correctness of the function.

[ljharb]

sure, np

[ljharb]

@goyakin @jugglinmike friendly ping! :-D

[jugglinmike]

Since we use the list of traps in multiple files, I think we should create a helper to enumerate them. This would be similar to testWithTypedArrayConstructors defined in harness/testTypedArray.js. We can use it like:

testWithTraps(assertTrapThrows);

I disagree. These tests are themselves for the a helper function. Introducing a helper to test a helper seems a little too abstract (and would necessitate tests for the helper for the tests for the helper :P).

[jugglinmike]

It's not enough to check that the trap didn't throw because we also want to be
sure that the specified function was invoked. If you want to enforce a loose
contract, you could assert this with a counter variable for each provided trap.
If you don't mind stricter tests (or just want to avoid all those counters),
you could

-var traps = allowProxyTraps({
-var input = {
   getPrototypeOf: function () {},
   setPrototypeOf: function () {},

 ...
   apply: function () {},
   construct: function () {},
-});
+};
+var traps = allowProxyTraps(input);

...and then

-function assertTrapSucceeds(trap) {
+function assertCustom(trap) {
+  if (input[trap] !== traps[trap]) {
+    throw new Test262Error('Expected custom trap');
+  }
+}

[ljharb]

That's a fair point - although I'd say it is enough here since there's a parallel test that asserts that it does throw when an override isn't defined. I'll update with this change.

[ljharb]

Updated!

[goyakin]

I disagree. These tests are themselves for the a helper function. Introducing a helper to test a helper seems a little too abstract (and would necessitate tests for the helper for the tests for the helper :P).

I can see how testing a helper for a helper might be weird, but we don't have to have the additional tests. We enumerate the traps manually in two places and need to manually verify that all of them are covered in both anyway.

Moreover, when a new trap is added, we won't have to look for all the files where all traps are used if we have the helper.

We currently need all traps only in two files, so this isn't much of a problem though.

[jugglinmike]

Looks good to me!

[goyakin]

We need to return traps from the function 😉

[ljharb]

whoops again :-) thanks, fixed

[ljharb]

Thank you! Onward towards stage 4!

[caitp]

Object.keys() does not include Symbols, only Strings --- so this doesn't really work. See https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/14411/steps/Test262%20-%20no%20variants/logs/symbols-included/text

[anba]

#518 fixes this issue.

[caitp]

alright, leave a note on my PR I guess