Encode expected error "phase" #778
Merged
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
Improve test consistency by using the `assert.throws` helper function to assert runtime exceptions. Remove superfluous code.
These tests specifically concern error produced from the global scope, precluding the use of the `assert.throws` helper function.
The expected errors in these tests cannot be asserted with the `assert.throws` helper function for various reasons. Re-format their meta-data according to the latest design in order to more precisely describe test expectations.
Extend the test generation tool to emit the recently-modified format of the "negative" meta-data. Update the effected test case files accordingly.
Authored via the following command:
$ find test -type f -print0 | \
xargs -0 sed \
-i 's/^\(\s*\)negative:\s*SyntaxError\s*$/\1negative:\n\1 phase: early\n\1 type: SyntaxError/g'
Because expectations regarding error "phase" are now expressed via test meta-data, the test runner may now enforce this requirement on negative tests. Remove the "NotEarlyError" from the project source. This reduces the amount of domain knowledge required to author tests and lessens the potential for inconsistencies between tests.
|
Looks good! Thanks so much for rebasing and breaking down the commits so they are easy to read and review! |
|
LGTM |
|
Thank you both!! |
jugglinmike
added a commit
to jugglinmike/test262-harness
that referenced
this pull request
Jun 30, 2017
In October of 2016, the frontmatter format of Test262 tests was updated to include more information about expected errors [1]. Prior to that change, the `negative` attribute was defined as a regular expression that matched the expected error's `name` attribute. Following that change, the `negative` attribute is defined as a dictionary containing `phase` and `type` attributes. The `type` attribute is a string value describing the expected error's `name` attribute. Update the validation logic to interpret the meta data according to this new format. [1] tc39/test262#778
This was referenced Jun 30, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I have rebased the patch previously submitted at gh-655. Please refer to that
pull request for motivation, design discussion, and suggestions for merging
strategy. This version applies the patterns established there; it contains no
substantial changes other than updating the error format of recently-introduced
tests.
I have verified the validity of the change set by running the tests in a recent
build of V8, first on Test262's
masterbranch and then on this branch. Thereported errors are almost identical; this factoring happens to catch a handful
of additional violations in that runtime:
...all of these owe to the same basic flaw, which is well-known to the V8 team:
https://bugs.chromium.org/p/v8/issues/detail?id=896
@tcare @leobalter I would like to avoid having to rebase this work a third
time--if at all possible, can you please prioritize review? Thank you!