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

[testharness.js] Honor promises from cleanup fns #8748

Merged
merged 41 commits into from Aug 9, 2018

Conversation

Projects
None yet
8 participants
@jugglinmike
Contributor

jugglinmike commented Dec 20, 2017

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


This change is Reviewable

jugglinmike added some commits Apr 17, 2017

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

Correct errors in existing tests that were uncovered by this new
functionality.
[testharness.js] Restrict cleanup fn return value
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.
@w3c-bots

This comment has been minimized.

w3c-bots commented Dec 20, 2017

Build PASSED

Started: 2017-12-22 22:30:30
Finished: 2017-12-22 22:37:03

View more information about this build on:

@sideshowbarker sideshowbarker removed their request for review Dec 20, 2017

@foolip

This comment has been minimized.

Contributor

foolip commented Dec 20, 2017

I'm trying the changes in https://chromium-review.googlesource.com/c/chromium/src/+/836619 since our CI here isn't able to handle changes this big.

@wpt-pr-bot wpt-pr-bot requested a review from sideshowbarker Dec 20, 2017

@foolip

Lots of comments, but this is looking really good. I'll be around today and tomorrow to keep reviewing.

@@ -30,8 +30,8 @@ <h1>Dedicated Web Worker Tests</h1>
{
"summarized_status": {
"status_string": "ERROR",
"message": "Error: This failure is expected.",
"stack": "(implementation-defined)"
"message": "Error in remote https://web-platform.test:9003/resources/test/tests/worker-error.js: Error: This failure is expected.",

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

Why did this change? Seems quite likely that this will cause failures somewhere, and that the lint was right to flag it.

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

The remote_done function severs the connection that a "remote" environment
has with its parent. It is intended to be invoked when a remote test completes.

  • In master today, remote_done is invoked before remote_error. When
    remote_error is later invoked, it delegates to remote_done. However,
    because that method has already been invoked, this causes an internal
    exception which interrupts further execution of the function body.
    remote_error is designed to invoke preventDefault on the uncaught
    exception event, but this never happens due to the runtime exception. That
    means the error "bubbles up" to the parent context. testharness.js (the
    instance running in the parent context) reacts to the uncaught error by
    updating the status accordingly.

    This is why the current failure message inaccurately describes an error that
    originated in the parent context.

  • With this patch applied, delegation to remote_done in remote_error has
    been refactored, so in the internal exception does not occur in this test,
    and the subsequent call to preventDefault proceeds as intended. This means
    the parent context does not receive an "uncaught error" event. Instead, it
    accurately reports the error with the text, "Error in remote [...]".

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

It was the "web-platform.test:9003" bit that worried me, not the change in behavior itself, although I did not understand the cause of that either. The new behavior seems fine, and if the port will always be 9003 that's also fine. Can you confirm that it passes if run locally, not just on Travis?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

Can you confirm that it passes if run locally, not just on Travis?

Every time :)

@@ -5,7 +5,7 @@
</head>
<script src="../../testharness.js"></script>
<body onload="load_test_attr.done()">

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

Looks like load_test_attr isn't defined anywhere, but why did this come up in this review? Does it fail in a novel way now?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

This test invokes the global timeout function in the initial turn of the
event loop.

  • In master today, this causes the internal notify_complete function to be
    invoked in that same turn. The harness synchronously renders the "timeout"
    status (and reports as much to the test runner)
  • On this branch, the notify_complete function is scheduled to be run with a
    new task (via setTimeout). The document load event fires in the
    intervening time, so by the moment the test status is reported, the "timeout"
    has been updated to an "error".

The root cause of this is the new all_async function. I've implemented that
to always invoke the final "complete" callback asynchronously, even in cases
where the individual operations have completed in a fully-synchronous manner. I
mentioned why I believe consistency in synchronicity is important in the pull
request description.

I suppose we could preserve the existing behavior by disallowing state
changes from "TIMEOUT" to "ERROR". However, both states essentially mean the
same thing: the test needs to be updated. I think "ERROR" is a more severe
case, so allowing that transition seems valid.

Also, it's worth mentioning that the usage we're discussing is pretty
artificial: when would a test synchronously trigger a timeout?

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

Unless you're saying that load_test_attr.done() is an elaborate way of throwing an exception to test what happens with exceptions thrown by load attribute event handlers, I still don't get it. I've sent #8774 to remove these bits in two tests, and the tests pass all the same.

If it is needed, can you rename it to not_a_variable.not_a_function() or something?

</head>
<body>
<h1>Promise Tests</h1>
<p>This test demonstrates the use of <tt>promise_test</tt> alongside synchronous tests.</p>

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

nit: <code> is what's used in specs, I had to think about what <tt> does.

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

MDN agrees, but
this was an attempt to respect local convention. Would you mind if I filed a
separate issue to switch to <code>?

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

Sure, I didn't notice any of the surrounding usage. Just leaving it alone without an issue is fine.

}, "first synchronous test");
promise_test(function() {
assert_array_equals(sequence, [1, 2]);

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

I think there may be something wrong with me, but I really like this kind of timing/ordering test :)

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

If assert_array_equals(sequence, [1, 2]); is wrong, then I don't want to be
right.

}, "Third synchronous test");
promise_test(function(t) {
assert_false(

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

Oh, this bit I didn't actually know before. I thought that the first step of the first promise test would be executed sync like async_test, but apparently not. Good to know, and good to have tested :)

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

Right on!

forEach(this.tests,
function(test) {
if (test.phase === test.phases.CLEANING) {
cleaningTest = test;

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

It looks like cleaningTest will be set to the last test in cleanup, but there can actually just be one. Can you add a comment to ensure the reader that this is not an oversight?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

Sure. I've also tried to think of a way to re-write the code to better express
the expectation, but besides some sort of "assert" statement, I can't think of
anything better.

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

Yeah, I think a comment is the best tradeoff, if we start asserting things and they ever fail, we'd have to think about where those exception end up and if it'll look like a harness error or a test error. No thanks :)

// sub-tests.
if (cleaningTest) {
this.status.status = this.status.ERROR;
this.status.message = "Cleanup function for test named \"" +

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

The cleanup function itself doesn't have a timeout, so maybe "Timeout while running cleanup for test named ..."?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

Oooh, that is a really nuanced distinction. I like your text much better!

*
* @param {array} value Zero or more values to use in the invocation of
* `predicate`
* @param {function} predicate A function that will be invoked once for

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

Calling this a predicate seems a bit weird to me, at least it's not a https://en.wikipedia.org/wiki/Predicate_(mathematical_logic) which is what I usually expect in code. Maybe just "callback", or something else?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

This is tough because the API expects two callback functions. What do you think
of "iteratee", is in _.forEach?

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

That would work for me, I have a soft spot for made up somethingee words. "iterCallback" and "doneCallback" would also WFM.

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

I like those, too. I've used the "snake case" versions in observance of local code style convention.

test._add_done_callback(testDone);
if (test.phase < test.phases.CLEANING) {
test.cleanup();

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

I see this was done before as well, but can you comment about why cleanup is started here, when in some cases apparently it can already have happened?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

As near as I can tell, this method was originally written to invoke the
cleanup method as a way to simulate completion for pending tests. But that's
pretty indirect, as your question indicates. It would be better to do something
like the following:

if (test.phase === test.phases.CLEANING) {
    test._add_done_callback(testDone);
    return;
}

if (test.phase < test.phases.CLEANING) {
    test.phase = test.phases.COMPLETE;
    this_obj.result(test);
}

testDone();

...but this version causes failures, so I will need to investigate further.
I'll have a better answer for you tomorrow.

This comment has been minimized.

@foolip

foolip Dec 21, 2017

Contributor

Leaving a comment or a TODO would be fine, given that this was already odd.

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

I was able to address this as part of the refactoring mentioned above.

@@ -2920,12 +3096,10 @@ policies and contribution forms [3].
}
test.set_status(test.FAIL, e.message, stack);
test.phase = test.phases.HAS_RESULT;
test.done();

This comment has been minimized.

@foolip

foolip Dec 20, 2017

Contributor

Why no test.done() here? Was it always unnecessary, or only now?

This comment has been minimized.

@jugglinmike

jugglinmike Dec 21, 2017

Contributor

This is due to a bug in my changes to test.done. In the version I originally
submitted, if invoked after a test has already transitioned to the "COMPLETE"
phase, it would exit without calling any "done" callbacks. Effectively, calling
test.done here caused the test harness to time out.

I've been thinking about the internal API of this change set. You previously
identified how the done_callbacks abstraction was a bit
leaky
, and this is another sign of that.

So I hope you don't mind, but I've refactored the internals for clarity. The
only outward change is that it allows us to re-introduce this test.done
invocation (which is superfluous in master, by the way--I've included a
comment to remove it later).

@foolip

This comment has been minimized.

Contributor

foolip commented Dec 20, 2017

There are rather a lot of new failures with new harness errors in https://chromium-review.googlesource.com/c/chromium/src/+/836619 that may be related:

css/css-conditional/test_group_insertRule.html
css/css-values/calc-unit-analysis.html
fetch/api/response/response-stream-disturbed-6.html
geolocation-API/getCurrentPosition_IDL.https.html
html/dom/interfaces.html
html/semantics/embedded-content/media-elements/playing-the-media-resource/play-in-detached-document.html
html/semantics/forms/the-select-element/selected-index.html
mimesniff/mime-types/parsing.any.html
mimesniff/mime-types/parsing.any.worker.html
offscreen-canvas/compositing/2d.composite.canvas.source-in.worker.html
offscreen-canvas/compositing/2d.composite.canvas.source-out.worker.html
offscreen-canvas/fill-and-stroke-styles/2d.pattern.paint.norepeat.coord2.worker.html
offscreen-canvas/fill-and-stroke-styles/2d.pattern.paint.norepeat.outside.worker.html
offscreen-canvas/fill-and-stroke-styles/2d.pattern.paint.repeatx.coord1.worker.html
service-workers/service-worker/client-navigate.https.html
user-timing/measure_exceptions_navigation_timing.html

There are some outside of wpt too, but probably the same root cause.

@jugglinmike, can you check the above and see if there are real regressions here?

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Aug 31, 2018

service workers: Simplified tests with async/await
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Aug 31, 2018

service workers: Simplified tests with async/await
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 17, 2018

service workers: Simplified tests with async/await
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 19, 2018

service workers: Simplified tests with async/await
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Return value in cleanup
Previously, many tests un-registered service workers using the
testharness.js API `add_cleanup` without returning the promise which
tracked the operation. While this ensured the operation was started at
the completion of the test, it did not guarantee that the operation
would be completed before the test was considered "complete."

In automation scenarios where many tests are executed in rapid
succession, this enabled a race condition: the navigation could
interrupt the un-registration process. In order to account for the
possibility of registrations that persisted from previous test failures,
the tests included "setup" code to defensively un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to ensure tests are
not considered "complete" until after service worker un-registration is
done.

[1] web-platform-tests#8748

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Use asynchronous cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] web-platform-tests#8748

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Refactor to use async cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] web-platform-tests#8748

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 20, 2018

service workers: Simplified tests with async/await - Part 1
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Return value in cleanup
Previously, many tests un-registered service workers using the
testharness.js API `add_cleanup` without returning the promise which
tracked the operation. While this ensured the operation was started at
the completion of the test, it did not guarantee that the operation
would be completed before the test was considered "complete."

In automation scenarios where many tests are executed in rapid
succession, this enabled a race condition: the navigation could
interrupt the un-registration process. In order to account for the
possibility of registrations that persisted from previous test failures,
the tests included "setup" code to defensively un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to ensure tests are
not considered "complete" until after service worker un-registration is
done.

[1] web-platform-tests#8748

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Use asynchronous cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] web-platform-tests#8748

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 20, 2018

[service-workers] Refactor to use async cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] web-platform-tests#8748

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 21, 2018

service workers: Simplified tests with async/await - Part 1
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 21, 2018

[service-workers] Refactor to use async cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] web-platform-tests#8748

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 21, 2018

service workers: Simplified tests with async/await - Part 1
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 21, 2018

[service-workers] Use asynchronous cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] web-platform-tests#8748

