-
Notifications
You must be signed in to change notification settings - Fork 15
Adopt Swift Testing in metadata extractor tests #51
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
Adopt Swift Testing in metadata extractor tests #51
Conversation
…t of changes which are strictly necessary to convert XCTest API usages into their Swift Testing counterparts
…able and idiomatic
…efactor where they will be needed at other scopes
…focused test functions in a sub-suite
…ore focused test functions in a new sub-suite named `Malformed proposals` (after the snapshot it references)
…this for `Good dates` test function but it has many more arguments and its performance in Xcode is somewhat poor currently
|
@dempseyatgithub, how does this look? Offline, we discussed a concern about whether it was safe to adopt Swift 6.2 features in this repo yet, but if this Swift CI Jenkins job is what performs that build, it seems like this should be safe since it does not build the package's tests. Either way, I'm interested to hear your feedback and hopefully get this landed! Thanks. |
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 requested / suggested some changes.
If any of them seem way out of line for idiomatic Swift Testing, please let me know. Obviously, you have had much more experience with it than I have, this is my first serious foray.
The two preferences I have (noted with suggestions in the review) are to explicitly annotate suites with @Suite and to name them using title case so they appear more like a heading.
One other question/piece of feedback. I find that sometimes when a test is gathering arguments with a lot of set-up (like Warning and errors() in this file) the actual test - including seeing the name of the test, gets lost in the noise of gathering the arguments.
Do you have any suggestions to aid in that? Is it a poor stylistic choice to always use a test description with @Test as opposed to using a raw identifier? (I realize that leads to the 'name the thing twice' issue)
Thank you for your patience and for doing the migration to Swift Testing.
And also thank you for answering my questions. I am really excited about Swift Testing (I just rewatched the 2024 WWDC Videos before doing this review, and got enthused all over again - such a beautiful API design!)
|
|
||
| @testable import EvolutionMetadataModel | ||
| @testable import EvolutionMetadataExtraction | ||
|
|
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.
The Proposal type could use a custom test description.
I don't know what the best practice is for adding test descriptions. (As part of the original definition of the type, as part of the test bundle, etc.)
In the case, it is a small extension and likely will not be joined by other test descriptions, so defining at the top doesn't add much clutter, but makes it obvious where that description is coming from.
Would appreciate any feedback you have in terms of canonical Swift Testing as to where this should live, but Proposal definitely needs a custom test description.
| extension Proposal: CustomTestStringConvertible { | |
| public var testDescription: String { id.isEmpty ? "No ID" : id } | |
| } |
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.
Happy to include such an extension and conformance. In terms of where it should go, that's very much a style question and we, the maintainers of Swift Testing, don't really offer canonical guidance about that so I would apply whatever your preferred Swift code style is generally.
My own personal tendency would be to place it within the // MARK: - Helpers section, which in this file is near the bottom. I generally like the "main" declarations in a file to appear closer to the top. But it's purely subjective/stylistic.
Thanks for reviewing! A few suggestions do differ from my own personal style, but it's small stuff and I'm of course happy to conform to your coding style and norms. None of your suggestions are out of line or inappropriate with what I would consider recommended and supported Swift Testing usage patterns.
Yeah I was beginning to feel a few of those
I don't consider it a poor choice but I suspect we're going to begin de-emphasizing the original string-based display names and recommending raw identifiers more.
|
dempseyatgithub
left a comment
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 looks good. Thanks for the changes and for the work in converting the tests.
This PR adopts Swift Testing in place of XCTest in this package repo's tests.
Modifications
I approached these changes as follows:
@Testfunction (there were only a handful) looking for ways to make them more idiomatic Swift Testing-style tests:Testing performed
I ran these at-desk from Xcode 26.1 Beta and confirmed all modified tests are passing.
Fixes #27