TestCodable: Replace fatalError() with XCTFail #1285
Conversation
|
@swift-ci please test |
TestFoundation/TestCodable.swift
Outdated
| } | ||
|
|
||
| let decoded: T | ||
| do { | ||
| decoded = try decode(data) | ||
| } catch { | ||
| fatalError("Unable to decode \(T.self) <\(value)>: \(error)") | ||
| XCTFail("Unable to decode \(T.self) <\(value)>: \(error)") |
There was a problem hiding this comment.
Should this just be returning false and leaving the XCTFail for the higher level call?
There was a problem hiding this comment.
I left it in because it acts as a handy debug message in the failure case and isn't any noisier if the decode works.
TestFoundation/TestCodable.swift
Outdated
| let data: Data | ||
| do { | ||
| data = try encode(value) | ||
| } catch { | ||
| fatalError("Unable to encode \(T.self) <\(value)>: \(error)") | ||
| XCTFail("Unable to encode \(T.self) <\(value)>: \(error)") | ||
| return false |
There was a problem hiding this comment.
Personally I would prefer not to have a return value of false which may forget to be called by the calling site. Instead, you should pass in the message into the expect() call and then there's no need to have the XCTAssertTrue sprinkled throughout the code base.
In other words, I like the idea of fatalError -> XCTFail change, but I don't like the modification of return type to Bool or the other changes in this file.
There was a problem hiding this comment.
If the return value isn't acted upon then a compiler warning is emitted.
The advantage of having the XCTAssert.. in the test_() function is that failing tests show up more easily when running the tests under Xcode, when run as normal unit tests and not via TestFoundation. This makes it quicker to know what is failing as Xcode will highlight the failing test rather than having to step though the code.
There was a problem hiding this comment.
If we're going to do something which requires the caller take action (as opposed to merely a warning when it doesn't work) then we should change this from returning a Bool to making it a throw, which will explicitly force unadorned calls to be handled. What do you think?
There was a problem hiding this comment.
Yes, agreed a throw is better since it causes and error rather than a warning if not handled. I will add a commit to see what it looks like.
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
There was a problem hiding this comment.
I would like @itaiferber to comment on this PR. It's on my list to get JSONEncoder and its tests back in sync with the overlay, so I'd like to confirm that these changes could be made over at https://github.com/apple/swift/blob/master/test/stdlib/CodableTests.swift too 😀
|
Looks good to me. WDYT @itaiferber ? |
|
Sorry for the delay; will review today when I get a moment. |
|
Looks reasonable to me! The stdlib test runner runs every test in a separate process so failing via fatalError is okay over there; this is much better for XCTest infrastructure though. |
|
@swift-ci please test and merge |
Having tests abort via
fatalError()is not as useful as just failing the tests in situations where further tests can still be run.