Skip to content
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

Use StaticString for file parameters in the XCTest overlay #888

Merged
merged 1 commit into from Jan 8, 2016

Conversation

briancroom
Copy link
Collaborator

The XCTest SDK overlay contains a set of XCTAssert* functions, each of which has file and line parameters, just as assert, fatalError, etc. do, however the XCAssert functions use String as the type of their file parameter, instead of StaticString as the other functions in the stdlib do for this purpose. This PR would change these parameters to be typed as StaticString.

See related discussion on SR-308 and apple/swift-corelibs-xctest#27 where @gribozavr suggested this change.

Advantages

  • Increases the consistency of how functions within the stdlib request file/line values
  • Brings the XCTest overlay functions in line with how they are defined in swift-corelibs-xctest
  • Minor efficiency improvement by using StaticString

Disadvantages

  • This is a breaking API change, although in a parameter that has a default value which is rarely overridden. This parameter is generally only used explicitly for when defining convenience assertion helpers which call into an XCTAssert* function. Such functions would have afileparameter themselves which would also need to be updated to useStaticString`.

Instead of just `String`. This makes it consistent with `fatalError`,
`assert`, etc., as well as `swift-corelibs-xctest`
@briancroom
Copy link
Collaborator Author

Looking forward to feedback on this! @gribozavr @mike-ferris-apple @modocache

@mike-ferris
Copy link
Contributor

I think this is a good change.

mike-ferris pushed a commit that referenced this pull request Jan 8, 2016
Use StaticString for `file` parameters in the XCTest overlay
@mike-ferris mike-ferris merged commit d30df98 into apple:master Jan 8, 2016
@briancroom
Copy link
Collaborator Author

Thanks! Happy to see this go through.

@briancroom briancroom deleted the use_StaticString branch January 8, 2016 00:11
@jrose-apple
Copy link
Contributor

This broke one of our internal tests; the offending code was something like this:

XCTFail("No such thingy: \(thingyID)")

I can't see us being the only ones who would do this, so I think we need to revert this. If we want to avoid string construction, we could consider making the message parameters be autoclosures instead.

@briancroom
Copy link
Collaborator Author

@jrose-apple Hmm that's weird - this change contains no changes to the type of the message parameter of the assertion methods, only to the file parameter. Can you provide any extra context on the error?

@jrose-apple
Copy link
Contributor

Oops, I'm sorry. I completely misunderstood the problem and oversimplified the example.

XCTFail("No such thingy: \(thingyID)", file: file, line: line)

So never mind, this was accounted for in the original request.

@mxcl
Copy link

mxcl commented Jan 8, 2016

The tests that broke are possibly because I was already working around this:

https://github.com/apple/swift-package-manager/blob/master/Tests/Functional/Utilities.swift#L18-L22

#if os(Linux)
typealias MyString = StaticString
#else
typealias MyString = String
#endif

@norio-nomura
Copy link
Contributor

This broke SwiftLint's test. realm/SwiftLint#425
The test expands results into XCTFail() that linting SwiftLint's work tree.
This changes disallow generating XCTest's assertion for dynamically located file.

NachoSoto added a commit to NachoSoto/Nimble that referenced this pull request Jan 27, 2016
Originally brought up in Quick#242, `XCTest` methods now take `StaticString` (see apple/swift#888 for details), so this won't compile just yet.
NachoSoto added a commit to NachoSoto/Nimble that referenced this pull request Jan 27, 2016
Originally brought up in Quick#242, `XCTest` methods now take `StaticString` (see apple/swift#888 for details). Unfortunately this won't compile now because `StaticString` is not available in Objective-C!
@briancroom
Copy link
Collaborator Author

This is also posing a challenge for the Nimble test matcher framework (see here). We expose both Swift and Objective-C APIs which funnel into XCTFail to report failed assertions, however StaticString doesn't bridge to ObjC which breaks the current approach to integration.

Conceptually, the __FILE__ preprocessor macro is pretty equivalent to Swift's __FILE__ value, although I imagine what the compiler emits isn't compatible. Does anyone know whether it would be at all feasible to introduce a mechanism for creating StaticString values using the output from the Objective-C __FILE__ macro?

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.

None yet

5 participants