Skip to content

Add utils for incremental parse test #1712

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2023
Merged

Add utils for incremental parse test #1712

merged 1 commit into from
Jun 3, 2023

Conversation

StevenWong12
Copy link
Contributor

@StevenWong12 StevenWong12 commented May 28, 2023 •
edited
Loading

This change should allow us to verify the correctness of incremental parse with a single function, containing round-trip, substructure, and reused nodes validation like assertParse.

@StevenWong12 StevenWong12 requested a review from ahoppen as a code owner May 28, 2023 17:22
@StevenWong12 StevenWong12 force-pushed the add_utils_for_incremental_parse_test branch from 7cc635a to c62a189 Compare May 28, 2023 17:24
ahoppen
ahoppen reviewed May 30, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think we are going in the right direction by having a single assert function that performs all these checks because that did turn out super valuable for assertParse. I’ve got a few design comments inline.

@StevenWong12
Copy link
Contributor Author

What do you think about cherry-picking this commit to the functionality pr (#1685) and just close this one? Since we could just test this util with the incremental parse test case. (I don't know how assertParse was tested)

@StevenWong12 StevenWong12 force-pushed the add_utils_for_incremental_parse_test branch 2 times, most recently from 692ab16 to d74d81e Compare May 31, 2023 11:05
ahoppen
ahoppen reviewed May 31, 2023
View reviewed changes
Tests/SwiftParserTest/IncrementalParsingTests.swift
Comment on lines +21 to +25
assertIncrementalParse(
"""
struct A⏩️⏸️A⏪️ { func f() {
"""
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look so nice and short now 🤩

@ahoppen
Copy link
Member

ahoppen commented May 31, 2023

What do you think about cherry-picking this commit to the functionality pr (#1685) and just close this one? Since we could just test this util with the incremental parse test case. (I don't know how assertParse was tested)

I think keeping this as a standalone PR makes sense. It provides value on its own and will allow us to keep the PR that actually implements incremental parsing focussed. Also: I think we’ve got this PR really close to being merged.

@StevenWong12 StevenWong12 force-pushed the add_utils_for_incremental_parse_test branch from d74d81e to 7548b83 Compare June 1, 2023 07:35
@StevenWong12
Copy link
Contributor Author

Great! let's just keep this.

@StevenWong12 StevenWong12 requested a review from ahoppen June 1, 2023 07:36
ahoppen
ahoppen reviewed Jun 1, 2023
View reviewed changes
ahoppen
ahoppen reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I’ve got a few minor comments left, otherwise LGTM.

Sources/_SwiftSyntaxTestSupport/IncrementalParseTestUtils.swift Outdated
line: line
)

for targetNode in expectedReusedNodes {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren’t really nodes, right? Nitpick: targetNode is a little ambiguous. What do you think about expectedReusedNode?

Copy link
Contributor Author

@StevenWong12 StevenWong12 Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these aren't actually nodes. I just can't come up with a more proper name for them😂. Do you have any idea about the name? I've renamed targetNode to expectedReusedNode.

@ahoppen
Copy link
Member

ahoppen commented Jun 1, 2023

Also, I think you might to need to rebase on top of main and run swift-format to pick up #1714.

@StevenWong12 StevenWong12 force-pushed the add_utils_for_incremental_parse_test branch from 7548b83 to 09bb68a Compare June 2, 2023 03:27
@StevenWong12
Copy link
Contributor Author

I've rebased this pr and re-run swift-format.

@kimdv
Copy link
Contributor

kimdv commented Jun 2, 2023

@swift-ci please test

@kimdv
Copy link
Contributor

kimdv commented Jun 2, 2023

@swift-ci please test windows

@ahoppen
Copy link
Member

ahoppen commented Jun 2, 2023

Looks like XCTExpectFailure is not available in XCTest on Linux. So we need to continue using

try XCTSkipIf(true, "Swift parser does not handle node reuse yet")

@StevenWong12 StevenWong12 force-pushed the add_utils_for_incremental_parse_test branch from 09bb68a to 259542b Compare June 3, 2023 01:49
@StevenWong12
Copy link
Contributor Author

Ah, I see. Already fixed this.

@ahoppen
Copy link
Member

ahoppen commented Jun 3, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge June 3, 2023 02:29
@ahoppen ahoppen merged commit 65ad81a into swiftlang:main Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StevenWong12 @ahoppen @kimdv