Skip to content

Add assertClassification for ClassificationTests #1916

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
Jul 25, 2023
Merged

Add assertClassification for ClassificationTests #1916

merged 1 commit into from
Jul 25, 2023

Conversation

StevenWong12
Copy link
Contributor

I made use of some infrastructure we have in IncrementalParseTestUtils.swift to add a assertClassification function to make ClassificationTests look nicer.

This could also be a start effort to translate the coloring_* lit-based test cases into XCTest.

(This pr is based on #1905)

@StevenWong12 StevenWong12 changed the title Add assertClassification for ClassificationTests WIP: Add assertClassification for ClassificationTests Jul 17, 2023
ahoppen
ahoppen reviewed Jul 17, 2023
View reviewed changes
@StevenWong12 StevenWong12 force-pushed the warp_assert_classification branch from 4cc19ed to 84e8677 Compare July 19, 2023 01:11
@StevenWong12
Copy link
Contributor Author

I make the range in assertClassification is essentially the range we want to test in instead of the range of all tokens that touch this range, which I think more intuitive. What do you think?

@StevenWong12 StevenWong12 marked this pull request as ready for review July 19, 2023 01:18
@StevenWong12 StevenWong12 changed the title WIP: Add assertClassification for ClassificationTests Add assertClassification for ClassificationTests Jul 19, 2023
@ahoppen
Copy link
Member

ahoppen commented Jul 19, 2023

The key point of the classification tests that had a range was to test the range parameter of classification(in:) so we should continue to pass the range to the classification method.

@StevenWong12 StevenWong12 force-pushed the warp_assert_classification branch from 84e8677 to 21eacdc Compare July 19, 2023 05:17
@StevenWong12
Copy link
Contributor Author

Ah, I see. Since that, do you think we should also test the classification(at: ) function? (which may lead to splitting the assertClassification into three functions like before to make it look nicer.)

@ahoppen
Copy link
Member

ahoppen commented Jul 19, 2023

I think having a test case that tests classification(at:) does make sense. But since I don’t think we need a lot of them (one or two are sufficient), and the test cases are even smaller, I don’t think we’ll need a separate assert function for it. We should be able to just check kind and range manually like we do today.

@StevenWong12 StevenWong12 force-pushed the warp_assert_classification branch 2 times, most recently from adf8137 to 6f39d72 Compare July 20, 2023 14:27
ahoppen
ahoppen reviewed Jul 20, 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.

Two minor comments, otherwise LGTM.

@StevenWong12 StevenWong12 force-pushed the warp_assert_classification branch from 6f39d72 to 889fd0b Compare July 21, 2023 00:17
@ahoppen
Copy link
Member

ahoppen commented Jul 21, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Jul 21, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit bef63d8 into swiftlang:main Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

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

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