Don't stomp on the object in verifyConfigurable test.

[cscott]

Just as we do for isWritable, revert the change after we've verified the property.

This helps us run tests in environments without full isolation between tests, as well as protecting test authors from shooting themselves in the foot inadvertently.

[goyakin]

This call would fail when the property doesn't originally exist given that desc would be undefined. Instead of checking the result of the hasOwnProperty call in the if condition, we should check the descriptor.

[jugglinmike]

Given that this test uses one aspect of the property descriptor API to test property descriptors themselves, I think there's a nontrivial opportunity for false positives in buggy implementations. It would be good to see unit tests for the new behavior added to the project's test/harness directory so that implementors can trace failures faster (e.g. they will be able to differentiate between bugs in the behavior under test and bugs in the test framework itself).

[ljharb]

should this just be if (result && desc)?

[leobalter]

@cscott @goyakin @bterlson @jugglinmike what's the conclusion this PR should take?

[leobalter]

When I was running the tests for this branch I've found this example from test/built-ins/TypedArrays/internals/DefineOwnProperty/non-extensible-redefine-key.js:

var sample = new TA([42, 43]);
  sample.foo = true;
  sample.bar = true;

  Object.preventExtensions(sample);

  assert.sameValue(
    Reflect.defineProperty(sample, "foo", {value:42}),
    true,
    "data descriptor"
  );

  assert.sameValue(sample.foo, 42);
  verifyEnumerable(sample, "foo");
  verifyWritable(sample, "foo");
  verifyConfigurable(sample, "foo");
...

defineProperty will work here because I'm redefining a pre-existing property, but as I'm preventing extensions in sample.

With these changes applied, verifyConfigurable will return the abrupt completion from trying to define a new descriptor. This comes from ValidateAndApplyPropertyDescriptor. Where it checks if the object is extensible only if I'm adding a new property.

---

With this case, I really prefer not creating more side effects than we are already doing. Tests should be atomic and a property should be consumed once for each case. Trying to recover the previous status will add an extra functionality that would open a path for anti-patterns.

What I believe we should add instead is a helper for to check a property at once, so we don't need to worry on the order for calling verify{Enumerable, Writable, Configurable}.

Example:

verifyProperty(sample, 'foo', { value: 42, enumerable: true, writable: false, configurable: false })

This is a case for another PR, anyway.

---

I'm closing this PR due to the long time with no further answer and believe I don't think it's currently a fit. Even though, I appreciate the contribution and efforts.