Wpt: `async_test` with async function produces undesirable results

[stephenmcgruer]

This was reported on https://groups.google.com/a/chromium.org/forum/#!topic/ecosystem-infra/2dpo_NDYOgc. If you have a test file which passes an async function to async_test, and that async function throws an exception, the sub-test will TIMEOUT and the harness will get an exception.

For example:

async_test(async function(t) {
  throw new Error('foo');
}, 'my test');

Harness status ERROR and test status TIMEOUT

async_test(async function(t) {
  assert_precondition(false, 'foo');
}, 'my other test');

Harness status PRECONDITION_FAILED (misleading...?!) and test status TIMEOUT.

In some ways this is working as intended; nothing has told the test object that it is finished, so it is reasonable for it to still be waiting. It is a bit weird that PRECONDITION_FAILED ends up being propagated to the test harness. In either case, however, I cannot see a use for passing an async function to async_test, so perhaps we should guard against it?

Naively I think async_test could check the return type of test_obj.step( and fail in some way if its a Promise, but I don't know testharness.js that well so that may be nonsense.

[foolip]

test already checks for this:
https://github.com/web-platform-tests/wpt/blob/1e489ab1cc0e4eb9175bef495b73ff898ae54d68/resources/testharness.js#L550-L562

It would make sense for async_test to do the same. Note that making this change will likely require changing lots of tests that return a value for various harmless reasons, as was the case for test.

[frehner]

I would like to work on this if that's ok.

I (locally) applied a patch to async_test that pretty much matches the logic outlined in test, but running all the tests locally to get a list of places that need to update is taking a long time :)