mkruisselbrink added a commit that referenced this pull request Sep 21, 2018

[service-workers] Return value in cleanup (#13163)
Previously, many tests un-registered service workers using the
testharness.js API `add_cleanup` without returning the promise which
tracked the operation. While this ensured the operation was started at
the completion of the test, it did not guarantee that the operation
would be completed before the test was considered "complete."

In automation scenarios where many tests are executed in rapid
succession, this enabled a race condition: the navigation could
interrupt the un-registration process. In order to account for the
possibility of registrations that persisted from previous test failures,
the tests included "setup" code to defensively un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to ensure tests are
not considered "complete" until after service worker un-registration is
done.

[1] #8748

wanghongjuan added a commit to wanghongjuan/wpt that referenced this pull request Sep 25, 2018

service workers: Simplified tests with async/await - Part 1
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748.
- The usage of unregister() method in some tests is not rigorous.  As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
>>   [NewObject] Promise<boolean> unregister();
  unregister() method must run these steps:
>> 1. Let promise be a promise.
...
>> 4. Return promise.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 27, 2018

Bug 1493409 [wpt PR 13163] - [service-workers] Return value in cleanu…
…p, a=testonly

Automatic update from web-platform-tests[service-workers] Return value in cleanup (#13163)

Previously, many tests un-registered service workers using the
testharness.js API `add_cleanup` without returning the promise which
tracked the operation. While this ensured the operation was started at
the completion of the test, it did not guarantee that the operation
would be completed before the test was considered "complete."

In automation scenarios where many tests are executed in rapid
succession, this enabled a race condition: the navigation could
interrupt the un-registration process. In order to account for the
possibility of registrations that persisted from previous test failures,
the tests included "setup" code to defensively un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to ensure tests are
not considered "complete" until after service worker un-registration is
done.

[1] web-platform-tests/wpt#8748
--

wpt-commits: 7897f9d5beff624590d2fc254d2a7eca6e2b1e8f
wpt-pr: 13163

dmose pushed a commit to dmose/gecko that referenced this pull request Sep 27, 2018

Bug 1493409 [wpt PR 13163] - [service-workers] Return value in cleanu…
…p, a=testonly

Automatic update from web-platform-tests[service-workers] Return value in cleanup (#13163)

Previously, many tests un-registered service workers using the
testharness.js API `add_cleanup` without returning the promise which
tracked the operation. While this ensured the operation was started at
the completion of the test, it did not guarantee that the operation
would be completed before the test was considered "complete."

In automation scenarios where many tests are executed in rapid
succession, this enabled a race condition: the navigation could
interrupt the un-registration process. In order to account for the
possibility of registrations that persisted from previous test failures,
the tests included "setup" code to defensively un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to ensure tests are
not considered "complete" until after service worker un-registration is
done.

[1] web-platform-tests/wpt#8748
--

wpt-commits: 7897f9d5beff624590d2fc254d2a7eca6e2b1e8f
wpt-pr: 13163

mattto added a commit that referenced this pull request Oct 18, 2018

[service-workers] Refactor to use async cleanup (#13165)
[service-workers] Refactor to use async cleanup

Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] #8748

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2018

Bug 1493416 [wpt PR 13165] - [service-workers] Refactor to use async …
…cleanup, a=testonly

Automatic update from web-platform-tests[service-workers] Refactor to use async cleanup (#13165)

[service-workers] Refactor to use async cleanup

Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] web-platform-tests/wpt#8748
--

wpt-commits: 1569ae03d34aa2d1eb2fce5de52eba07673eae2b
wpt-pr: 13165

xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Nov 8, 2018

Bug 1493416 [wpt PR 13165] - [service-workers] Refactor to use async …
…cleanup, a=testonly

Automatic update from web-platform-tests[service-workers] Refactor to use async cleanup (#13165)

[service-workers] Refactor to use async cleanup

Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would *not*
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1], but the functionality is only
available to tests declared using `promise_test`. Update tests which
were previously declared with `async_test` and use the new "async
cleanup" API to schedule service worker un-registration so that it
occurs regardless of the result of each test.

[1] web-platform-tests/wpt#8748
--

wpt-commits: 1569ae03d34aa2d1eb2fce5de52eba07673eae2b
wpt-pr: 13165

jugglinmike added a commit to bocoup/wpt that referenced this pull request Dec 3, 2018

[service-workers] Use asynchronous cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] web-platform-tests#8748

jugglinmike added a commit to bocoup/wpt that referenced this pull request Dec 3, 2018

[service-workers] Use asynchronous cleanup
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] web-platform-tests#8748

mattto added a commit that referenced this pull request Dec 4, 2018

[service-workers] Use asynchronous cleanup (#13164)
Previously, many tests un-registered service workers only after all
assertions had been satisfied. This meant that failing tests would not
un-register workers. In order to account for workers that persisted from
previous test failures, the tests included "setup" code to defensively
un-register such workers.

The `Test#add_cleanup` method was recently extended to support
asynchronous "clean up" operations [1]. Use that API to schedule service
worker un-registration so that it occurs regardless of the result of
each test.

[1] #8748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment