RFC: Async Helpers

[ptomato]

This proposes a harness/asyncHelpers.js file with two helpers in it, asyncTest() and assert.throwsAsync(). See the RFC for further motivation and details.

Note the draft RFC process in #3525. My idea for this pull request is that besides being an RFC in its own right, we use it as a test case for the process. That is, we follow the draft RFC process and hopefully either validate it, or surface changes that need to be made, after which we can land the RFC process in our documentation.

[ljharb]

Since a promise rejection is not inherently an exception, I prefer node's naming of assert.rejects, but otherwise this looks great.

[Ms2ger]

As discussed, this makes sense to me. I have no strong opinions on the naming.

[jugglinmike]

Thanks for taking the time to write this, Philip! I especially appreciate all the research you did for the prior art.

Most of the rationale applies to assert.throwsAsync. There's comparatively little motivation for the asyncTest function. If I understand the design, then its behavior can be replicated in test bodies with relatively little code:

-asyncTest(async function () {
+(async function () {
   await assert.throwsAsync(TypeError, Array.fromAsync([], null), "null mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], {}), "non-callable object mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], "String"), "string mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], true), "boolean mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], 3.1416), "number mapfn");
-});
+})().then($DONE, $DONE);

This has more characters, but it isn't as concerning as the "boilerplate" you've identified for assert.throwsAsync. On the plus side, it avoids introducing another concept for test writers to learn.

Sticking with .then($DONE, $DONE) also avoids introducing an alternative means of expressing the same intent. That might not be so bad if we were certain that all new asynchronous tests would use asyncTest. Even in that case, though, I don't know if we could completely replace that pattern, so this new utility would potentially contribute to the many "eras" of test-writing styles that cut across this project's files.

Of course, both concerns apply to any new utility function, but they feel more compelling relative to the convenience that asyncTest appears to offer.

[jugglinmike]

Since a promise rejection is not inherently an exception, I prefer node's naming of assert.rejects, but otherwise this looks great.

@ljharb This seems like a result of the proposed API's fluidity. If it only accepted a Promise instance, then assert.rejects would sound like a better name to me, too.

I'd prefer the new assertion accepted only function values, but not only because it increases parity with assert.throws and makes naming a little easier. Yesterday, Jordan made a good point about "callables" and function values being relatively disjoint. Like him, I can't think of a time where the same value, intentionally passed to assert.throwsAsync, could be interpreted in different ways. But I can imagine folks writing (and even approving) tests which mistakenly use one in place of the other. For instance:

await assert.throwsAsync(TypeError, Array.fromAsync, "string value");

That would pass with the current design, even though the intention might have been to validate:

await assert.throwsAsync(TypeError, Array.fromAsync("string value"));

[ptomato]

I'm not attached to the naming, I do have a preference for assert.throwsAsync because it communicates the parallel with assert.throws, but I think it's clear either way.

[Ms2ger]

Thanks for taking the time to write this, Philip! I especially appreciate all the research you did for the prior art.

Most of the rationale applies to assert.throwsAsync. There's comparatively little motivation for the asyncTest function. If I understand the design, then its behavior can be replicated in test bodies with relatively little code:

-asyncTest(async function () {
+(async function () {
   await assert.throwsAsync(TypeError, Array.fromAsync([], null), "null mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], {}), "non-callable object mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], "String"), "string mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], true), "boolean mapfn");
   await assert.throwsAsync(TypeError, Array.fromAsync([], 3.1416), "number mapfn");
-});
+})().then($DONE, $DONE);

This has more characters, but it isn't as concerning as the "boilerplate" you've identified for assert.throwsAsync. On the plus side, it avoids introducing another concept for test writers to learn.

Sticking with .then($DONE, $DONE) also avoids introducing an alternative means of expressing the same intent. That might not be so bad if we were certain that all new asynchronous tests would use asyncTest. Even in that case, though, I don't know if we could completely replace that pattern, so this new utility would potentially contribute to the many "eras" of test-writing styles that cut across this project's files.

While it's true that (async function () { ... })().then($DONE, $DONE); works, I find it much harder to read and much less discoverable than asyncTest - asyncTest expresses the intent of the code much more clearly, IMO. (Writing is correctly is also an issue. I, at least, always need to copy/paste it to be sure I get it right, while typing asyncTest is much nicer.) Also, using asyncTest can improve the developer experience when making other errors, such as forgetting the async flag (as Philip mentioned), or passing a non-async function.

