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
Conversation
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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 ;)
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? |
step_wait and step_wait_and_then are fine by me. |
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. |
I think I'm OK with |
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. |
There was a problem hiding this 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` / |
There was a problem hiding this comment.
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
?)
Co-authored-by: Stephen McGruer <smcgruer@chromium.org>
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 |
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:
(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. |
For examples you could link web-platform-tests/wpt#24038 I suppose. (Though that also needs updating for the new names.) |
I think we have enough approvals here to go ahead and merge. |
No description provided.