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
[testharness.js] Implement promise_setup
#20187
[testharness.js] Implement promise_setup
#20187
Conversation
This enacts the change specified by WPT RFC 32 https://github.com/web-platform-tests/rfcs/blob/master/rfcs/asynchronous_setup.md
A new RFC seems excessive, just a PR to update the proposal and linking to that from the already merged PR is OK I think. |
@jugglinmike I think it seems weird to allow setup after tests have been created. Maybe we should make that a harness error? |
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.
This in principle LGTM, but I'd rather others also reviewed it.
@zcorpan It's true that after tests have been created, setup doesn't cause any exceptions or harness errors. However, it's not exactly "allowed," either. The harness silently ignores such invocations: Lines 2478 to 2482 in 295f22b
This condition seems like evidence of mistake in test design, so I agree that alerting authors via a harness error would be preferable. However, I would rather maintain parity with We could discuss implementing a harness error separately, either as a precursor to this change or as a follow-up. |
Follow-up SGTM. I've filed #20336 |
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.
Looks great! A few comments, nothing blocking (mostly my own dumb questions).
}); | ||
}, 'Error for subsequent invocation of `async_test`'); | ||
|
||
promise_test(() => { |
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.
Most ridiculous nit ever (feel free to ignore): to me this goes before the tests for subsequent invocations of test/async_test, as it shares more in common with the 'setup fails for some reason' tests.
I'll call this reviewed enough now. 🙂 |
This enacts the change specified by WPT RFC 32
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/asynchronous_setup.md
This implementation differs from the algorithm in WPT RFC 32 in one regard: the operations are deferred until the completion of any subtests which were previously defined using
promise_test
. Take the following test for example:As written in the RFC, the subtest named "a" and the setup code would run in parallel. The subtest named "b" would run following the completion of both.
In the implementation proposed here, the setup code does not run until after the subtest named "a" has completed. The subtest named "b" runs after the setup code has completed. The application of any optional setup properties is likewise deferred, so in keeping with the existing behavior of
setup
, they are ignored.My cursory search through recent test results didn't turn up any occurrences of this pattern (i.e. setup after some tests are defined), but it's hard to make a conclusive inquiry because asynchronous setup is currently accomplished in an ad-hoc way. My motivation for this divergence is more about consistency and predictability than it is about supporting existing tests.
If we agree that this is in-line with the spirit of the RFC, then I can amend the RFC accordingly. If folks feel this is too substantial a change to make in a code review, I can file a new RFC to propose the alteration.