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] Disallow `test` return value #12958

Merged
merged 4 commits into from Sep 17, 2018

Conversation

Projects
None yet
4 participants
@jugglinmike
Contributor

jugglinmike commented Sep 12, 2018

As a follow up to gh-12898, I wondered about similar test authoring mistakes
that we could avoid from within testharness.js. Are there any synchronous tests
that return a value? This would generally be harmless, but it that value were a
thenable, then it could be an unintentional use of test instead of
promise_test. TaskCluster again helped to answer the question:

$ ./wpt tc-download --ref suspicious-sync --repo bocoup/wpt --artifact-name suspicious.txt --out-dir suspicious
INFO:tc-download:suspicious-2/wpt-firefox-nightly-testharness-4-DlCidQw0SvOYFb_9dvqWLg-suspicious.txt
INFO:tc-download:suspicious-2/wpt-chrome-dev-testharness-4-P0O_249FTdGyXlloowzNRQ-suspicious.txt
cat suspicious/*
http://web-platform.test:8000/dom/events/CustomEvent.html - this is likely a falsey mistake
http://web-platform.test:8000/web-animations/timing-model/animations/setting-the-target-effect-of-an-animation.html - Setting the target effect to null causes a pending playback rate to be applied
http://web-platform.test:8000/dom/events/CustomEvent.html - this is likely a falsey mistake
http://web-platform.test:8000/web-animations/timing-model/animations/setting-the-target-effect-of-an-animation.html - Setting the target effect to null causes a pending playback rate to be applied

The test function does not recognize the return value of the provided
test body. Authors may mistakenly use this API instead of
promise_test, a function which is designed to respond to promise
values. Previously, mistakes like this would produce tests that
spuriously passed.

Update the test function to fail immediately in response to a body
that returns a value. Also correct a test which exhibited the
programming mistake that this change is intended to discourage (a survey
using both the Chrome and Firefox browsers indicated that this is the
only test in WPT which would be affected by this change).

[tesrtharness.js] Disallow `test` return value
The `test` function does not recognize the return value of the provided
test body. Authors may mistakenly use this API instead of
`promise_test`, a function which is designed to respond to promise
values. Previously, mistakes like this would produce tests that
spuriously passed.

Update the `test` function to fail immediately in response to a body
that returns a value. Also correct a test which exhibited the
programming mistake that this change is intended to discourage (a survey
using both the Chrome and Firefox browsers indicated that this is the
only test in WPT which would be affected by this change).
@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Sep 12, 2018

Almost forgot: here's the branch I used to conduct the survey https://github.com/bocoup/wpt/commits/suspicious-sync

var value = test_obj.step(func, test_obj, test_obj);
test_obj.step(function() {
assert(value === undefined,

This comment has been minimized.

@jgraham

jgraham Sep 12, 2018

Contributor

This is OK, but if I was being picky I might do

var msg = "Test object returned a value " + value";
if value.hasOwnProperty("then") {
  msg += " Consider writing promise_test instead";
}
test_obj.set_status(test_obj.ERROR, msg);
test_obj.phase = test_obj.phases.HAS_RESULT;
test_obj.done();

This comment has been minimized.

@gsnedders

gsnedders Sep 12, 2018

Contributor

+1 for @jgraham's suggestion.

@wpt-pr-bot wpt-pr-bot requested a review from tobie Sep 13, 2018

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Sep 13, 2018

Thanks for the review! @jgraham's example referenced a non-existent property, test_obj.ERROR instead of test_obj.FAIL. I can guess why they made that mistake: this condition really is a harness error, not a test failure. By communicating it in those terms, the problem will be harder to miss if it crops up in a test that is expected to fail.

I've updated the patch to report via a harness error. In order to limit the number of test files, I'm using a different approach for managing the tests. I've also implemented the hint that @jgraham originally requested and included some extra code to protect from the wacky values that tests might return.

The original commit message is no longer accurate, but I'd be happy to update it once this patch is approved.

@jgraham

The implementation looks fine. The selftest looks a little over complex?

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Sep 17, 2018

The test for this needs to verify that a harness error is reported, so it can't be expressed using testharness.js alone. When I first automated this test suite, I extended the manual tests with JSON that describes how the harness is expected to behave. This seemed like the lowest-friction way to get us up and running, even if it did require specialized test execution logic.

Since then, I've recognized a few deficiencies with the approach. For one, the tests are difficult to author because the process involves manually writing JSON into an HTML file, because error reports are difficult to interpret, and because there is some ambiguity with how the unordered subtest results should be encoded in an array. Secondly, there can only be one harness error per file, so when you are writing many tests for harness errors (as in the case here), you are forced to express expectations across many files.

These problems were particularly frustrating in gh-8748, so I experimented with this approach there--see resources/test/tests/unit/exceptional-cases.html. I'm continuing that trend with these tests because I feel it's much more technically sound. I'm interested in converting all the existing tests to use this approach, and that should make things a little better still through the introduction of a shared utility function and the removal of the JSON-scraping interpretation logic. If you have specific recommendations for further simplification, I'm all ears!

@jugglinmike jugglinmike changed the title from [tesrtharness.js] Disallow `test` return value to [testharness.js] Disallow `test` return value Sep 17, 2018

@jgraham jgraham merged commit 2da097f into web-platform-tests:master Sep 17, 2018

1 check passed

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

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

Adapt to "Disallow `test` return value" change in wpt
The change in question:
web-platform-tests/wpt#12958

This change was done to flag possible test authoring mistakes. While
most of the updates in this CL are just adding braces arrow functions,
in set-root-scroller.html there was a genuine problem with the test.

In assert_selection.js, to avoid updating many many test using
`test(() => assert_selection(...), desc)`, change `assert_selection` to
not have a return value and add `assert_selection_and_return_sample`.

Bug: 892613
Change-Id: Idaee49e40a9d85f5dfb7124c35a0d864bdbf016f
Reviewed-on: https://chromium-review.googlesource.com/c/1268341
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597834}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment