-
Notifications
You must be signed in to change notification settings - Fork 263
Make XCTAssertEqual with accuracy more generic #294
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
Conversation
Equality with accuracy isn't limited to floating point tests. Sometimes integer (or other numeric types) need to be accurate within certain bounds for the purposes of testing. This change allows any numeric type that has magnitude and distance to be used with `XCTAssertEqual(_:_:accuracy:)` and `XCTAssertNotEqual(_:_:accuracy:)`.
Thanks @gormster !
I would recommend augmenting these two existing test files to add test coverage for your additions here:
|
Didn't bother changing the name of the test case, though. Seemed like a big change for no real benefit. Also fixed one malformed (though still working) regex in the existing test checks.
Cheers @stmontgomery. I just added some to NegativeAccuracyTestCase; seemed kind of pointless to have the exact same test in two places. |
@swift-ci test |
@swift-ci test |
Often if means the messages between jenkins and github got lost. |
@@ -171,10 +171,10 @@ public func XCTAssertEqual<T: Equatable>(_ expression1: @autoclosure () throws - | |||
} | |||
} | |||
|
|||
public func XCTAssertEqual<T: FloatingPoint>(_ expression1: @autoclosure () throws -> T, _ expression2: @autoclosure () throws -> T, accuracy: T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) { | |||
public func XCTAssertEqual<T: Strideable & Numeric>(_ expression1: @autoclosure () throws -> T, _ expression2: @autoclosure () throws -> T, accuracy: T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) { |
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've done some local experimentation with the version of this code which is included in Xcode, and I believe it's possible to simplify this code to only require conformance to the Numeric
protocol and remove the Strideable
requirement. I think the latter is needed currently because the code uses distance(to:)
to calculate differences, but I'm not sure if that way of getting the difference is strictly necessary. If we distill the core "are two numbers equal w/accuracy" calculation down to a helper function, I think that can look something like this:
private func areEqual<T : Numeric>(_ exp1: T, _ exp2: T, accuracy: T) -> Bool {
if exp1 == exp2 {
return true
} else {
// NaN values are handled implicitly, since the <= operator returns false when comparing any value to NaN.
let difference = (exp1.magnitude > exp2.magnitude) ? exp1 - exp2 : exp2 - exp1
return difference.magnitude <= accuracy.magnitude
}
}
And this removes the need for distance(to:)
and anything else which currently requires Strideable
.
Could we employ this technique in this PR too, so we can change the type requirements to only need Numeric
? (Or, if anyone knows a strong reason to keep the current implementation, let me know.) Thanks!
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
This was closed due to the branch change noted above, but I'm landing this in #319 |
Equality with accuracy isn't limited to floating point tests. Sometimes integer (or other numeric types) need to be accurate within certain bounds for the purposes of testing. This change allows any numeric type that has magnitude and distance to be used with
XCTAssertEqual(_:_:accuracy:)
andXCTAssertNotEqual(_:_:accuracy:)
.I'll be honest - I don't have a clue where in the Tests directory I'm supposed to put a regression test for this. Should I create a new test target? Seems a bit excessive, but I'm lost otherwise.