I think overall asyncTest wins over (async function () { ... })().then($DONE, $DONE);. The latter pattern currently exists in ~20 tests, so it doesn't seem like replacing the existing uses would be an issue.

[littledan]

I take it that this RFC does not require changes by test embedders, and only makes things easier for test authors, right? My opinion is that there should be a fairly low bar for such changes, and they shouldn't be subject to excessive gatekeeping/bikeshedding. It's fine to write out an RFC with detailed rationale for such a change, but I don't think it should be required for such issues in the future. I encourage you to land this RFC and the supporting code file, and in general, that you land additions to helper files without requiring strong agreement from everyone who's been involved in test262.

[ptomato]

Summary of discussions

Since this is the first, "trial balloon" RFC, I wanted to take things slow and allow plenty of time for comments from stakeholders. I announced the RFC in various TC39 Matrix channels a few weeks ago, and included it in our status update in last week's TC39 plenary. So far the only comment resulting from that was Dan's.

So with this post I'd like to propose that we move the RFC to final comment period, with a disposition of "merge". Below I'll write a summary of the discussions that occurred and what changes I propose to make (or have already made) based on them. Please feel free to correct or expand if I didn't summarize accurately.

Naming

Jordan prefers renaming assert.throwsAsync() to assert.rejects() in order to make the distinction that a promise rejection is not inherently an exception. (link) Mike agrees about this distinction but would prefer that the API remains assert.throwsAsync() and is narrowed to only accept a callable, in order to avoid confusion and for a better parallel with assert.throws(). (link) Others have indicated no strong preference about the name.

Mike gave an example of a non-obvious misuse of the API, which would be prevented by only accepting a callable. I think this is a good motivation for assert.throwsAsync() to only accept a callable, so I propose to make that change. This gives us the best of both worlds: the narrowing of input type seems like it could resolve Jordan's naming concern.

Proposed conclusion: Update the RFC to only accept callables in assert.throwsAsync(), not thenables.

Scope of API

Mike doesn't find asyncTest() compelling because sticking with (async function () { ... })().then($DONE, $DONE) doesn't require introducing a new concept for test writers to learn, and avoids multiple ways of expressing the same intent in the test corpus. (link)

Ms2ger finds that asyncTest() communicates the test writer's intent more clearly and allows detecting when the async flag is missing (see also next point below).

I do see that asyncTest() introduces a way to achieve something that already existed, but I still see value in it. It's at a higher level of abstraction, which is easier for new contributors to grasp, because you don't have to understand the underlying $DONE mechanism in order to just use asyncTest(). There is still justification for using the lower-level $DONE API, in more complicated tests where the common case covered by asyncTest() doesn't apply. I can add a paragraph to the motivation in the RFC explaining this high-level / low-level distinction.

In order to address Mike's concern about multiple ways to do the same thing within the test corpus, I propose that we port existing tests to use asyncTest() where possible, as part of the implementation of the RFC.

The changes proposed above would hopefully address Mike's concerns sufficiently in order to proceed with asyncTest().

Proposed conclusions: Add additional motivation for asyncTest() to the RFC, and port existing tests to use it when implementing the RFC.

Detection of missing async flag

Jordan mentioned on the implementation PR a concern about throwing a sync exception from asyncTest() which is otherwise an async function. (link) I agree this is unusual, but I think the case of the harness treating a test as sync when the async flag is omitted, and being able to detect that, is a good justification. Cam pointed out that not all of the implementations' test harnesses fail a test on unhandled promise rejections. (link) If they did, this would be less necessary, but solving it this way is probably easier than imposing another requirement on implementations. Therefore, I propose to keep the status quo since I think it's sufficiently motivated despite being slightly unorthodox. I'll add a paragraph about implementations' divergent behaviour on unhandled promise rejections.

Proposed conclusion: Strengthen the motivation for this detection in the RFC by mentioning implementations' current behaviour.

Next steps