Should I just wait out the test results or is there a more efficient way of finding all the places that need to be corrected? (e.g. I did a search for async_test(async and found some places, but I don't think that is comprehensive enough)

[stephenmcgruer]

@frehner that would be great! If you can create a pull request with your patch, I can set off a 'trigger' run for you which will run all the tests in the suite and should give us good coverage (see https://web-platform-tests.org/running-tests/from-ci.html). Then we can iterate on the patch to fixup the problematic tests.

EDIT: Note that I suspect that a naive grep for async_test(async will probably hit 95% of the cases, so you may want to start by tackling those. I find 87 files in WPT (some with multiple entries) that contain that string.

[frehner]

Thanks, I should be able to get a PR up around the weekend and we can go from there.

[frehner]

Forgive me for my newness / ignorance, I'm still trying to fully understand test harness's api:

I wanted to be sure I was migrating the tests in the right way; after playing around with various methods, this is the way that I've found that works - but I'm not sure if it's the "best" way.

Say we have this test:

https://github.com/web-platform-tests/wpt/blob/c99723afe33bc5b265e41c1a9446eaff7f8e0d57/html/browsers/browsing-the-web/navigating-across-documents/anchor-fragment-form-submit-withpath.html#L19-L34

The way it appears to me that it should be updated is like the following - note that I'm still using async_test but it now returns a normal function, and then I have to t.step around the async function.

async_test(t => {
  t.step(async () => {
    const iframe = document.querySelector('iframe');
    const iframeLoadPromise = new Promise(resolve => iframe.onload = resolve);
    iframe.src = '/';
    await iframeLoadPromise;

    const anchor = document.getElementById('anchor');
    anchor.href = '/#';
    t.step(() => anchor.click());
    window.onmessage = t.step_func(event => {
      if (typeof event.data === 'string' && event.data.includes('navigation')) {
        assert_equals(event.data, 'form navigation laskjdf');
        t.done();
      }
    });
  })
});

Why this way? Well, I tried changing it to promise_test with no other changes but that didn't appear to actually go into the assert_equals call. I verified that by changing the assertion to a bad one and it still "passes."

Anyway, when you have a second, if you don't mind letting me know if I'm going down the right path or if I've missed something. Thank you :)

[stephenmcgruer]

Hey @frehner, thanks for looking at this :)

The reason that test wouldn't work (as written) as a promise test is that the promise isn't going to wait for window.onmessage. So:

promise_test(async t => { 
  const iframe = document.querySelector('iframe'); 
  const iframeLoadPromise = new Promise(resolve => iframe.onload = resolve); 
  iframe.src = '/'; 
  await iframeLoadPromise;  // <--- promise 'waits' for this to finish

  const anchor = document.getElementById('anchor'); 
  anchor.href = '/#'; 
  t.step(() => anchor.click()); 
  window.onmessage = t.step_func(event => { // <-- promise doesn't wait for this!
    if (typeof event.data === 'string' && event.data.includes('navigation')) { 
      assert_equals(event.data, 'form navigation'); 
      t.done(); 
    } 
  }); 
  // <--- there's an implicit return new Promise() here, which won't wait for the window.onmessage above.
}); 

The tricky part here is that the test is written to handle multiple onmessage events, and just look for the one that matches what they are trying to assert. So trying to wrap that in a single promise chain of events is a pain.

Instead, I think we shoud keep it async. But unfortunately your solution isn't quite right - I don't think step should ever have an async function passed to it either. I need to double check this, but you can see the problem by adding a throw new Error('foo'); inside the async function and running the test. The error escapes the test framework and shows up at the top-level.

I think I would probably rewrite the test like this:

async_test(t => {
  const iframe = document.querySelector('iframe');
  iframe.onload = t.step_func(() => {
    const anchor = document.getElementById('anchor');
    anchor.href = '/#';
    anchor.click();  // NOTE: I removed the t.step() around this function, because I don't believe its doing anything.
    window.onmessage = t.step_func(event => {
      if (typeof event.data === 'string' && event.data.includes('navigation')) {
        assert_equals(event.data, 'form navigation');
        t.done();
      }
    });
  });
  iframe.src = '/';
});

[frehner]

Ah that makes sense - removing the awaits with t.step_func where possible and just removing the async func.

Thanks.

[frehner]

Ok, so, based on your feedback above, I went ahead and tried to fix another test that had this issue, and created a draft PR #24021

I'm still not very confident in my understanding of the test framework - though my confidence is growing - so I wanted to push this one up to see if you think I've done it right. If so, then I'll feel good about progressing without asking for as much feedback. I just didn't want to get really far into the PR just to find out that I've been doing it wrong the whole time.

In summary - sorry to bother you again, but how does this look so far?

[stephenmcgruer]

@frehner Hi Anthony; just wanted to check in and see if you're still working on this issue. If not, no worries, we just want to make sure it doesn't accidentally get double-work on it :)

[frehner]

Not currently working on it, no. :)

[foolip]

I keep seeing async_test(async () => { ... }) in code review, today it was https://chromium-review.googlesource.com/c/chromium/src/+/2355390 that prompted me to look at this issue again.

This is a very natural thing to attempt given the naming, and plenty of cases of it have slipped through code review.

The string "async_test(async" appears in these tests:

content-security-policy/frame-src/frame-src-same-document-meta.sub.html
content-security-policy/frame-src/frame-src-same-document.sub.html
css/cssom/CSSStyleSheet-constructable-disallow-import.tentative.html
css/selectors/focus-visible-007.html
css/selectors/focus-visible-011.html
html/browsers/browsing-the-web/navigating-across-documents/anchor-fragment-form-submit-withpath.html
html/cross-origin-embedder-policy/reporting-navigation.https.html
html/cross-origin-embedder-policy/reporting-to-endpoint.https.html
navigation-timing/nav2_test_navigate_iframe.html
navigation-timing/test_document_onload.html
reporting/bufferSize.html
reporting/disconnect.html
reporting/path-absolute-endpoint.https.sub.html
resource-timing/resource_nested_dedicated_worker.worker.js
screen-wake-lock/wakelock-onrelease.https.html
screen_enumeration/getScreens.tentative.https.window.js
screen_enumeration/isMultiScreen.tentative.https.window.js
webcodecs/video-encoder.html
webcodecs/video-track-reader.html
webrtc/RTCPeerConnection-onnegotiationneeded.html

I think we'll keep accumulating cases like this until there's a lint or runtime error that prevents it. I'm removing the "good first issue" label as this won't be straightforward due to the existing tests and needing to test run changes in downstream users of WPT (Chromium, Gecko, WebKit).

[stephenmcgruer]

There are now PRs landed or out for every async_test(async that I am aware of in WPT, and I've fixed all the cases in downstream Chromium too. So it's time to revive the work frehner did in https://github.com/web-platform-tests/wpt/pull/24021 to make it a harness error to return a promise in an async_test, and see what breaks!