This repository has been archived by the owner on Sep 19, 2018. It is now read-only.
Require "thenable" for promise_test
#252
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `promise_test` function is intended to run an asynchronous test, interpreting its status via a "thenable" object provided via return value. Previously, it would accept functions that did not provide such a value, and this meant that tests could fail silently. Because there is no use case for using `promise_test` in this way, the condition always reflects a programming error. Update the harness to explicitly fail the test whenever this occurs, prompting contributors to correct their usage prior to submitting the test.
|
This may not be a strong enough indicator. The existing code detects the bug I referenced above, but I suspect that test failures were expected when the patch was submitted, so it may not have been evident that the tests were failing for the wrong reason. @jgraham: do you think we should fail the test suite itself in this case? |
|
Yes, I think it makes sense to move this logic outside the |
|
Okay @jgraham, I've updated this to fail the harness. This version still causes |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
promise_testfunction is intended to run an asynchronous test, interpreting its status via a "thenable" object provided via return value. Previously, it would accept functions that did not provide such a value, and this meant that tests could fail silently. Because there is no use case for usingpromise_testin this way, the condition always reflects a programming error.Update the harness to explicitly fail the test whenever this occurs, prompting contributors to correct their usage prior to submitting the test.
@jgraham This is in response to web-platform-tests/wpt#5228 . It's difficult to say how many tests this change might effect; would you mind vetting it against WPT using Mozilla's testing infrastructure, like we did with #240 ?
This change is