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

RFC 207: step_wait_promise method #207

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented Aug 7, 2024

@sadym-chromium sadym-chromium changed the title RFC TODO: step_wait_promise method RFC 207: step_wait_promise method Aug 7, 2024
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 14, 2024

What happens if the promise rejects?

@sadym-chromium
Copy link
Contributor Author

What happens if the promise rejects?

The step will fail with the rejection reason

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 14, 2024

Please clarify in the rfc

@sadym-chromium
Copy link
Contributor Author

Please clarify in the rfc

Right, I missed it. Done.

@sadym-chromium
Copy link
Contributor Author

I assume the RFC is accepted. @past could you please merge?

@past
Copy link
Member

past commented Aug 27, 2024

I will give this another week, given that it was filed after our last WPT infra meeting, since many people are still out of office. I see no objections and one approval, so I expect it will be swiftly approved. I will bring it up in our meeting next Tuesday.


## Details

* **`step_wait_promise(promise, description, timeout=default_timeout)`**
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add a Test.step_wait_promise_done as well?

Test.step_wait already optionally takes a Promise as its first argument, so is this basically just:

Test.prototype.step_wait_promise(promise, description, timeout) {
    return this.step_wait_func(() => promise, description, timeout)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, this should work. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, wait. The step_wait_func does not respect the timeout of the cond:

var wait_for_inner = test_this.step_func(() => {
    Promise.resolve(cond()).then( // `step_wait_func` waits for the `cond` disregarding `timeout` argument.
        step, // Timeout is checked here
        test_this.unreached_func("step_wait_func"));
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But perhaps this can be fixed in the step_wait_func method instead.

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.

6 participants