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

Add the ability for MTKTestable test(_:) blocks to throw #37

Merged
merged 6 commits into from Dec 19, 2018

Conversation

schrismartin
Copy link

XCTest allows test methods to denote a test failure by marking the test as throwing. This makes the testing of throwing functions a tad simpler.

func testThrowingFunctionSucceeds() throws {
    try doSomethingThatShouldNotFail()
}

This tends to be much more elegant than something like this:

func testThrowingFunctionSucceeds() {
    do {
        try doSomethingThatShouldNotFail()
    }
    catch {
        XCFail("An error occurred: \(error)")
    }
}

In the same light, the test(_:) function could be much more ergonomic by allowing uncaught errors to fail a test via propagation.

func testThrowingMethodSucceeds() throws {
    try Object.test { instance in
        try instance.doSomethingThatShouldNotFail()
    }
}

The test(_:) function was marked rethrows to allow the user to forego this functionality while testing functions that don't throw. This prevents the cruft of having to try non-throwing functionality within a test when unnecessary.

func testNonThrowingMethod() {
    Object.test { instance in
        instance.doSomethingThatCannotFail()
        
        // Make assertions below...
    }
}

This PR is source breaking – anything that is already using the test(_:) method will need to add the throws and rethrows keywords. This should be a minimal change, all of which can be caught by the compiler due to a Type does not conform to protocol 'MTKTestable' compiler error.

@schrismartin schrismartin changed the title Add the ability for test blocks to throw Add the ability for MTKTestable test(_:) blocks to throw Nov 14, 2018
Copy link
Contributor

@nhgrif nhgrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README needs to be updated.

The consequence of this change is that suddenly everything that uses the Testable protocol now needs to be marked with a try, and then additionally wrapped in something that can handle it (either XCTAssertThrowsError, a do-catch block, or marking the testing function as throws... or I guess using try?/try!). I definitely understand the convenience of this in cases where thrown errors should cause a test to fail, but what does this do for all the probably more common cases where the code in the test block doesn't even have a path to throw an error?

Is there a path to more cleanly implement this so we only have to add the error handling paths when we do want to test for this?

For example, what does it look like if failing on thrown errors is the default path?

static func test(_ testBlock: (Self) throws -> Void) {
    let testVC = instanceForTesting()
    testVC.loadView()
    testVC.viewDidLoad()
    do {
      try testBlock(testVC)
    }
    catch let error {
      XCTFail("An error occurred: \(error)")
    }
}

(noting that if we do take this path, the test method needs to take in file/line arguments and pass them to that XCTFail message.

With this pattern, we don't break existing code. We also remove the requirement for adding try any time anyone wants to use this functionality.

In the case that we do want the testBlock to throw something, we've eliminated the need to write a bunch of boiler plate, the original goal of this PR.

This pattern also has one extra added benefit over your PR: if there is more test code after the call to test, it still gets to run. If the call to test itself throws us out of the test method, none of the code after it will have an opportunity to execute.

@schrismartin
Copy link
Author

@nhgrif

This is the beauty of rethrows! Similarly to map's functionality...

public func map<T>(_ transform: (Element) throws -> T) rethrows -> [T]

You don't have to try to map(_:) every time you use it, only when the closure inside map(_:) throws. Similarly, you'll only need to use try in front of test(_:) when the closure inside test(_:) throws an error. I've fixed the function signature inside of the default UIViewController implementation, but you will need to modify the signature in other places where it's used. Fortunately, this will be caught by the compiler and is relatively easy to fix with a find/replace.

... as for the README and test, I'll make some changes there.

@nhgrif
Copy link
Contributor

nhgrif commented Nov 29, 2018

@schrismartin Use Cmd+Option+/ on the functions you're modifying to get it to regen the doc comment. It will expect comments on what error the function can throw.

@nhgrif nhgrif merged commit 62d5741 into develop Dec 19, 2018
@nhgrif nhgrif deleted the feature/MTKTestable-throwing branch December 19, 2018 20:52
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

2 participants