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

Cleanup more cases of passing async function to `async_test` #25517

Merged
merged 3 commits into from Sep 16, 2020

Conversation

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Sep 14, 2020

See #21435


This CL should be rebase-merged (the individual commits are standalone)

@stephenmcgruer stephenmcgruer force-pushed the smcgruer/async_async branch from 2cc306f to f593731 Sep 14, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 14, 2020 Inactive
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/async_async branch from f593731 to 938dab6 Sep 14, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 14, 2020 Inactive
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/async_async branch from 938dab6 to 10aa301 Sep 14, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 14, 2020 Inactive
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/async_async branch from 10aa301 to f72a78f Sep 14, 2020
@stephenmcgruer stephenmcgruer marked this pull request as ready for review Sep 14, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 14, 2020 Inactive
@annevk
annevk approved these changes Sep 15, 2020
Copy link
Member

@annevk annevk left a comment

Is the plan to make this throw directly with a suggestion to use promise_test instead? (Should have checked OP.)

@foolip
foolip approved these changes Sep 15, 2020
Copy link
Member

@foolip foolip left a comment

Looks good, have some nits!

test.done();
}, {buffered: true});
observer.observe();
return new Promise(resolve => {

This comment has been minimized.

@foolip

foolip Sep 15, 2020
Member

await here to avoid .then

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 15, 2020
Author Contributor

Wait, you can grab the results of the resolve too? :O

That's super cool, I wonder if we should update https://web-platform-tests.org/writing-tests/testharness-api.html?highlight=promise_test#promise-tests (yet again) to suggest await-ing rather than then-ing for this trick. (see the 'Note that in the promise chain constructed ...' section)

This comment has been minimized.

@foolip

foolip Sep 15, 2020
Member

I would definitely suggest updating those docs. I get the impression they were written before async/await were widespread enough to be depended upon.

reporting/bufferSize.html Outdated Show resolved Hide resolved
let disconnectPromise = test_driver.generate_test_report("Test message.")
.then(() => { observer.disconnect(); });

return Promise.all([observerPromise, disconnectPromise]);

This comment has been minimized.

@foolip

foolip Sep 15, 2020
Member

Was this test broken, or why does it need to be changed to run these two bits in parallel? The overall test would read better if just using await.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 15, 2020
Author Contributor

I'm not sure how you could do this with await. I'd love your help to figure that out. I had real trouble picking the expected flow of this test apart originally, and a Promise.all was the only way I could get it to work.

This comment has been minimized.

@foolip

foolip Sep 15, 2020
Member

I'd ask Paul, but looking at the original it looks to me like the first bit is setting up the stuff that matters to the test, and the test_driver call is just there to generate the reports.

So there is an order dependency, but one has to create the observer first.

I'd first create a reportsPromise, and await that after the disconnect.

Promise.all works and there is no raciness or anything to worry about, but it is harder to tell what the test does with it, I think.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 15, 2020
Author Contributor

Right, I see. Ok, I made the ordering explicit, but in doing so I actually discovered something I didn't expect - if the assert fails then I get a harness level error.

I need to dig into why, but for now I've uploaded a variant with a deliberately failing error to show this off.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 16, 2020
Author Contributor

Ah, got it. This is a nasty case.

The ReportingObserver's callback is actually called during the call to test_driver.generate_test_report, not afterwards when we are await-ing on observerPromise. This places it in a context where testharness.js can't really 'see' it (I'm not very sure why, I find testharness.js very hard to reason about), so the error from the assert_false propagates to the unhandledrejection handler and the harness is set to ERROR.

Wrapping the then in a t.step_func 'works' but is very unintuitive for the reader (imo):

diff --git a/reporting/disconnect.html b/reporting/disconnect.html
index ffbfe5a8e9..3907740181 100644
--- a/reporting/disconnect.html
+++ b/reporting/disconnect.html
@@ -12,11 +12,11 @@ promise_test(async test => {
     let observerPromise = new Promise(resolve => {
       observer = new ReportingObserver(resolve);
       observer.observe();
-    }).then(reports => {
+    }).then(test.step_func(reports => {
       assert_false(true, 'Testing what happens if this assert fails')
       assert_equals(reports.length, 1);
       assert_equals(reports[0].body.message, "Test message.");
-    });
+    }));
 
     // The observer should still receive this report even though disconnect()
     // is called immediately afterwards.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 16, 2020
Author Contributor

It's roughly equivalent to the following I think.

    let firstResolve;
    let promiseA = new Promise(resolve => {
      firstResolve = resolve;
    }).then(reports => {
      assert_false(true, 'Testing what happens if this assert fails')
    });

    await new Promise(resolve => {
      setTimeout(() => {
        firstResolve();
        setTimeout(() => {
          resolve();
        }, 100);
      }, 100);
    });

    await promiseA;

I don't have a great solution to this other than to use t.step_func and document it clearly. Any thoughts @foolip ?

This comment has been minimized.

@foolip

foolip Sep 16, 2020
Member

What you ran into in 52af27e was almost certainly an unhandled promise rejection, because nothing was awaiting the result of that promise, the test function was still awaiting the test_driver call.

If you make yourself a reportsPromise which can never reject but only resolves with the reports, and put the asserts after const reports = await reportsPromise, this problem should go away.

But I admit this is a case where using Promise.all makes it obvious that promise rejections at any time are handled.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 16, 2020
Author Contributor

Ah, right, that makes sense.

I've switched to your suggestion; I think it is slightly clearer in terms of the expected ordering (though of course its technically more complex, observerPromise will actually be resolved as part of test_driver.generate_test_report but we still need to await it to get the results).

reporting/disconnect.html Show resolved Hide resolved
webcodecs/video-track-reader.html Show resolved Hide resolved
webcodecs/video-track-reader.html Outdated Show resolved Hide resolved
webcodecs/video-track-reader.html Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 15, 2020 Inactive
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer left a comment

(Changes for most of the comments are in the latest commit. I'll rebase them into the set of commits after all comments are solved and I have final approval)

There's one comment left to resolve, need help for it :D

test.done();
}, {buffered: true});
observer.observe();
return new Promise(resolve => {

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 15, 2020
Author Contributor

Wait, you can grab the results of the resolve too? :O

That's super cool, I wonder if we should update https://web-platform-tests.org/writing-tests/testharness-api.html?highlight=promise_test#promise-tests (yet again) to suggest await-ing rather than then-ing for this trick. (see the 'Note that in the promise chain constructed ...' section)

reporting/bufferSize.html Outdated Show resolved Hide resolved
let disconnectPromise = test_driver.generate_test_report("Test message.")
.then(() => { observer.disconnect(); });

return Promise.all([observerPromise, disconnectPromise]);

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Sep 15, 2020
Author Contributor

I'm not sure how you could do this with await. I'd love your help to figure that out. I had real trouble picking the expected flow of this test apart originally, and a Promise.all was the only way I could get it to work.

webcodecs/video-track-reader.html Show resolved Hide resolved
webcodecs/video-track-reader.html Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 15, 2020 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25517 Sep 16, 2020 Inactive
@foolip
foolip approved these changes Sep 16, 2020
Copy link
Member

@foolip foolip left a comment

LGTM % naming nit

reporting/disconnect.html Outdated Show resolved Hide resolved
@stephenmcgruer stephenmcgruer force-pushed the smcgruer/async_async branch from 035e6cf to 78c6ba0 Sep 16, 2020
@stephenmcgruer stephenmcgruer merged commit e0020b8 into master Sep 16, 2020
30 checks passed
30 checks passed
detect-deployment
Details
Azure Pipelines Build #20200916.52 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 (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
download-firefox-nightly Community-TC (pull_request)
Details
infrastructure/ tests Community-TC (pull_request)
Details
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
resources/ tests Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
tools/ integration tests (Python 2) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
tools/ unittests (Python 2) Community-TC (pull_request)
Details
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
tools/ unittests (Python 3.8) Community-TC (pull_request)
Details
update-built Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@stephenmcgruer stephenmcgruer deleted the smcgruer/async_async branch Sep 16, 2020
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

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