Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RFC for wait_for[_callback] methods #56

Merged
merged 5 commits into from Jun 12, 2020
Merged

Add RFC for wait_for[_callback] methods #56

merged 5 commits into from Jun 12, 2020

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Jun 1, 2020

No description provided.

rfcs/wait_for.md Outdated Show resolved Hide resolved
rfcs/wait_for.md Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jun 2, 2020

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.

@Ms2ger Ms2ger changed the title Add RFC for wait_for[_condition] methods Add RFC for wait_for[_callback] methods Jun 2, 2020
Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

rfcs/wait_for.md Outdated

To encourage this pattern we introduce two new methods on `Test`:

`wait_for_callback(cond, callback, description, timeout=3000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed: wait_for_callback sounds like it waits for a callback to be called. Not sure what would be better… callback_wait_for, maybe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought, but also don't know what to call it instead. Maybe something involving "step" to make it clear (?) that the callback will be wrapped in a step func automatically. t.wait_for_step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also don't know what's really better. callback_wait_for perhaps, although it's a bit weird. step seems a bit weird because both this and the promise variant are implied steps at least when running the condition function. More verbose names would be like step_wait_cond step_wait_cond_and_callback. I guess if we dropped _cond it wouldn't be too bad? step_wait and step_wait_and_callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I suppose we could assume that all modern browser devs and other test contributors should be familiar with Rust and so use step_wait and step_wait_and_then for the promise/callback versions respectively? (that's a serious proposal despite the non-serious assumption ;)

@annevk
Copy link
Member

annevk commented Jun 8, 2020

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
Copy link
Contributor Author

jgraham commented Jun 8, 2020

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
Copy link
Contributor

Ms2ger commented Jun 9, 2020

step_wait and step_wait_and_then are fine by me.

@annevk
Copy link
Member

annevk commented Jun 9, 2020

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
Copy link
Contributor Author

jgraham commented Jun 9, 2020

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
Copy link
Member

annevk commented Jun 9, 2020

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.

Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the current proposal to introduce three APIs?

step_wait - returns a Promise
step_wait_func - returns void, calls a callback
step_wait_func_done - returns void, optionally calls a callback, after calling the callback it calls done()?

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, e.g.

step_wait(cond_func, description, timeout=3000, interval=100, callback=null)

  • Returns a promise if callback=null, otherwise returns void and calls the callback. EDIT: Actually we could just always return a promise here, but also call the callback if its non-null.

step_wait_done(cond_func, description, timeout=3000, interval=100, callback=null)

  • Always returns void, but calls the callback too before calling done() if callback is non-null.

* Features like this and `step_timeout` could be useful in support
files, but using testharness.js features from support files is
tricky.
* This RFC doesn't propose a lint to flag uses of `step_timeout` /
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misunderstand this risk, but we already have a lint for setTimeout: https://github.com/web-platform-tests/wpt/blob/master/tools/lint/rules.py#L383 . We could change this lint to encourage wait_for instead, with step_timeout as a fallback if necessary.

A harder question is whether to or how to lint step_timeout of course, since that may be necessary more often (or do we strongly feel you should usually be using wait_for?)

rfcs/wait_for.md Outdated Show resolved Hide resolved
Co-authored-by: Stephen McGruer <smcgruer@chromium.org>
@jgraham
Copy link
Contributor Author

jgraham commented Jun 9, 2020

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
Copy link
Contributor

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
Copy link
Member

annevk commented Jun 11, 2020

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

@jgraham
Copy link
Contributor Author

jgraham commented Jun 12, 2020

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

@jgraham jgraham merged commit 9f60b96 into master Jun 12, 2020
@Ms2ger Ms2ger deleted the wait_for branch June 12, 2020 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants