Wpt: testharness.js: extend `Test#add_cleanup` to honor Promises

[jugglinmike]

Upon completion, some tests must use asynchronous operations to properly
restore global state. This is the case in many service worker tests, for
instance, where ServiceWorkerRegistration#unregister must be completed before
the test is truly "done."

In its current form, Test#add_cleanup is unusable for this purpose because it
assumes synchronous behavior.

Extend that method to inspect the return value of the provided callback
function. If the value is a thenable, the test should not be considered
"complete" until that thenable settles (along with all additional thenables
specified in this way). Additional considerations:

• Waiting should occur regardless of the sub-test's result.
• All Promises should be allowed to settle, making the semantics of the
  standard Promise.all method (where any one rejection prompts overall
  rejection) inappropriate
• Waiting should be subject to the same timeout restrictions as the test body
  itself.
• Rejected promises should cause test failures, but if the test body itself has
  failed, the test error should take precedence.
• If practical, the implementation should not introduce a dependency on the
  global Promise constructor or its methods. Barring that, references to those
  APIs should only be evaluated after a callback function has returned a
  thenable.

[foolip]

@jugglinmike you did some work in https://github.com/w3c/web-platform-tests/pull/6151 but I guess there's more to be done here?

[foolip]

In https://github.com/w3c/web-platform-tests/issues/7188#issuecomment-341720370 @jakearchibald said

This would be really useful for the service worker tests. In some cases we're meaningless promise_tests to do this.

Can you link to some such cases? That would be a reason to bump the priority of this I think.

[mkruisselbrink]

Can you link to some such cases? That would be a reason to bump the priority of this I think.

https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/indexeddb.https.html is one such case (see the two nested "clean up" promise_tests inside the last test in that file).

[foolip]

OK, I'll call it roadmap, the goal being to change https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/indexeddb.https.html to use add_cleanup instead. That should drive whatever else needs to change.

[pwnall]

More test case data, to help prioritize:

1) This would help all the tests for the Async Cookies API.
2) IndexedDB tests currently call IDBFactory.delete in an add_cleanup handler and complete before the database is technically removed. While technically correct, this can result in flakiness as bots may run out of space while database deletion requests pile up. add_cleanup is so convenient that I don't consider removing the occasional to be worth the extra complexity introduced by manual cleanup. However, if add_cleanup would accept Promises, that'd be an easy fix.

[bsittler]

This additionally would make cleanup more reliable for tests that need to clean up or reset resources managed by Python handlers, e.g. HttpOnly cookies and temporary files, by making a cleanup fetch() possible (I realize blocking XHR can likely already do this in document contexts, but I'm optimistic it soon won't be able to.)

[foolip]

Good news, @jugglinmike will be coming along to try to finish this soonish.

[jugglinmike]

I'm hoping to have a patch for this ready for review some time next week. @Ms2ger will you be available to review that for me?

[Ms2ger]

Possibly. Do you need it done by the end of the year?

[jugglinmike]

Hey folks! I'm almost done with a patch for this (you can review my work-in-progress here). It's been a bit trickier than we expected, though. I'm working on a few related improvements:

• [ ] Extend testharness.js suite with explicit distinction between "unit tests" and "integration tests" (see gh-8597)
• [ ] Introduce concept of testharness.js "test variants" and implement a "no-promise" variant (not yet submitted--depends on the above)
• [ ] Allow testharness.js to be modified without invalidating existing tests (see gh-8735)
• [ ] Remove add_cleanup return value from existing tests that currently provide one (see gh-8742)

My motivation for the last change is that I would like for this patch to cause a harness error whenever a "cleanup" function returns a non-thenable value (since this may indicate a programming error that could make tests more fragile).

[jugglinmike]

Alrighty, a patch is now available for review: https://github.com/w3c/web-platform-tests/pull/8748

[foolip]

This came up in issue triage today. @jugglinmike, do you intend to keep going with https://github.com/w3c/web-platform-tests/pull/8748?

[jugglinmike]

I would love to, but at the moment my hands are quite full with https://wpt.fyi. I'll bring this up when we speak next Monday.

[gsnedders]

Downgrading to "priority:backlog" given I don't think this will happen in Q2? Unless you want to pick it (presumably the open PR in #8748) up @foolip or @lukebjerring perhaps?

[foolip]

All, it's time to celebrate, and to start using this in IndexedDB, Service Workers, and wherever else you were missing it!