Modify assertMacroExpansion() so that it only conditionally calls XCTFail().
#2403
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…XCTFail()`. This PR changes the signature of `assertMacroExpansion()` such that an alternative failure handler can be passed for use when e.g. swift-testing is being used. Right now, the function calls `XCTAssertEqual()`, `XCTFail()`, etc. on the assumption that the caller is using XCTest. This assumption does not always hold and the dependency on XCTest may prevent a developer from writing effective tests leveraging `assertMacroExpansion()` while also using swift-testing or other testing libraries. It is not possible for swift-syntax to include swift-testing as a dependency at this time, so the function cannot be amended to directly call the equivalent of `XCTFail()` from that library. Nor can the function check if the current process is running an XCTest-based test because XCTest does not expose API to determine if a test is running. Until such time as swift-syntax can directly call into swift-testing, developers can do so when calling `assertMacroExpansion()` like so: ```swift assertMacroExpansion( "@foo bar", "expanded_foo bar", macros: ..., ... ) { message, fileID, filePath, line, column in Issue.record(Comment(rawValue: message), fileID: fileID, filePath: String(describing: filePath), Int(line), Int(column)) } ``` An improved interface is a future direction. Resolves #2400.
|
@swift-ci please test |
ahoppen
reviewed
Dec 13, 2023
Comment on lines
+78
to
+87
| if note.message != spec.message { | ||
| let message = describeDifferenceBetweenStrings(note.message, spec.message, "message of note does not match") | ||
| failureHandler( | ||
| message, | ||
| spec.originatorFileID, | ||
| spec.originatorFilePath, | ||
| spec.originatorLine, | ||
| spec.originatorColumn | ||
| ) | ||
| } |
There was a problem hiding this comment.
What do you think about having a assertStringsEqualWithDiff version that takes a failureHandler. I think that would make that this comparison is essentially doing an assertion.
(same in the other locations where you replaced assertStringsEqualWithDiff )
Comment on lines
+211
to
+214
| /// - originatorFileID: The file ID at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | ||
| /// - originatorFilePath: The file path at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | ||
| /// - originatorLine: The line at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | ||
| /// - originatorColumn: The column at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. |
There was a problem hiding this comment.
Suggested change
| /// - originatorFileID: The file ID at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | |
| /// - originatorFilePath: The file path at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | |
| /// - originatorLine: The line at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | |
| /// - originatorColumn: The column at which this ``NoteSpec`` was created, so that assertion failures can be reported at its location. | |
| /// - originatorFileID: The file ID at which this ``DiagnosticSpec`` was created, so that assertion failures can be reported at its location. | |
| /// - originatorFilePath: The file path at which this ``DiagnosticSpec `` was created, so that assertion failures can be reported at its location. | |
| /// - originatorLine: The line at which this ``DiagnosticSpec `` was created, so that assertion failures can be reported at its location. | |
| /// - originatorColumn: The column at which this ``DiagnosticSpec `` was created, so that assertion failures can be reported at its location. |
Comment on lines
+89
to
+97
| if location.line != spec.line { | ||
| failureHandler( | ||
| "line \(location.line) of note does not match \(spec.line)", | ||
| spec.originatorFileID, | ||
| spec.originatorFilePath, | ||
| spec.originatorLine, | ||
| spec.originatorColumn | ||
| ) | ||
| } |
There was a problem hiding this comment.
Similar here, what about having assertEqual that takes a failureHandler?
(same in the other locations where you replaced XCTAssertEqual)
Comment on lines
-37
to
+40
| internal let originatorFile: StaticString | ||
| internal let originatorFileID: String | ||
| internal let originatorFilePath: StaticString | ||
| internal let originatorLine: UInt | ||
| internal let originatorColumn: UInt |
There was a problem hiding this comment.
Instead of storing/passing all these 4 members every single time, what do you think about introducing a struct that holds them? That should also simplify the signature of failureHandler and make failureHandler extensible if we choose to add one of the parameters / deprecate one of them like filePath.
| @@ -299,6 +423,7 @@ func assertDiagnostic( | |||
| /// - testModuleName: The name of the test module to use. | |||
| /// - testFileName: The name of the test file name to use. | |||
| /// - indentationWidth: The indentation width used in the expansion. | |||
| /// - failureHandler: A closure to call if the assertion fails. This closure may be called more than once. If `nil`, `XCTFail()` is called. | |||
There was a problem hiding this comment.
Suggested change
| /// - failureHandler: A closure to call if the assertion fails. This closure may be called more than once. If `nil`, `XCTFail()` is called. | |
| /// - failureHandler: A closure to call if the assertion fails. This closure may be called more than once. If `nil` defaults to `XCTFail`. |
bnbarham
reviewed
Dec 14, 2023
Comment on lines
+404
to
+405
| // FIXME: detect if XCTest is currently running a test and only call XCTFail() if it is. | ||
| // FIXME: support swift-testing. |
There was a problem hiding this comment.
I'm not a huge fan of FIXME's that don't have corresponding issues. I'd just leave this out and file an issue to support swift-testing by default when we can.
|
I don't think we're pursuing the right solution here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the signature of
assertMacroExpansion()such that an alternative failure handler can be passed for use when e.g. swift-testing is being used.Right now, the function calls
XCTAssertEqual(),XCTFail(), etc. on the assumption that the caller is using XCTest. This assumption does not always hold and the dependency on XCTest may prevent a developer from writing effective tests leveragingassertMacroExpansion()while also using swift-testing or other testing libraries.It is not possible for swift-syntax to include swift-testing as a dependency at this time, so the function cannot be amended to directly call the equivalent of
XCTFail()from that library. Nor can the function check if the current process is running an XCTest-based test because XCTest does not expose API to determine if a test is running.Until such time as swift-syntax can directly call into swift-testing, developers can do so when calling
assertMacroExpansion()like so:An improved interface is a future direction.
Resolves #2400.