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.

Sign up for GitHub

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

Jump to bottom

RFC 207: step_wait_promise method #207

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

RFC 207: step_wait_promise method #207

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc29412
RFC step_wait_promise
sadym-chromium Aug 7, 2024
b9d6be8
update RFC number
sadym-chromium Aug 7, 2024
85b235a
add description
sadym-chromium Aug 7, 2024
66be6c7
describe promise rejection
sadym-chromium Aug 14, 2024
6810269
Update rfcs/step_wait_promise.md
sadym-chromium Aug 15, 2024
1a3c18b
lines
sadym-chromium Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions rfcs/step_wait_promise.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# RFC 207: `step_wait_promise` method

## Summary

Add a `step_wait_promise(promise, description, timeout=default_timeout)` method to
sadym-chromium marked this conversation as resolved.
Show resolved Hide resolved
the wpt `Test` object. This method waits for the given `promise` to resolve within
the specified `timeout`. If the promise is not resolved within the timeout period, it
raises an assertion error: `Timed out waiting on condition`.

## 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.

* `promise`: The Promise object to wait for.
* `description`: Error message to add to assert in case of failure.
* `timeout`: Timeout in ms. This is multiplied by the global
`timeout_multiplier`. Defaults to `step_wait_*` default of 3000ms.
* Behavior:
* The function waits for the given `promise` to resolve.
* If the `promise` resolves within the `timeout`, the function returns the
resolved value of the promise.
* If the `promise` does not resolve within the `timeout`, an `AssertionError`
is raised with the "Timed out waiting on condition", `description` and
stacktrace.

## Motivation

* Support for BiDi events: Facilitates testing scenarios involving BiDi events
in the testdriver API, where asynchronous events are expected.
* Improved efficiency compared to `step_wait`: Eliminates the need for periodic
checks with intervals, allowing the test to proceed immediately once the awaited
event occurs, potentially leading to faster test execution. Event though interval
sadym-chromium marked this conversation as resolved.
Show resolved Hide resolved
of
100ms seems to be small enough, it can still accumulate to a significant amount of
time.

## Examples

In [console/console-count-logging.html](https://github.com/web-platform-tests/wpt/blob/ea3f611e8d3e7debc3b9cb98b0e63254657fa7eb/console/console-count-logging.html#L33),
`log_entries_promise` is awaited without a wrapper, which in case of not having the
required events within the test timeout, the test will fail with a generic timeout
hiding the actual step and stacktrace.

### Before:

```javascript
// Wait for the log entries to be added.
const log_entries = await log_entries_promise;
```

In case of not having the required events, the test will crash with a generic
timeout error:

```
/console/console-count-logging.html
TIMEOUT Console count method default parameter should work - Test timed out
TIMEOUT /console/console-count-logging.html
```

### After

```javascript
// Wait for the log entries to be added.
const log_entries = await test.step_wait_promise(log_entries_promise,
"Wait for the log entries to be added.");
```

In case of not having the required events, the test will crash with a generic
timeout error:

```
/console/console-count-logging.html
FAIL Console count method default parameter should work - step_wait_promise: Wait for the log entries to be added. Timed out waiting on condition
at async Test.<anonymous> (http://web-platform.test:8000/console/console-count-logging.html:33:29)
```

## Risks

* Adding API surface: Increases the complexity of the testing framework and requires
ongoing maintenance.

## Considerations

* The implementation should respect the "timeout multiplier" setting to ensure
consistency with other timeout-related behaviors in the testing framework and to
work
correctly in the presence of slow or overloaded systems.