I realized the draft process in #3525 could be a bit clearer about what exactly the process is for signing off in order to enter FCP. Here's what I propose. For RFCs in the Rust community, the process is shepherded by a bot which creates a checklist of maintainers, and automatically starts the FCP timer once a majority have checked off their names in the list, and no more than two remain outstanding. For the Rust process the remaining signoffs aren't needed to merge the RFC, but since we already have a process of "consensus minus one" in the maintainers group, it makes sense to me that once we enter FCP, if there is more than one signoff still needed, we need to make an extra effort to track down at least one before FCP ends.

I'll make the changes proposed above, soon. If by that time I haven't heard any objections then I can make a checklist and describe the "move to FCP" part of the process in better detail in #3525!

[jugglinmike]

Thanks for the thorough write-up, @ptomato! For the most part, it seems to accurately reflect everyone's intentions and the path forward. There is a discrepancy that we ought to consider, though: the prevalence of the .then($DONE, $DONE) pattern.

Earlier in this thread, @Ms2ger reported that the pattern exists in only ~20 tests. That made any harm from introducing an alternate pattern seem negligible, and it also made rewriting the existing uses seem trivial. However, today in the Test262 Matrix channel, @cjtenny shared evidence that the number is a couple orders of magnitude larger:

cjtenny
I don't remember how we got to the idea that there were only a handful of .then($DONE, $DONE) invocations:

~/repos/test262$ grep -HErn 'then\(\$DONE, \$DONE\)' test/ | cut -d : -f 1 | sort | uniq | grep -v 'test/harness/' | wc -l
5276

@ptomato does the prevalence of the older pattern factor in to your valuation of a new one? The code review will certainly be more onerous!

[cjtenny]

Hey @jugglinmike , sorry for the delay - I wanted to respond in a bit more detail.

First to clear up the misunderstanding: @Ms2ger clarified to me that he'd looked specifically for the pattern of an IIFE followed by that invocation. There are many fewer cases of that.

Second, I realized many of the ~5000 invocations were procedurally generated from templates & cases in src/. There are only about ~1000 actual unique files that invoke .then($DONE, $DONE).

I did more work characterizing other cases in test262 to understand a good way forward. Many of the invocations would be poor candidates for asyncTest, because they use $DONE in several places or have specific chaining and checking of promise threading, even if they ultimately invoke .then($DONE, $DONE) at some point.

I decided to refactor all the 'basic' cases I could find - about 50 files, 4 of them leading to another 20 generated cases. I started trying to refactor all the other 'simple' cases. For example, a 'simple' case might look like this:

// near the end of the test
someAsyncFunction(/*{ args }*/).then(value => {
  assert.sameValue(value, 1, "called once");
}).then($DONE, $DONE);

But many quickly start to look like:

someAsyncFunction(/* {args }*/).then({retFn} => {
  assert.sameValue(retFn.constructor, C);
  retFn().then(() => {
    throw new Test262Error("Should not resolve");
  }, (error) => {
    assert.sameValue(...);
   ...
  }).then($DONE, $DONE);
}).catch($DONE);

There are also many tests (calling, eventually, .then($DONE, $DONE)) that implement ad-hoc, incomplete versions of assert.throwsAsync which would require more involved refactoring to use assert.throwsAsync, but which further demonstrate the utility it will have going forward.

I've updated the implementation PR with the refactoring of the simplest cases to use asyncTest. I also included a suggestion in CONTRIBUTING.md to use asyncTest.

I think refactoring these cases also strikes a good balance of illustrating the clear way for simple cases, while not setting the precedent that we need to exhaustively refactor tests for new language features or harness features. I also think the benefit of asyncTest includes that you don't have to consider all the confusing possible ways to write asyncTests that I found in the code base; I included the note in CONTRIBUTING.md to balance the problem of people maybe looking at arbitrary older tests as examples.

Overall, @jugglinmike , does that address your concerns? If you're okay with it and give a 👍 , I'd like to continue with the previous plan discussed in the maintainers' meeting to move this to final comment as soon as I hear from you. Otherwise, what do you think we should do?

[jugglinmike]

Sounds good to me!

[ptomato]

This was signed off for final comment period (pending Mike's final look, which has occurred) in the maintainers meeting of 2023-01-05, with a disposition of "merge".

As per the draft process that specifies 10 calendar days, final comment period ends 2023-01-19 12:00 Pacific time.

[cjtenny]

As the final comment period has closed, I think we can merge this :) and if there are no further comments on #3728 , too, we can merge that and push tests ready for review based upon it!