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

[testharness.js] Implement promise_setup #20187

Merged
merged 2 commits into from Nov 22, 2019

Conversation

jugglinmike
Copy link
Contributor

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:

promise_test(async () => {}), 'a');
promise_setup(async () => {});
promise_test(async () => {}), 'b');

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.

@foolip
Copy link
Member

foolip commented Nov 9, 2019

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.

@zcorpan
Copy link
Member

zcorpan commented Nov 18, 2019

@jugglinmike I think it seems weird to allow setup after tests have been created. Maybe we should make that a harness error?

Copy link
Member

@gsnedders gsnedders left a 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.

@jugglinmike
Copy link
Contributor Author

@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:

wpt/resources/testharness.js

Lines 2478 to 2482 in 295f22b

Tests.prototype.setup = function(func, properties)
{
if (this.phase >= this.phases.HAVE_RESULTS) {
return;
}

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 setup for this patch.

We could discuss implementing a harness error separately, either as a precursor to this change or as a follow-up.

@zcorpan
Copy link
Member

zcorpan commented Nov 20, 2019

Follow-up SGTM. I've filed #20336

@wpt-pr-bot wpt-pr-bot requested a deployment to wpt-preview-20187 November 20, 2019 21:10 Abandoned
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a 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).

docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
resources/test/tests/unit/promise_setup.html Show resolved Hide resolved
resources/test/tests/unit/promise_setup.html Show resolved Hide resolved
});
}, 'Error for subsequent invocation of `async_test`');

promise_test(() => {
Copy link
Contributor

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.

resources/testharness.js Show resolved Hide resolved
@zcorpan
Copy link
Member

zcorpan commented Nov 22, 2019

I'll call this reviewed enough now. 🙂

@zcorpan zcorpan merged commit 783959b into web-platform-tests:master Nov 22, 2019
@zcorpan zcorpan deleted the testharness-promise_setup branch November 22, 2019 14:12
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

7 participants