Skip to content
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] Don't complete after allowed exc. #19612

Merged

Conversation

@jugglinmike
Copy link
Contributor

jugglinmike commented Oct 10, 2019

The following tests intentionally produce an uncaught exception and then define
subtests:

  • html/semantics/scripting-1/the-script-element/module/error-and-slow-dependency.html
  • html/webappapis/scripting/processing-model-2/window-onerror-parse-error.html
  • html/webappapis/scripting/processing-model-2/window-onerror-runtime-error.html
  • html/webappapis/scripting/processing-model-2/window-onerror-runtime-error-throw.html

testharness.js immediately transitions to "complete" and ignores the
subsequent subtests.


Today, the tests referenced in the commit message are reported as passing single-page tests. Once we've completed the implementation of WPT RFC 28, they will instead be reported as test harness errors.

As an alternative to this patch, we could modify each of the tests. If a subtest is defined before the exception is thrown, then all the subtests are reported as expected. (We could, for example, declare the existing synchronous subtests as asynchronous subtests instead.)

Changing the harness is definitely riskier, but it also seems more correct to me.

This change could interfere with single-page tests that use allow_uncaught_exceptions. My experiments don't indicate that any such tests exist, but it's difficult to demonstrate or summarize that research here. A more blunt heuristic might do. Only three tests use allow_uncaught_exception without invoking functions named test, promise_test, or async_test:

$ git grep -l allow_uncaught_exception | xargs grep -LE '\b(test|promise_test|async_test)\s*\('
IndexedDB/fire-error-event-exception.html
IndexedDB/fire-success-event-exception.html
custom-elements/upgrading/upgrading-enqueue-reactions.html

All three define subtests indirectly through helper functions.

The following tests intentionally produce an uncaught exception and then define
subtests:

- html/semantics/scripting-1/the-script-element/module/error-and-slow-dependency.html
- html/webappapis/scripting/processing-model-2/window-onerror-parse-error.html
- html/webappapis/scripting/processing-model-2/window-onerror-runtime-error.html
- html/webappapis/scripting/processing-model-2/window-onerror-runtime-error-throw.html

testharness.js immediately transitions to "complete" and ignores the
subsequent subtests.
@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 12, 2019

This patch caused a failure in the testharness.js test suite, and that failure demonstrated an edge case I hadn't considered. After looking in to it further (research summarized below), it looks like the edge case was limited to the infrastructure test itself. I don't think it demonstrates anything unsound about this patch.


It's possible that worker-based tests (which require done to be invoked) have been written to rely on the "done on uncaught exception" behavior which testharness.js currently exhibits. This change would cause those tests to begin reporting as TIMEOUT (as it did with the failing infrastructure test).

It doesn't look like this is a problem, though. There are six worker sources which enable allow_uncaught_exception:

$ git grep -lE 'importScript.*testharness' | xargs grep -l allow_unca
docs/writing-tests/testharness-api.md
html/webappapis/scripting/events/event-handler-processing-algorithm-error/synthetic-errorevent-click.worker.js
html/webappapis/scripting/events/event-handler-processing-algorithm-error/workerglobalscope-runtime-error.worker.js
html/webappapis/scripting/events/event-handler-processing-algorithm-error/workerglobalscope-synthetic-errorevent.worker.js
html/webappapis/scripting/events/event-handler-processing-algorithm-error/workerglobalscope-synthetic-event.worker.js
html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/support/promise-rejection-events.js
resources/test/tests/functional/worker-uncaught-allow.js

With the exception of the testharness.js test mentioned above, all of them invoke done() at the top-level (and the uncaught exception does not interfere with this).

There are three multi-global tests that use allow_uncaught_exception:

$ find . -name '*any.js' | xargs grep -l allow_unca
./IndexedDB/idb-explicit-commit-throw.any.js
./web-locks/held.tentative.https.any.js
./html/webappapis/microtask-queuing/queue-microtask-exceptions.any.js

For these, the done() call is inserted by the server, but again, the uncaught exception does not occur at the top level, so each invocation is always evaluated.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Oct 14, 2019

In principle this looks OK, I think. But please push to at least one of the trigger branches for Firefox or Chrome nightly in order to check for unexpected regressions.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Oct 15, 2019

So I see a Fx run; the wpt.fyi diff is https://wpt.fyi/results/?diff&filter=ADC&run_id=321040007&run_id=344290004 Are any of those changes worrying?

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 15, 2019

@jgraham I think we're good

23 tests are flaky in Firefox
43 tests are reftests (so they aren't affected by changes to testharness.js)
10 are wdspec tests (so they aren't affected by changes to testharness.js)

More interestingly, there were 4 tests that had bugs:

...and 5 tests whose modified results represent a correction (including one that I didn't spot above):

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Oct 18, 2019

@jgraham Do you think this needs further validation?

@jugglinmike jugglinmike merged commit e9465a5 into web-platform-tests:master Oct 23, 2019
12 checks passed
12 checks passed
build-and-publish
Details
Azure Pipelines Build #20191014.88 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
Taskcluster (push) TaskGroup: success
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - safari[experimental] Safari results
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.