RFC 75: async_promise_test

ArthurSonzogni commented 3 years ago

The parallel version of promise_test(). Test can run multiple of them without worrying about timeout.

ArthurSonzogni commented 3 years ago

In particular, I would be interested getting @stephenmcgruer opinion. This was previously discussed here and here. +CC @mikewest / @antosart FYI.

zcorpan commented 3 years ago

I agree with the need for this. Having different tests implement this functionality in their own way isn't great.

No strong opinion on naming. We would have:

test() // executed directly, implicitly done when the function returns.
async_test() // executed directly, explicitly marked done, allows parallel execution with other tests.
promise_test() // waits for promise_setup and previous promise_tests to complete before starting.
promise_test_parallel() // waits for promise_setup to complete (?), allows parallel execution with other tests.

annevk commented 3 years ago

async_promise_test perhaps?

stephenmcgruer commented 3 years ago

In particular, I would be interested getting @stephenmcgruer opinion. This was previously discussed here and here.

I apologize - I apparently completed missed that question to me previously! I'm honestly not sure how, so I can only say sorry.

I like this proposal (I've often wished that test was just promise-based by default, and that it was async by default, but that would be a lot of compat-style work to make that happen...), and I like annevk's name for it offhand. I agree that we should think about how it is implemented a bit; the test-defined version is cute but would not fit into testharness.js (for a start, it will throw if promise_setup is used, and a naive implementation of async_promise_test wouldn't work with promise_setup).

Let me think about this a bit over the next few days. If anyone is motivated to make a prototype that would be amazing, otherwise I will try to sink some time into that (but really cannot promise for at least a few weeks).

cc @web-platform-tests/wpt-core-team to help make sure folks have seen this.

ArthurSonzogni commented 3 years ago

Thanks! Happy you like this proposal.

I like @annevk suggestion: async_promise_test. Then we can represent the 4 kind of tests this way:

	in sequence	in parallel
Function	test	async_test
Promise	promise_test	async_promise_test

I can make a prototype if you want, when I find some free time. Not soon, but there's no hurry.

jgraham commented 3 years ago

FWIW I agree with this in principle, and think that the async_promise_test name is probably the best option even though async turns out to be an unfortunate prefix these days due to confusion related to async function(){}. I also agree that there needs to be some detail on how this will interact with the other parts of the API, and setup in particular, in order to approve the RFC.

ArthurSonzogni commented 3 years ago

I also agree that there needs to be some detail on how this will interact with the other parts of the API, and setup in particular.

In the PR: https://github.com/web-platform-tests/wpt/pull/27916

I added async_test() and async_promise_test() to be deferred until the execution of every promise_setup() defined earlier. This was previously the case only for promise_test(). Doing this for async_test() allows async_promise_test() to share the same implementation. I think it makes sense. WDYT?

stephenmcgruer commented 3 years ago

Sorry Arthur, I should have looked at your PR long before now and I failed to do so. I've moved it to the top of my list for tomorrow. Thanks for your efforts here and sorry again 😢 .

ArthurSonzogni commented 3 years ago

No problems. This can take as long as it needs. I am not in a hurry.

foolip commented 3 years ago

At the risk of opening up an issue that others already consider settled, I do have a comment about the table in https://github.com/web-platform-tests/rfcs/pull/75#issuecomment-781201853, which was this:

	in sequence	in parallel
Function	test	async_test
Promise	promise_test	async_promise_test

I think the inclusion of the synchronous test has hidden that there's actually a missing async variant, and if we ever wanted to add it it's not clear what to name it given the precedent.

If we set aside test as synchronous tests, of which there are no variants, there could be 4 async variants:

	in sequence	in parallel
Callback	N/A	async_test
Promise	promise_test	this RFC

If we had a blank(er) slate and longer lines, I'd probably suggest:

	in sequence	in parallel
Callback	callback_test	callback_test_parallel
Promise	promise_test	promise_test_parallel

However, migrating from async_test to the verbose callback_test_parallel would not be great.

Still, I do think that the originally proposed promise_test_parallel is better than async_promise_test which suggests a connection to async_test which I can't see.

Do others feel strongly about this?

annevk commented 3 years ago

The connection to async_test is that async_test and async_promise_test run in parallel and not in sequence. I think you're correct that there's a missing variant, but I don't think that argues for a different naming scheme, unless async_test was also renamed. (And I don't think we'd ever add the missing variant, as promise_test is fine for that.)

foolip commented 3 years ago

@annevk I agree that there's not much point in adding the missing variant, that'd be very niche. (And some tests make async_test sequential by just defining one test at the end of another.)

Still, I take the "async" in async_test to mean just "asynchronous". These tests do run in parallel, but other test frameworks have async tests that run sequentially, and it doesn't show up in the naming.

Aside: async is also unfortunate given that it's now a JS keyword, and things like async_test(async () => { ... }) actually don't work, but the proposed async_promise_test(async () => { ... }) would. I do think we should consider renaming it to callback_test or something, but not as part of this RFC.

I think the original promise_test_parallel was OK, if a bit long. Some names seem inevitable if we want to consistently encode both the "promise" and "parallel" aspects. But how about just parallel_test, would that not work?

foolip commented 3 years ago

In addition to naming, there's a behavior I think we should be deliberate about. As it turns out, the following currently works:

promise_test((t) => {
  setTimeout(t.step_func_done(), 1000);
  return new Promise(() => {}); // never resolves
});

In other words promise_test can be ended by invoking t.done(). Should we propagate this behavior to the new parallel variant?

Friday night musing

I have sometimes wished that test(async () => { ...}) could just work, but as @stephenmcgruer that would require a lot of work, and I know that @jgraham has been skeptical of making behavior depend on the arguments or what the callback returns.

But still, supporting both of these patterns seems technically straightforward:

parallel_test(async () => {
  await things;
  assert_things();
});

parallel_test(t => {
  callback_things(t.step_func_done(() => {
    assert_things();
  });
});

The debatable thing about it is going to be checking if the return value of the initial callback is a thenable and letting that be a fork in the behavior. Maybe this is a bad idea unless we also do it for test.

ArthurSonzogni commented 3 years ago

In addition to naming, there's a behavior I think we should be deliberate about. As it turns out, the following currently works:
promise_test((t) => {
  setTimeout(t.step_func_done(), 1000);
  return new Promise(() => {}); // never resolves
});

I don't have a strong opinion. I just added unit_tests for both async_promise_test and promise_test. They are both supporting this. I believe it is good to be consistent at least.

foolip commented 3 years ago

@gsnedders, @jgraham and me discussed next steps for this last week. I seem to be alone in wanting to avoid async in the name, or at least it should be fixed together with a plan to migrate the name of async_test.

It would be great to understand two things via instrumented runs:

Whether promise_setup is ever used together with async_test currently? That should cause a harness error, but it might occur in the test suite anyway.
Whether promise_tests are currently ever ended by calling t.done(). If this reveals mostly mistakes and few/no legitmate uses of it, then perhaps it shouldn't be extended to async_promise_test.

domenic commented 2 years ago

I would have liked to use this today.

ArthurSonzogni commented 2 years ago

I would have liked to use this today.

Sorry @domenic , I got discouraged ;-( I didn't expect this would take forever to land.

The implementation, test, documentation are done. What remains is mostly making decisions about / or investigating @foolip questions above. I would be happy if someone wants to take over the work.

One issue that I observed: successful assertions aren't attributed to the right test if they aren't wrapped inside step_func. Only the failures are correctly attributed. That's not a big deal for me. There are already many tests using async_promise_test or promise_test_parallel in WPTs. I guess in the meantime, you can define it yourself, like others are doing:

  let async_promise_test = (promise, description) => {
    async_test(test => {
      promise(test)
        .then(() => {test.done();})
        .catch(test.step_func(error => { throw error; }));
    }, description);
  };

foolip commented 1 year ago

Whether promise_setup is ever used together with async_test currently? That should cause a harness error, but it might occur in the test suite anyway.

@jgraham suggested just greping for this. git grep -l promise_setup | xargs grep -l async_test shows that it doesn't happen in real tests:

docs/writing-tests/testharness-api.md
import-maps/resources/test-helper.js
resources/test/tests/unit/promise_setup.html
resources/testharness.js

Whether promise_tests are currently ever ended by calling t.done(). If this reveals mostly mistakes and few/no legitmate uses of it, then perhaps it shouldn't be extended to async_promise_test.

Let's just ignore this risk. It's not great if this happens, but we can fix it for promise_test and async_promise_test alike in the future if we're worried enough about it.

ArthurSonzogni commented 1 year ago

Awesome seeing this merged!

I got discouraged and abandonned, but now that the RFC is merged, I guess this is going to happen.

Are you expecting something from me now? Do you want me to rebase and update the associated pull request implementing this RFC: https://github.com/web-platform-tests/wpt/pull/27916 ?

jcscottiii commented 1 year ago

Hi @ArthurSonzogni. Apologies for taking so long. We hope that you are willing to update that PR.

FYI: Also, you may want to potentially try your changes on top of this PR https://github.com/web-platform-tests/wpt/pull/38806/files. While it doesn't address async_promise_test or what this RFC is trying to achieve, it may change what you expect for the existing promise_test

web-platform-tests / rfcs

RFC 75: async_promise_test #75