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

tests: improve tests that are supposed to throw #1430

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

weissi
Copy link
Member

@weissi weissi commented Mar 3, 2020

Motivation:

The

do {
    try someOperation()
XCTFail("should throw") // easy to forget
} catch error as SomethingError {
    XCTAssertEqual(.something, error as? SomethingError)
} catch {
    XCTFail("wrong error")
}

pattern is not only very long, it's also very error prone. If you forget
any of the XCTFails, you might not tests what it looks like

XCTAssertThrowsError(try someOperation) { error in
    XCTAssertEqual(.something, error as? SomethingError)
}

is much safer and shorter.

Modifcations:

Do many of the above replaces.

Result:

Cleaner, shorter, and safer tests.

Motivation:

The

    do {
        try someOperation()
	XCTFail("should throw") // easy to forget
    } catch error as SomethingError {
        XCTAssertEqual(.something, error as? SomethingError)
    } catch {
        XCTFail("wrong error")
    }

pattern is not only very long, it's also very error prone. If you forget
any of the XCTFails, you might not tests what it looks like

    XCTAssertThrowsError(try someOperation) { error in
        XCTAssertEqual(.something, error as? SomethingError)
    }

is much safer and shorter.

Modifcations:

Do many of the above replaces.

Result:

Cleaner, shorter, and safer tests.
} catch ChannelError.alreadyClosed {
return
return // as well as already closed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any value in something like assertMaybeThrows which validates the error if one is thrown but doesn't fail if no error is thrown? I think I've only seen one or two occurrences of this elsewhere here so probably not...

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few but unfortunately our TestUtils.swift doesn't really scale. It's always just available in one test module but we have lots of modules and like 5 different repos :|. So I now prefer to stick with plain XCTest whenever possible...

@weissi weissi merged commit 8912d1a into apple:master Mar 3, 2020
@weissi weissi deleted the jw-better-throw-pattern branch March 3, 2020 18:14
@weissi weissi added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Mar 3, 2020
pull bot pushed a commit to scope-demo/swift-nio that referenced this pull request Mar 3, 2020
Motivation:

The

    do {
        try someOperation()
	XCTFail("should throw") // easy to forget
    } catch error as SomethingError {
        XCTAssertEqual(.something, error as? SomethingError)
    } catch {
        XCTFail("wrong error")
    }

pattern is not only very long, it's also very error prone. If you forget
any of the XCTFails, you might not tests what it looks like

    XCTAssertThrowsError(try someOperation) { error in
        XCTAssertEqual(.something, error as? SomethingError)
    }

is much safer and shorter.

Modifcations:

Do many of the above replaces.

Result:

Cleaner, shorter, and safer tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants