Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

gormster
Copy link
Contributor

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:).

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.

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:)`.
@stmontgomery stmontgomery self-assigned this Jan 10, 2020
@stmontgomery
Copy link
Contributor

Thanks @gormster !

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.

I would recommend augmenting these two existing test files to add test coverage for your additions here:

Tests/Functional/NegativeAccuracyTestCase/main.swift
Tests/Functional/FailureMessagesTestCase/main.swift

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.
@gormster
Copy link
Contributor Author

Cheers @stmontgomery. I just added some to NegativeAccuracyTestCase; seemed kind of pointless to have the exact same test in two places.

@spevans
Copy link
Contributor

spevans commented Jan 13, 2020

@swift-ci test

@gormster
Copy link
Contributor Author

gormster commented Jan 15, 2020

weird - the builds both succeeded (linux, mac) but are both still showing as waiting for status to be reported. something up with Jenkins, or just slow?

@spevans
Copy link
Contributor

spevans commented Jan 15, 2020

@swift-ci test

@spevans
Copy link
Contributor

spevans commented Jan 15, 2020

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) {
Copy link
Contributor

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!

@shahmishal shahmishal closed this Oct 6, 2020
@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please rebase on top of the main and we can reopen.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

@stmontgomery
Copy link
Contributor

This was closed due to the branch change noted above, but I'm landing this in #319

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.

4 participants