Skip to content

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

Merged
merged 6 commits into from
Aug 29, 2022

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 26, 2022

This is an assortment of multiple test-only changes, migrating all tests to an enhanced version of AssertParse.

  • Add an environment variable SKIP_SELF_PARSE to skip the self parsing test for local development
    • Not running testSelfParse reduces the test time from ~30 seconds to ~2 seconds and it’s really useful for local development to iterate faster.
  • Add the following capabilities to AssertParse:
    • From XCTAssertSingleDiagnostics:
      • Check which diagnostics are produced
      • Check that applying the Fix-Its from the diagnostics produces an expected fixed source text
    • From XCTAssertHasSubstructure:
      • Check that the parsed syntax tree contains a substructure
  • Remove XCTAssert[Single]Diagnostics and XCTAssertHasSubstructure and migrate those tests to the now enhanced AssertParse
  • Allow adding markers to the source code of the form #^MARKER^# and assert that diagnostics are produced at the locations of these markers
  • Move tests from RecoveryTests.swift and DiagnosticTests.swift to the other test files because the tests are now no longer fundamentally different than the parsing tests in these files
  • Change AssertParse 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.

@ahoppen ahoppen requested review from rintaro, bnbarham and CodaFi August 26, 2022 14:54
@ahoppen
Copy link
Member Author

ahoppen commented Aug 26, 2022

@swift-ci Please test

@ahoppen ahoppen changed the title Unify all tests to use an enhanced version AssertParse Unify all parser tests to use an enhanced version AssertParse Aug 26, 2022
@CodaFi
Copy link
Contributor

CodaFi commented Aug 26, 2022

Change AssertParse 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.

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
Copy link
Contributor

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?

Copy link
Member Author

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 {
Copy link
Contributor

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?

Copy link
Member Author

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.
Copy link
Contributor

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?

Copy link
Member Author

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)
Copy link
Contributor

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.

Comment on lines +15 to +17
DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected 'for'"),
DiagnosticSpec(locationMarker: "DIAG_1", message: "Expected ':'"),
DiagnosticSpec(locationMarker: "DIAG_2", message: "Expected ')'"),
Copy link
Contributor

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.

Copy link
Member Author

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 '->'")
Copy link
Contributor

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 🤔

Copy link
Member Author

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.

Comment on lines +462 to +467
// 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}}
Copy link
Contributor

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?

Copy link
Member Author

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`.
These tests now also use `AssertParse` and should thus live next to all the other declaration parsing tests.
@ahoppen ahoppen force-pushed the ahoppen/test-improvements branch from 2382536 to 459da60 Compare August 28, 2022 06:52
@ahoppen
Copy link
Member Author

ahoppen commented Aug 28, 2022

@swift-ci Please test

@ahoppen ahoppen merged commit bb62ba2 into swiftlang:main Aug 29, 2022
@ahoppen ahoppen deleted the ahoppen/test-improvements branch August 29, 2022 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants