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

Tests for requestIdleCallback deadline computation #31177

Merged
merged 11 commits into from Apr 13, 2022
Merged

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Oct 10, 2021

Deadline should be:

  • capped at 50ms
  • capped at 1000/60 (~16ms) when there is a pending rAF callback
  • capped at the time of the next timeout
  • be updated when the above conditions change, during the callback itself

See whatwg/html#7166

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests for this!!

<script>

promise_test(async () => {
for (let i = 0; i < 10; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10 times?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to make it a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the right way to accomplish this.
The problem with deadlines is that they are "maximum" values, meaning that the UA can always decide to make them lower. So the tests check that they are below the cap, but that can happen by accident. By testing several time I try to reduce accidental passes, and catch cases where the deadline is greater than it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be worthwhile to ask testing folks? (they used to be on the W3C #testing IRC channel, I'm not 100% sure that's still the case - @foolip may know)

Copy link
Member

Choose a reason for hiding this comment

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

We're now on Matrix at https://app.element.io/#/room/%23wpt:matrix.org

For a case like this where it's possible to accidentally pass, this approach of trying many times to provoke the bug is a pattern I've seen before and I don't really have a better suggestion. To do better would require some additional control over the runtime, and usually that's not worth the effort to add.

It's an unfortunate fact that tests like this will be flaky failures when the bug exists. I don't know how to improve on that.

An elaborate comment on why it's like this and how to understand a failure would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea this is not easy because those deadlines are capped by the spec, but they can always be shorter, so testing several times can only reduce flakiness but not eliminate it, in case of a bug.

Added a comment

requestidlecallback/deadline-max-rAF-dynamic.html Outdated Show resolved Hide resolved
requestidlecallback/deadline-max-timeout.html Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Nov 18, 2021

The corresponding HTML spec PR has been merged.
@yoavweiss @foolip, something else missing?

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM from my perspective

noamr and others added 7 commits April 13, 2022 16:34
Deadline should be:
- capped at 50ms
- capped at 1000/60 (~16ms) when there is a pending rAF callback
- capped at the time of the next timeout
- be updated when the above conditions change, during the callback itself

See whatwg/html#7166
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
@noamr noamr merged commit 6ae31b2 into master Apr 13, 2022
@noamr noamr deleted the requestIdleCallback branch April 13, 2022 14:46
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request Apr 27, 2022
…s#31177)

* Tests for requestIdleCallback deadline computation

Deadline should be:
- capped at 50ms
- capped at 1000/60 (~16ms) when there is a pending rAF callback
- capped at the time of the next timeout
- be updated when the above conditions change, during the callback itself

See whatwg/html#7166

Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants