-
Notifications
You must be signed in to change notification settings - Fork 441
Unify all parser tests to use an enhanced version AssertParse
#641
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
Conversation
@swift-ci Please test |
AssertParse
AssertParse
I designed it that way so that we could write computed text. But I suppose if we wanted to recover that, a local function would suffice. |
@@ -174,6 +110,9 @@ public struct SubtreeMatcher { | |||
|
|||
/// Same as `XCTAssertSameStructure`, but uses the subtree found from parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to replace this comment since XCTAssertSameStructure
doesn't exist any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I already added the replacement comment below, I just forgot to remove the original.
} | ||
} | ||
|
||
class FixItApplier: SyntaxRewriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems generally useful, any reason to make it test-only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are two reasons:
- It’s still incomplete. For example, there’s no way to specify which Fix-It should be applied if a diagnostic offers multiple. I’d like to flesh it out in tests first. If we’re happy with the result at some point, we can consider making it public.
- The Fixed-Up view of the syntax tree should provide this functionality to clients.
/// These markers are removed before parsing the source file. | ||
/// By default, `DiagnosticSpec` asserts that the diagnostics is produced at a location marked by `#^DIAG^#`. | ||
/// `parseSyntax` can be used to adjust the production that should be used as the entry point to parse the source code. | ||
/// If `substructure` is not `nil`, asserts that the parsed syntax tree contains this substructure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asserting substructure used to allow specifying a marker to start at as well. Is there any way to do that now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is not. I wanted to add it once we need it so that we actually have a test case that tests it.
|
||
/// Parse `markedSource` and perform the following assertions: | ||
/// - The parsed syntax tree should be printable back to the original source code (round-tripping) | ||
/// - Parsing produced the given `diagnostics` (`diagnostics = []` asserts that the parse was successful) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow nil
when we don't care about the diagnostics, or did you want to enforce that they're always specified if it isn't a successful parse?
EDIT: After looking at the rest, having this required seems reasonable to me.
DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected 'for'"), | ||
DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected ':'"), | ||
DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected ')'"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously not for this PR and even somewhat unrelated to these diagnostics - but should we limit the expected diagnostic to one per location? Not sure it's entirely useful to have both for
and :
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure it that’s easy to enforce. We only resolve source locations from syntax nodes when printing the diagnostics and before that, these diagnostics are anchored to different nodes (the missing for
token and the missing :
token). So the only place I think we could realistically enforce this would be AssertParse
.
} | ||
""", | ||
diagnostics: [ | ||
DiagnosticSpec(message: "Expected '->'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth specializing this one 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll add a FIXME.
// expected-error@-1 {{class name can only start with a letter or underscore, not a number}} | ||
// expected-error@-2 {{'c' is not a valid digit in integer literal}} | ||
func 24method() {} | ||
// expected-error@-1 {{function name can only start with a letter or underscore, not a number}} | ||
// expected-error@-2 {{'m' is not a valid digit in integer literal}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these left in to help with the FIXME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
This allows me to run all other tests in a few seconds.
Update `AssertParse` to take the a list of expected diagnostic specifications that we are expected to produce.
Now that `AssertParse` is able to assert for diagnostics, it doesn’t make sense to keep the diagnostic test cases in a separate test file.
The goal is to get rid of RecoveryTests.swift altogether once we’ve merged `XCTAssertHasSubstructure` with `AssertParse`.
…r in a syntax tree
These tests now also use `AssertParse` and should thus live next to all the other declaration parsing tests.
2382536
to
459da60
Compare
@swift-ci Please test |
This is an assortment of multiple test-only changes, migrating all tests to an enhanced version of
AssertParse
.SKIP_SELF_PARSE
to skip the self parsing test for local developmenttestSelfParse
reduces the test time from ~30 seconds to ~2 seconds and it’s really useful for local development to iterate faster.AssertParse
:XCTAssertSingleDiagnostics
:XCTAssertHasSubstructure
:XCTAssert[Single]Diagnostics
andXCTAssertHasSubstructure
and migrate those tests to the now enhancedAssertParse
#^MARKER^#
and assert that diagnostics are produced at the locations of these markersAssertParse
to take the source text as a proper parameter instead of a closure. I found the way of passing the source text via a closure rather confusing.Asserting for the diagnostics showed that we weren’t producing diagnostics in all expected cases and that some of the produced diagnostics are bad. These cases should be fixed in follow-up PRs but it won’t be the job of this PR because it’s quite large already.