Skip to content

Conversation

nononoah
Copy link
Collaborator

Summary

This PR makes allowances for throwing assertions in ActionTester.verify and RenderTester.render. These changes are meant to support tests which take advantage of XCTest's expanding API for test cases which throw.

Example of the impact of this change:

Old:

func test_saveEvents() {
    Action
        .tester(withState: state)
        .send(action: .beginSave)
        .verifyOutput { output in
            XCTAssertEqual(output.events[0], .startedSave)
        }
}

New:

func test_saveEvents() throws {
    try Action
        .tester(withState: state)
        .send(action: .beginSave)
        .verifyOutput { output in
            let first = try XCTUnwrap(output.events.first)
            XCTAssertEqual(first, .startedSave)
        }
}

Checklist

  • Unit Tests
  • N/A UI Tests
  • N/A Snapshot Tests (iOS only)
  • N/A I have made corresponding changes to the documentation

import XCTest

extension WorkflowAction {
public extension WorkflowAction {
Copy link
Collaborator Author

@nononoah nononoah Feb 17, 2021

Choose a reason for hiding this comment

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

These access control declaration updates were performed by SwiftFormat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmmm. I'd rather the access control declarations live on the functions themselves (so we don't accidentally expose something that wasn't meant to be exposed). I wonder if there's a SwiftFormat flag we can change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we haven't pinned SwiftFormat to an exact version. I guess the newer version's doing this automatically.
I'll fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed here, @nononoah can you rebase after my PR lands?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to do so!

.verifyState { XCTAssertTrue($0) }
}

func test_stateTransitions_throw() throws {
Copy link
Collaborator Author

@nononoah nononoah Feb 17, 2021

Choose a reason for hiding this comment

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

This tests and those that follow are demonstrative of the testing style these changes support, and serve to ensure we continue to support this testing style.

If others find them unnecessary, I'm happy to remove them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine. I don't think it's strictly necessary — it's more of a "ensure the code doesn't change" test than a unit test, but this will help prevent unintended breakages.

@nononoah nononoah changed the title [WorkflowTests] Allow assertions to throw [WorkflowTests] Support assertions which throw Feb 17, 2021
Copy link
Collaborator

@AquaGeek AquaGeek left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, Noah!

import XCTest

extension WorkflowAction {
public extension WorkflowAction {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmmm. I'd rather the access control declarations live on the functions themselves (so we don't accidentally expose something that wasn't meant to be exposed). I wonder if there's a SwiftFormat flag we can change.

.verifyState { XCTAssertTrue($0) }
}

func test_stateTransitions_throw() throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine. I don't think it's strictly necessary — it's more of a "ensure the code doesn't change" test than a unit test, but this will help prevent unintended breakages.

@nononoah nononoah force-pushed the noahb/rethrowing-test-functions branch from 608c8af to 142bec8 Compare February 22, 2021 16:00
@nononoah nononoah closed this Feb 22, 2021
@nononoah nononoah reopened this Feb 22, 2021
@nononoah nononoah merged commit 1a5eb0d into square:main Feb 23, 2021
@nononoah nononoah deleted the noahb/rethrowing-test-functions branch February 23, 2021 18:13
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.

3 participants