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] Reject non-thenable values #12898

Merged
merged 1 commit into from Sep 8, 2018

Conversation

Projects
None yet
5 participants
@jugglinmike
Contributor

jugglinmike commented Sep 7, 2018

[testharness.js] Reject non-thenable values

Previously, any test defined via the promise_test API would fail
immediately if its body returned the value undefined. The test would
not fail if it returned any other value. Because the motivation for
using promise_test is to track resolution with a "thenable" value
(i.e. an object with a "then" method), this was overly permissive and
allowed contributors to write tests which spuriously passed [1].

Update testharness.js to enforce more stringent criteria on the value
return by promise_test bodies: that it is a "thenable" value. This
change is incompatible with an exiting functional test, but it does not
effect any tests in WPT (as verified by a survey using both the Chrome
and Firefox browsers). Update the functional test accordingly.

[1] cca6b68


As noted in the commit message, the current leniency recently allowed test bugs that could have been avoided.

Because there is no use case for a promise_test that does not return a thenable, I've also tried to verify that no other tests in WPT exhibit this problem. I modified testharness.js and wptserve to log those cases, introduced an intentional infraction, and ran those changes through Chrome dev and Firefox Nightly on Taskcluster. I used the new tc-download feature to inspect the result:

$ ./wpt tc-download --repo-name bocoup/wpt --ref suspicious-promise --out-dir koo --artifact-name suspicious.txt
$ ls koo
wpt-chrome-dev-testharness-4-0-suspicious.txt  wpt-firefox-nightly-testharness-4-0-suspicious.txt
$ cat koo/*
http://web-platform.test:8000/dom/events/CustomEvent.html - CustomEvent
http://web-platform.test:8000/dom/events/CustomEvent.html - CustomEvent

Since Firefox and Chrome reported the intentional infraction, I think the process is sound. Since they reported no other infractions, I don't think their are any previously-existing errors.

[testharness.js] Reject non-thenable values
Previously, any test defined via the `promise_test` API would fail
immediately if its body returned the value `undefined`. The test would
*not* fail if it returned any other value. Because the motivation for
using `promise_test` is to track resolution with a "thenable" value
(i.e. an object with a "then" method), this was overly permissive and
allowed contributors to write tests which spuriously passed [1].

Update testharness.js to enforce more stringent criteria on the value
return by `promise_test` bodies: that it is a "thenable" value. This
change is incompatible with an exiting functional test, but it does not
effect any tests in WPT (as verified by a survey using both the Chrome
and Firefox browsers). Update the functional test accordingly.

[1] cca6b68
@mikewest

This LGTM, thanks for making sure my tests won't be bad in the future!

@foolip

foolip approved these changes Sep 8, 2018

Excellent, thanks for the detailed explanation and well-tested fix, Mike!

@foolip foolip merged commit 8657903 into web-platform-tests:master Sep 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Sep 11, 2018

My pleasure!

aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 8, 2018

Switch a `promise_test` to a plain synchronous `test`
This adapts to "Reject non-thenable values" upstream:
web-platform-tests/wpt#12898

This is blocking updating LayoutTests/resources/testharness.js.

Bug: 892613
Change-Id: Ifbc91824c0a5687a74cc6f16e02457da60dc4e85
Reviewed-on: https://chromium-review.googlesource.com/c/1264585
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597521}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment