Add tests for stage 3 proposal error cause

[legendecas]

Implementation Status:

• ChakraCore: chakra-core/ChakraCore@b992a5d
• JSC: WebKit/WebKit@b03c4f4

[jugglinmike]

Thanks for the patch, @legendecas!

Test262 has some automated checks that run against all pull requests, and the results are available at the bottom of each GitHub Pull Request page. The checks are labeled as "build & lint tests." For this Pull Request, the check is reporting a problem:

Linting complete. 3 errors found.
/home/circleci/test262/test/built-ins/Error/cause_property.js: HARNESS - verifyConfigurable & verifyProperty may not be used in the same file
/home/circleci/test262/test/built-ins/NativeErrors/AggregateError/cause-property.js: HARNESS - verifyConfigurable & verifyProperty may not be used in the same file
/home/circleci/test262/test/built-ins/NativeErrors/cause_property_native_error.js: HARNESS - verifyConfigurable & verifyProperty may not be used in the same file

The verifyProperty is capable of verifying the entire property descriptor. Would you mind updating your patch to use that instead of verifyNotEnumerable, verifyWritable, and verifyConfigurable?

(We recommend using verifyProperty instead of a combination of the more targeted functions because it's so tricky to get the sequence of those functions right. As a bonus, verifyProperty fails with a message describing every violated expectation--implementers will only see the first violation from the more targeted functions.)

Here are some ideas about improving coverage:

• InstallErrorCause has two observable behaviors that we're not testing yet. They're strange edge cases, but those make them the most likely for implementers to miss!

  • 1. If Type(options) is Object and ? HasProperty(options, "cause") is true, then

    Could you add a test for when HasProperty returns an abrupt completion?

  • Let cause be ? Get(options, "cause").

    Could you add a test for when Get returns an abrupt completion?

• In all cases, the error "message" is installed before the error "cause." This might seem inconsequential, but if implementers get it backwards, then that could lead to interoperability bugs. For example:

  var sequence = [];
  new Error(
    {toString() { sequence.push('toString'); }},
    {get cause() { sequence.push('cause'); }}
  );
  

[jugglinmike]

Although we expect developers to set the cause to Error instances, there's nothing in the proposal which requires that. Would you mind using a plain Object instead? That will help verify that the feature is value-agnostic without reducing the test's ability to assert identity.

[jugglinmike]

Hi again, @legendecas. Would you like a hand making those changes?

[legendecas]

@jugglinmike sorry for the delay, just updated the tests to include your great suggestions. thanks.

[jugglinmike]

Hi there, @legendecas! I noticed that you pushed up a new version yesterday, so I've taken another look. (In the future, if you could avoid rewriting previously-submitted commits, that will make it easier for folks to review your changes.)

This new version is great! There are a few minor improvements that we can still make, but they don't concern correctness, so I'll make a follow-up patch to discuss further.

However, there was one issue that was related to correctness: the Proxy constructor was being invoked without the new operator. That prevented the test from passing in conforming runtimes. Since the correction was straightforward and since I'd like to make these tests available to consumers as soon as possible, I've gone ahead and pushed a fix to this branch. Hope you don't mind!

[legendecas]

@jugglinmike thanks a lot for the correctness update! It is not quite easy to check the test correctness with tools that I'm aware of. I'm so sorry for the poor updates 😵 .