test262 icon indicating copy to clipboard operation
test262 copied to clipboard

Define policy for Promise rejections in sync tests

Open jugglinmike opened this issue 4 years ago • 4 comments

Although the semantics being verified by a synchronous test may be observable during the initial evaluation, subsequently-rejected jobs may indicate errors in test design or anomalies in the implementation under test.

A more relaxed policy would instruct implementations to ignore any unhandled rejections in synchronous tests. Compared to the policy introduced by this patch, such a policy would impose fewer constraints on test authors. However, it would also make their tests more susceptible to mistakes, and it would also reduce Test262's ability to identify implementation defects.

Since test authors are in full control of the Promise values created in their tests, they can consistently handle rejections. This requirement will help surface mistakes (whether in test design or in the implementation under test).


This change introduces a policy where historically there has been no policy, and there are certainly other alternatives. Starting with a concrete proposal seemed like the best way to inspire productive discussion.

Today, V8 satisfies this policy when tested using the test262-harness utility. SpiderMonkey interprets unhandled rejections similarly to uncaught exceptions, but it does not recognize the "name" of the error constructor.

Chakra, JavaScriptCore, QuickJS, GraalJS, Engine262, and XS all ignore unhandled rejections in synchronous tests.

Simply following the majority would dictate a different policy than what I'm suggesting in this patch. As the commit message explains, I think the more strict policy is beneficial for both test authors and for the implementations themselves.

As far as I'm aware, no synchronous tests intentionally produce uncaught exceptions today. This change will help us consistently vet future contributions, and it has the potential to surface implementation bugs which have been ignored.

To Test262's consumers: would you mind sharing how difficult it would be to adhere to this policy?

Whatever policy we choose will impact (at least) ease of contribution, test expressiveness, and ease of consumption. Does this patch strike a good balance?

jugglinmike avatar May 13 '21 19:05 jugglinmike

This looks good to me. Nice work identifying this and coming up with a straight forward solution. I have two questions:

  1. Safe to assume we will do a new version for this?
  2. What kind of "outreach" plan do you have? You'll need to coordinate with implementers so they know to update their own runners (or you could do the updates for them, which we've done in the past).

rwaldron avatar May 14 '21 15:05 rwaldron

This looks good to me. Nice work identifying this and coming up with a straight forward solution.

Thanks!

  1. Safe to assume we will do a new version for this?

Yeah, that sounds right to me. Test262 is an odd application for SemVer, so I'm having a hard time classifying this as a "new feature" or a "breaking change." Choosing the latter is probably safest, so this would bring the project to 5.0.0.

In the absence of a formal release process, I think this patch should include the change. Do you agree?

  1. What kind of "outreach" plan do you have? You'll need to coordinate with implementers so they know to update their own runners (or you could do the updates for them, which we've done in the past).

The "harness" test introduced here will alert the appropriate people when they receive it and observe a new test failure. That may be sufficient because we don't intend to rely on this behavior. In other words: we won't be writing tests with unhandled rejections; we're only specifying it to reduce the margin for error. We know consumers have mechanisms for ignoring failing tests.

I don't want to assume that implementing these semantics will be trivial for consumers, but again, since it's not strictly required...

This is making me rethink the language in this patch. If we really don't intend to write tests with unhandled rejections, do we need to require the behavior? Would it make more sense to use RFC 2119 SHOULD in INTERPRETING.md and skip the version bump altogether?

So much for "straightforward"

jugglinmike avatar May 14 '21 17:05 jugglinmike

Sorry for the delay... I'm actually not sure how to answer that last question :|

rwaldron avatar Jun 24 '21 17:06 rwaldron

I want to be neutral to this change but my personal preference says this is the opposite direction we should take.

As far as I'm aware, no synchronous tests intentionally produce uncaught exceptions today. This change will help us consistently vet future contributions, and it has the potential to surface implementation bugs which have been ignored.

This will require attention to the flow of all tests and I guess we might actually have uncaught exceptions.

By their own nature, unhandled rejections should not solute the flow of runtime execution. It's not meant to be an end of code execution and therefore just results into an abrupt completion that is ignored.

I'm in favor of avoiding tests if they have any accidental/unintentional and unnecessary rejected assertions, but I'm not a fan to require runtimes to expose the unhandled rejections when there is no actual code for that.

leobalter avatar Jul 29 '21 20:07 leobalter