class: middle, center # The Long Road to Async Cleanup 2018-09-18 .softer[ (press the
p
key to view presenter's notes) ] .license[
This presentation is licensed under a
Creative Commons Attribution-ShareAlike 4.0 International License
. ] --- class: middle, center background-image: url(img/calipers.png) background-size: contain # Discussion Which is your favorite testing framework and why? .image-attribution[ [Calipers!](https://www.flickr.com/photos/fciron/5510886440/) by [Lewis Meyer](https://www.flickr.com/photos/fciron/) is licensed [CC BY-NC 2.0](https://creativecommons.org/licenses/by-nc/2.0/) ] --- .web-page-screenshot[ ![Bocoup's Service Workers project](img/bocoup-service-workers.png) ] --- # The problem ```js promise_test(function(t) { var worker = "resources/client-navigate-worker.js"; var scope = "resources/client-navigate-frame.html"; return service_worker_register(t, worker, scope) .then(function() { /* etc. */ }) .then(service_worker_unregister(t, scope)); }); ``` -- ```js promise_test(function(t) { var worker = "resources/client-navigate-worker.js"; var scope = "resources/client-navigate-frame.html"; return service_worker_unregister_and_register(t, worker, scope) .then(function() { /* etc. */ }) .then(service_worker_unregister(t, scope)); }); ``` --- class: side-by-side ```js promise_test(function() { return change_global_state() .then(function() { // conformance test }) .then(function() { // revert global state }); }); ``` -- ```js promise_test(function() { var cleanup = function() { // revert global state }; var test_body = change_global_state() .then(function() { // conformance test }); // Ensure that test "clean up" is // deferred until after the test body // executes. `Test#add_cleanup` cannot // be used for this purpose because // the operation is asynchronous, and // `add_cleanup` does not support // asynchronous operations at the time // of this writing. See // https://github.com/web-platform-tests/wpt/issues/6075 // Ensure also that test failure is // not hidden by successful cleanup // operation. return test_body .then(cleanup, cleanup) .then(function() { return test_body; }); }); ``` ??? This is an asynchronous version of `try`/`finally`. It could be expressed more concisely with an `async` function or with `Promise.prototype.finally`. We had a goal to make these tests as accessible as possible, so neither of these were viable options. --- # Initial Interest ![Initial interest](img/comments/initial-interest.png) --- # It Begins ![It begins](img/comments/it-begins.png) ??? This is a screenshot of a discussion thread on GitHub. It is also a screenshot of naivety. --- # The process 1. Write some code 2. Use the new code to run *all of WPT* in Chromium's build system 3. Identify new failures in the web platform tests 4. Investigate what change to the test harness triggered the failures--these are regressions -- Why Chromium? Why all of WPT? --- class: center, middle # No Tests. ??? Prior to this project, WPT's test harness was essentially untested. It included a handful of manual tests, but a manual test is no more resilient to rot than documentation. We essentially used all of WPT as a test suite. --- class: center, middle background-image: url(img/servo.png) background-size: contain ??? The Servo browser presented another challenge. Servo is the experimental browser that was started at Mozilla. It also runs WPT, and it had a unique deficiency. -- ![No Promises in Servo](img/comments/servo-no-promises.png) ??? It would be easy to change the test harness in a way that made it impossible for Servo to run WPT. We wanted to avoid this, so we took on an additional constraint: we would only use the `Promise` API if the current test already involved promises. --- # Improved testing ![Improved testing](img/comments/improved-testing.png) - [gh-5627: Introduce automated tests for testharness.js](https://github.com/web-platform-tests/wpt/pull/5627) - [gh-6151: [testharness.js] `Test#add_cleanup` tests and fix](https://github.com/web-platform-tests/wpt/pull/6151) - [gh-8597: [testharness.js] Formalize test types](https://github.com/web-platform-tests/wpt/pull/8597) - [gh-11150: [testharness.js] Extend test suite with "variants"](https://github.com/web-platform-tests/wpt/pull/11150) --- # The process 1. Write some code 2. Use the new code to run *all of WPT* in Chromium's build system 3. Identify new failures in the web platform tests 4. **Investigate what change to the test harness triggered the failures--these are regressions** --- # Diagnosing failures - **Behavioral regressions (my fault)** - Test bugs (someone else's fault) - Implementation bugs (someone else's fault) ??? Since we were vetting correctness by using the web platform tests themselves, even just diagnosing failures was a challenge. Reports wouldn't describe test harness regressions directly; instead, they appeared to highlight browser implementation errors. Those failures fell into 3 basic categories. While it's easy to assign blame, it doesn't matter whose "fault" it was. All of the failures were my problem. --- ### Behavioral regressions Expected scheduling behavior: Task | Microtask | Synchronous steps 1 | 1 | test() ---------> { /*body*/ } --> testharness complete 2 | 1 | (uncaught exceptions) Task | Microtask | Synchronous steps 1 | 1 | promise_test() 1 | 2 | `--------> { return p; } 1 | 3 | `---------> testharness complete 2 | 1 | (uncaught exceptions) Regression: Task | Microtask | Synchronous steps 1 | 1 | promise_test() 1 | 2 | `--------> { return p; } 2 | 1 | | (uncaught exceptions) 3 | 1 | `-------------------------------> testharness complete ??? One particularly challenging issue concerned the event loop, tasks, and microtasks. The initial version of the patch identified a number of tests with uncaught exceptions. It was tempting to call this an improvement, but we ultimately decided that ignoring these types of exceptions was an undocumented feature of the test harness. --- ### Behavioral regressions Existing usages of `add_cleanup` - [gh-8742: Do not return value from "cleanup" functions](https://github.com/web-platform-tests/wpt/pull/8742) - [gh-11733: [fullscreen] Do not return value from cleanup fns](https://github.com/web-platform-tests/wpt/pull/11733) - [gh-11740: [html] Do not return value from cleanup fns](https://github.com/web-platform-tests/wpt/pull/11740) - [gh-11741: [fetch] Do not return value from cleanup fns](https://github.com/web-platform-tests/wpt/pull/11741) - [chromium-1109513: Make add_cleanup step in no-inherit-type.html return undefined](https://chromium-review.googlesource.com/c/chromium/src/+/1109513) (thanks, Philip!) ??? Plenty of tests were already using `add_cleanup`, which was just fine by us. However, some of them returned a value Even though doing so had no effect, this was a concern for two reasons: 1. The patch made it an error for a test to return a non-promise value. This would help authors avoid making mistakes (e.g. `return foo` instead of `return foo()`), but it was also a new restriction, so we were responsible for integrating it. 2. Tests that happened to return promise values would suddenly start to behave differently after the patch was landed. We didn't want to introduce any behavior changes to existing tests, since it would be hard for the tests' maintainers to recognize cases where this was a problem --- ```diff diff --git a/html/semantics/forms/the-select-element/selected-index.html b/html/semantics/forms/the-select-element/selected-index.html index 6c30698a8ae1..46f19da7da29 100644 --- a/html/semantics/forms/the-select-element/selected-index.html +++ b/html/semantics/forms/the-select-element/selected-index.html @@ -70,7 +70,7 @@ assertSelectedIndex(select, 2); - this.add_cleanup(() => select.selectedIndex = 0); + this.add_cleanup(() => { select.selectedIndex = 0; }); }, "set (HTMLSelectElement)"); ``` ??? This was generally straightforward. -- ```diff diff --git a/fullscreen/api/element-ready-check-allowed-cross-origin-manual.sub.html b/fullscreen/api/element-ready-check-allowed-cross-origin-manual.sub.html index fa753f8b41be..92d499bc691b 100644 --- a/fullscreen/api/element-ready-check-allowed-cross-origin-manual.sub.html +++ b/fullscreen/api/element-ready-check-allowed-cross-origin-manual.sub.html @@ -7,7 +7,9 @@ async_test((t) => { - t.add_cleanup(() => document.exitFullscreen()); + t.add_cleanup(() => { + Promise.resolve(document.exitFullscreen()).catch(() => {}); + }); // When a message is received from a child frame, ensure that the report // matches the expectations. ``` ??? Though sometimes, it required a little extra work. --- # Diagnosing failures - Behavioral regressions (my fault) - **Test bugs (someone else's fault)** - Implementation bugs (someone else's fault) --- ### Test bugs Uncaught errors and unhandled rejections - [gh-12266: [client-hints] Detect errors to avoid timeouts](https://github.com/web-platform-tests/wpt/pull/12266) - [gh-12267: [web-locks] Detect errors to avoid timeouts](https://github.com/web-platform-tests/wpt/pull/12267) - [gh-12268: [workers] Detect errors to avoid timeouts](https://github.com/web-platform-tests/wpt/pull/12268) - [chromium-1109689: Fix two testharness.js tests with harness errors due to stray asserts](https://chromium-review.googlesource.com/c/chromium/src/+/1109689) (thanks, Robert!) --- class: middle ```diff diff --git a/workers/modules/dedicated-worker-import-blob-url.any.js b/workers/modules/dedicated-worker-import-blob-url.any.js index 9a50b23893b3..e3f61867c5b9 100644 --- a/workers/modules/dedicated-worker-import-blob-url.any.js +++ b/workers/modules/dedicated-worker-import-blob-url.any.js @@ -11,7 +11,10 @@ function import_blob_url_test(testCase) { const blobURL = URL.createObjectURL(blob); const worker = new Worker(blobURL, { type: 'module'}); - const msgEvent = await new Promise(resolve => worker.onmessage = resolve); + const msgEvent = await new Promise((resolve, reject) => { + worker.onmessage = resolve; + worker.onerror = (error) => reject(error && error.message); + }); assert_array_equals(msgEvent.data, testCase.expectation); }, testCase.description); } ``` ??? These were mostly easy fixes. --- ### Test bugs Race conditions [gh-11082: [css-fonts] Avoid race condition](https://github.com/web-platform-tests/wpt/pull/11082) Surprisingly, just the one. --- background-image: url(img/comments/css-review.png) background-size: contain ??? However, fixing even just one test can take quite a lot of work. This review involved six people and ran for just over 3 months. --- ### Test bugs Unhandled promise rejections ```js function fillStackThenCallPut(foo, request, response) { try { fillStackThenCallPut(foo, request, response); } finally { // This runs thousands of times, causing the console to spew "Uncaught // (in promise) TypeError: Response body is already used" messages, but // it's harmless. // stack is full. if (putRunsLeft > 0) { --putRunsLeft; foo.put(request, response); } } } ``` [chromium-1150882: Allow unhandled promise rejection in LayoutTest](https://chromium-review.googlesource.com/1150882) --- ```diff diff --git a/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-is-locked.html b/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-is-locked.html index ae651ec0af9e8..d6335ebde831d 100644 --- a/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-is-locked.html +++ b/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-is-locked.html @@ -2,6 +2,11 @@