Add parse failure tests #1820
Merged
Add parse failure tests #1820
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
Adds test cases that validate the parser fails when expected by
asserting that any descendant of the root is an error tree. There
is a test per-production, if that production has terminal tokens,
or represents an alternation of productions that are distinguished
by terminal tokens. For example, SHAPE_OR_APPLY_STATEMENT chooses
between shape and apply statements based on whether the current
lexemme is "apply".
Only cases where the terminal tokens are invalid are tested because
if a non-terminal (ie. another production) is invalid, the error tree
is a child of the tree for that production. Since we also have tests
that make sure the correct children are present for each production,
we should have enough coverage to only test invalid non-terminals.
For example, when parsing this shape statement:
```
structure Foo.Bar {}
```
we know the `Foo.Bar` is part of the IDENTIFIER production, we have
tests that validate the the IDENTIFIER production is present, and
we have tests that check for errors in IDENTIFIERs, so we don't
need to specifically test this kind of error for shape statements.
One caveat is that the existing tests I'm referring to only ensure
all expected children are present for valid trees, so there is a
logical inconsistency in the argument that these tests provide full
coverage, but I can't think of a case that could cause a problem.
Either way, these tests should provide _good enough_ coverage.
Some bug fixes:
- Fixed an issue where the error caused by not finding a "namespace"
in NAMESPACE_STATEMENT was recovered by the parent tree.
- Expect IDENTIFIER for "with" token in MIXINS production.
mtdowling
requested changes
Jun 13, 2023
smithy-syntax/src/main/java/software/amazon/smithy/syntax/TreeType.java
Outdated
Show resolved
Hide resolved
Adds more test cases for `*Statement` productions to ensure loops are broken out of properly when parsing fails.
mtdowling
approved these changes
Jun 15, 2023
mtdowling
pushed a commit
that referenced
this pull request
Jun 16, 2023
* Add parse failure tests
Adds test cases that validate the parser fails when expected by
asserting that any descendant of the root is an error tree. There
is a test per-production, if that production has terminal tokens,
or represents an alternation of productions that are distinguished
by terminal tokens. For example, SHAPE_OR_APPLY_STATEMENT chooses
between shape and apply statements based on whether the current
lexemme is "apply".
Only cases where the terminal tokens are invalid are tested because
if a non-terminal (ie. another production) is invalid, the error tree
is a child of the tree for that production. Since we also have tests
that make sure the correct children are present for each production,
we should have enough coverage to only test invalid non-terminals.
For example, when parsing this shape statement:
```
structure Foo.Bar {}
```
we know the `Foo.Bar` is part of the IDENTIFIER production, we have
tests that validate the the IDENTIFIER production is present, and
we have tests that check for errors in IDENTIFIERs, so we don't
need to specifically test this kind of error for shape statements.
One caveat is that the existing tests I'm referring to only ensure
all expected children are present for valid trees, so there is a
logical inconsistency in the argument that these tests provide full
coverage, but I can't think of a case that could cause a problem.
Either way, these tests should provide _good enough_ coverage.
Some bug fixes:
- Fixed an issue where the error caused by not finding a "namespace"
in NAMESPACE_STATEMENT was recovered by the parent tree.
- Expect IDENTIFIER for "with" token in MIXINS production.
* Add cases for productions with loops
Adds more test cases for `*Statement` productions to ensure loops
are broken out of properly when parsing fails.
mtdowling
pushed a commit
that referenced
this pull request
Jun 19, 2023
* Add parse failure tests
Adds test cases that validate the parser fails when expected by
asserting that any descendant of the root is an error tree. There
is a test per-production, if that production has terminal tokens,
or represents an alternation of productions that are distinguished
by terminal tokens. For example, SHAPE_OR_APPLY_STATEMENT chooses
between shape and apply statements based on whether the current
lexemme is "apply".
Only cases where the terminal tokens are invalid are tested because
if a non-terminal (ie. another production) is invalid, the error tree
is a child of the tree for that production. Since we also have tests
that make sure the correct children are present for each production,
we should have enough coverage to only test invalid non-terminals.
For example, when parsing this shape statement:
```
structure Foo.Bar {}
```
we know the `Foo.Bar` is part of the IDENTIFIER production, we have
tests that validate the the IDENTIFIER production is present, and
we have tests that check for errors in IDENTIFIERs, so we don't
need to specifically test this kind of error for shape statements.
One caveat is that the existing tests I'm referring to only ensure
all expected children are present for valid trees, so there is a
logical inconsistency in the argument that these tests provide full
coverage, but I can't think of a case that could cause a problem.
Either way, these tests should provide _good enough_ coverage.
Some bug fixes:
- Fixed an issue where the error caused by not finding a "namespace"
in NAMESPACE_STATEMENT was recovered by the parent tree.
- Expect IDENTIFIER for "with" token in MIXINS production.
* Add cases for productions with loops
Adds more test cases for `*Statement` productions to ensure loops
are broken out of properly when parsing fails.
syall
pushed a commit
to Xtansia/smithy
that referenced
this pull request
Aug 11, 2023
* Add parse failure tests
Adds test cases that validate the parser fails when expected by
asserting that any descendant of the root is an error tree. There
is a test per-production, if that production has terminal tokens,
or represents an alternation of productions that are distinguished
by terminal tokens. For example, SHAPE_OR_APPLY_STATEMENT chooses
between shape and apply statements based on whether the current
lexemme is "apply".
Only cases where the terminal tokens are invalid are tested because
if a non-terminal (ie. another production) is invalid, the error tree
is a child of the tree for that production. Since we also have tests
that make sure the correct children are present for each production,
we should have enough coverage to only test invalid non-terminals.
For example, when parsing this shape statement:
```
structure Foo.Bar {}
```
we know the `Foo.Bar` is part of the IDENTIFIER production, we have
tests that validate the the IDENTIFIER production is present, and
we have tests that check for errors in IDENTIFIERs, so we don't
need to specifically test this kind of error for shape statements.
One caveat is that the existing tests I'm referring to only ensure
all expected children are present for valid trees, so there is a
logical inconsistency in the argument that these tests provide full
coverage, but I can't think of a case that could cause a problem.
Either way, these tests should provide _good enough_ coverage.
Some bug fixes:
- Fixed an issue where the error caused by not finding a "namespace"
in NAMESPACE_STATEMENT was recovered by the parent tree.
- Expect IDENTIFIER for "with" token in MIXINS production.
* Add cases for productions with loops
Adds more test cases for `*Statement` productions to ensure loops
are broken out of properly when parsing fails.
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.
Adds test cases that validate the parser fails when expected by asserting that any descendant of the root is an error tree. There is a test per-production, if that production has terminal tokens, or represents an alternation of productions that are distinguished by terminal tokens. For example, SHAPE_OR_APPLY_STATEMENT chooses between shape and apply statements based on whether the current lexemme is "apply".
Only cases where the terminal tokens are invalid are tested because if a non-terminal (ie. another production) is invalid, the error tree is a child of the tree for that production. Since we also have tests that make sure the correct children are present for each production, we should have enough coverage to only test invalid non-terminals. For example, when parsing this shape statement:
we know the
Foo.Baris part of the IDENTIFIER production, we have tests that validate the the IDENTIFIER production is present, and we have tests that check for errors in IDENTIFIERs, so we don't need to specifically test this kind of error for shape statements.One caveat is that the existing tests I'm referring to only ensure all expected children are present for valid trees, so there is a logical inconsistency in the argument that these tests provide full coverage, but I can't think of a case that could cause a problem. Either way, I think these tests provide good enough coverage.
Some bug fixes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.