Add RFC for wait_for[_callback] methods

[annevk]

FWIW, this seems really good. Moving away from step_timeout and instead making everything use this will lead to a lot less idle time if I understand things correctly. Both when running things locally and when running things on CI as we'll no longer be bound by the lowest common denominator.

[annevk]

I've started playing with this locally and found that having wait_for_done for async_test wrappers would be useful as a way to avoid having to pass () => t.done() to wait_for_callback. Would this require a new RFC or is this a small enough amendment that I can add it and it can be accepted together with the remainder this week?

[jgraham]

I'd consider that a small enough change to be done as an addon to this RFC. AFAIK the main blocker here at the moment is some sense of consensus on API names. @Ms2ger and @foolip previously had feedback; do any of the alternatives suggested appeal to you?

[Ms2ger]

step_wait and step_wait_and_then are fine by me.

[annevk]

Maybe step_wait_func and step_wait_func_done? This would be a slight change to my proposed wait_for_done in that a callback can still be passed, but I think is more consistent with step_func and step_func_done and probably worthwhile therefore.

[jgraham]

I think I'm OK with step_wait_func and step_wait_func_done although you don't really wait on func. If you want step_wait_func_done to be useful I guess the callback has to be optional.

[annevk]

Yeah, I was thinking it would be optional. Usually you probably don't need it, but sometimes it might be useful for some additional asserts.

[jgraham]

I'm not sure if this is bad practice, but my initial feeling was to always make the callback optional and just have the behavior depend on that,

I'm not a huge fan of this kind of overloading and it seems quite unidiomatic for testharness.js in general (e.g. we have a distinction between test() and promise_test(), although the latter could be "any test() where the function returns a thenable", and we could even have said that async_test() was a test() with no function passed in since in the early days it didn't take a function; fortunately we didn't since it turned out to be better to allow an initial function).

[stephenmcgruer]

I'm not a huge fan of this kind of overloading and it seems quite unidiomatic for testharness.js in general

Ok, seems reasonable to avoid it since its unidiomatic for testharness. I personally quite like it, but outside of some literature searching I think we're doing to personal opinion there :)

I can't come up with a better name for this, so I guess this LGTM. I wonder slightly if we should be helping folks who are listening for actual events (rather than a condition change), but maybe that should be an extension to EventWatcher instead. For example:

promise_test(async t => {
  let a = create_animation();
  // This is expected to happen in 1-2 frames. If the impl is broken, it will take
  // the entire test timeout time! 
  await a.ready;  
});

(Of course then you get into the fact that technically on a very slow machine 1-2 frames could take a long time, so we shouldn't be encouraging authors to define their own timeouts and should just depend on the whole test timeout, I guess...)

Anyway, long story short, LGTM but the PR needs updated with new names (and note my comment on the risks section). It would also be great if we could give a handful of examples in the PR of where people are doing things today that would be improved by this.

[annevk]

For examples you could link https://github.com/web-platform-tests/wpt/pull/24038 I suppose. (Though that also needs updating for the new names.)

[jgraham]

I think we have enough approvals here to go ahead and merge.