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

jugglinmike commented Nov 8, 2019

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
Contributor

gsnedders left a comment

This in principle LGTM, but I'd rather others also reviewed it.

@jugglinmike
Copy link
Contributor Author

jugglinmike commented Nov 20, 2019

@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

@stephenmcgruer stephenmcgruer self-requested a review Nov 20, 2019
@wpt-pr-bot wpt-pr-bot requested a deployment to wpt-preview-20187 Nov 20, 2019 Abandoned
Copy link
Contributor

stephenmcgruer left a comment

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
});
}, 'Error for subsequent invocation of `async_test`');

promise_test(() => {

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Nov 20, 2019

Contributor

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
9 checks passed
9 checks passed
build-and-publish
Details
Azure Pipelines #20191121.87 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
@zcorpan zcorpan deleted the bocoup:testharness-promise_setup branch Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.