`async_test` with async function produces undesirable results #21435

stephenmcgruer · 2020-01-27T14:37:11Z

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.

The text was updated successfully, but these errors were encountered:

foolip · 2020-01-27T17:09:07Z

test already checks for this:

wpt/resources/testharness.js

Lines 550 to 562 in 1e489ab

    
           if (value !== undefined) { 
        
               var msg = "Test named \"" + test_name + 
        
                   "\" inappropriately returned a value"; 
        
               try { 
        
                   if (value && value.hasOwnProperty("then")) { 
        
                       msg += ", consider using `promise_test` instead"; 
        
                   } 
        
               } catch (err) {} 
        
               tests.status.status = tests.status.ERROR; 
        
               tests.status.message = msg; 
        
           }

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 · 2020-05-28T01:10:08Z

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 · 2020-05-28T11:47:29Z

@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 · 2020-05-28T23:24:15Z

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

frehner · 2020-05-31T15:41:00Z

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:

wpt/html/browsers/browsing-the-web/navigating-across-documents/anchor-fragment-form-submit-withpath.html

Lines 19 to 34 in c99723a

    
           async_test(async t => { 
        
             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'); 
        
                 t.done(); 
        
               } 
        
             }); 
        
           });

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 · 2020-06-01T13:14:12Z

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 · 2020-06-01T14:23:52Z

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

Thanks.

frehner · 2020-06-05T23:33:18Z

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 · 2020-07-15T13:06:32Z

@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 · 2020-07-15T15:29:01Z

Not currently working on it, no. :)

foolip · 2020-08-17T08:32:55Z

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).

See #21435

…st in webrtc/, a=testonly Automatic update from web-platform-tests Fix tests that return values to async_test in webrtc/ (#25719) These tests are written correctly, they just need to not return the promises they use as part of the flow. They could be rewritten to use promise_tests if desired, but that would have performance implications (as it changes them from concurrent to sequential) so leaving that for now. See web-platform-tests/wpt#21435 -- wpt-commits: db9d339c441f03b2b89b00bce3662fb6e043c6fb wpt-pr: 25719

…tion to `async_test`, a=testonly Automatic update from web-platform-tests Convert async_test to promise_test in webcodecs/video-track-reader.html See web-platform-tests/wpt#21435 -- Convert await-using async tests to promise_test in reporting/ See web-platform-tests/wpt#21435 -- Fix a few cases of await-using async_test in css/ and html/ See web-platform-tests/wpt#21435 -- wpt-commits: 33e3f6e95100e5d409099ecbdbeb07c0827b0058, 80ccc18f3f5b1e0fc933d8ce806ddbf5475ad2e1, e0020b8951abeb3b1c809b1073712a96ce037e7b wpt-pr: 25517 UltraBlame original commit: 883c461469ed232bf63951c5579259fa4cb71569

…e promise_test, a=testonly Automatic update from web-platform-tests Change some async_tests in webrtc/ to use promise_test (#25659) See web-platform-tests/wpt#21435 -- wpt-commits: bed7a2397d50c90694d2ab67b9cf33c274c2e75a wpt-pr: 25659 UltraBlame original commit: b89ec30a8daf57b40e7079b633b0d196e3ad4fc7

…ation/ to use promise_test, a=testonly Automatic update from web-platform-tests Change some async_tests in screen_enumeration/ to use promise_test (#25660) See web-platform-tests/wpt#21435 -- wpt-commits: 316892ab1896f95e9548a63ae2ff84c1120f30af wpt-pr: 25660 UltraBlame original commit: 5e96ed0fcca517d4d8b6f97555154fabb01d71ff

…ar-window-name.https.html, a=testonly Automatic update from web-platform-tests Change async_test to promise_test in clear-window-name.https.html (#25572) These all appear to be straight-forward promise tests, not async tests. See web-platform-tests/wpt#21435 -- wpt-commits: d8e451a277b65aeb573845bbbd900a6cb87e1798 wpt-pr: 25572 UltraBlame original commit: 46df596cc6b9d310c3f6894c9bb7c8b455b6f239

…st in encrypted-media/, a=testonly Automatic update from web-platform-tests Fix tests that return values to async_test in encrypted-media/ (#25717) In both cases here, the test was correctly written for async_test, they just didn't need to return the promise they used as part of the test flow. See web-platform-tests/wpt#21435 -- wpt-commits: d0804458e8797790f61efc73ab2446c6e507a30f wpt-pr: 25717 UltraBlame original commit: abc4def732890fbde3883f10356af6a255958062

…st in webrtc/, a=testonly Automatic update from web-platform-tests Fix tests that return values to async_test in webrtc/ (#25719) These tests are written correctly, they just need to not return the promises they use as part of the flow. They could be rewritten to use promise_tests if desired, but that would have performance implications (as it changes them from concurrent to sequential) so leaving that for now. See web-platform-tests/wpt#21435 -- wpt-commits: db9d339c441f03b2b89b00bce3662fb6e043c6fb wpt-pr: 25719 UltraBlame original commit: 5f39aad86e2e243034fef317378cf1c9f094c6b4