1460786 - [wpt-sync] Sync PR 8748 - [testharness.js] Honor promises from cleanup fns

Status: NEW

(Testing :: web-platform-tests, defect, P4)

Description

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Sync web-platform-tests PR 8748 into mozilla-central (this bug is closed when the sync is complete).

PR: https://github.com/w3c/web-platform-tests/pull/8748
Details from upstream follow.

Mike Pennisi <[e-mail]> wrote:
>   [testharness.js] Honor promises from cleanup fns 
>  
>  Extend the implementation of the `Test#add_cleanup` method to detect
>  cases where the provided function returns a "thenable" object. In such
>  cases, defer test completion until all such eventual values settle. If
>  any eventual value is rejected, report a harness error (as this
>  represents an unpredictable state that may invalidate subsequent tests).
>  
>  This behavior prevents tests from being run synchronously. In order to
>  promote predictability in test harness behavior (and to avoid
>  invalidating existing tests which rely on synchronicity), only expose
>  the new behavior for tests defined using the `promise_test` API (as
>  these tests were originally implemented to run in separate turns of the
>  event loop).
>  
>  Introduce `Tests#set_status` in order to de-couple status setting from
>  remote context closing. This is necessary because uncaught errors may
>  require asynchronous cleanup, making it inappropriate to synchronously
>  close communication channels with remote contexts in such cases.
>  
>  Ensure that this implementation does not introduce a new dependency on
>  the global `Promise` constructor in order to avoid interfering with
>  tests that do not require it (as doing so would invalidate those tests
>  in contexts where `Promise` is not available).
>  
>  ---
>  
>  The return value of the function provided to `Test#add_cleanup` is only
>  meaningful for tests created with the `promise_test` API, and in those
>  cases, only when it is a "thenable" object. If a cleanup function
>  returns an invalid value, this may reflect a programming error that will
>  silently degrade test stability.
>  
>  Detect cases where an invalid value is returned and trigger immediate
>  harness failure, helping contributors identify mistakes.
>  
>  ---
>  
>  This patch is intended to resolve gh-6075. Since filing that issue, I've
>  identified a few new details that warrant mentioning:
>  
>  - **This feature is limited to tests created with the `promise_test` API.**
>    Asynchronous cleanup logic could invalidate tests that were designed to run
>    in a fully-synchronous manner. While a careful implementation could allow
>    synchronous tests to "opt in" to the new behavior, APIs that are not
>    consistently synchronous or asynchronous are [notoriously difficult to reason
>    about](http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony).
>  - **Cleanup functions are invoked in parallel.** We could investigate running
>    them in series, but I think the proper order in that case would be "first-in,
>    last-out." Given that the fully-synchronous behavior in `master` today is
>    "first-in, first-out," this could invalidate tests. So for now, I've settled
>    on addressing this with documentation (suggesting that consumers introduce
>    their own scheduling mechanism in cases where ordering is important).
>  - **Inappropriate return values trigger harness error.** Because the API is
>    more implicit than a function call, it's possible that test authors may
>    unintentionally provide inappropriate values. This kind of error may not be
>    immediately obvious since it will introduce race conditions that may only
>    occasionally influence test behavior. In order to highlight the mistakes (and
>    avoid a source of instability), this patch triggers a harness error whenever
>    a cleanup function returns an inappropriate value.
>  
>  After a brief investigation into the current usage of `Test#add_cleanup` in WPT
>  today, I've identified two files that return a value. I've opened gh-8742 to
>  correct that. As noted there, if the maintainers would like to use this new
>  functionality, I think it's safer to allow them to do so intentionally (rather
>  than implicitly changing it out from underneath them).
>  
>  This patch avoids introducing a dependency on the global `Promise` constructor
>  for any tests that have not "opted in" to this behavior. I've verified this
>  locally using an extended version of the test harness for `testharness.js`, and
>  I intend to submit that for review separately (since it relies on a more
>  generic improvement--see gh-8597).
>  
>  Also note that as of yesterday, any change to the contents of `testharness.js`
>  will invalidate a few web platform tests. I've submitted gh-8735 to address
>  this. If we find that we'd rather land this patch before that one, then I will
>  update the file hashes which are hard-coded into those test files (as a final
>  step of the review process).
>  
>  
>