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

Migrate to Latest OCMock, Demonstrate Improved Unit Testing #347

Merged
merged 7 commits into from
Jun 11, 2017

Conversation

Adlai-Holler
Copy link
Member

@Adlai-Holler Adlai-Holler commented Jun 10, 2017

Unit tests are great! tl;dr: This diff migrates us from OCMock 2.2 to 3.4, and migrates ASMultiplexImageNodeTests to the latest syntax as a demonstration.

  • The new version of OCMock supports a great new syntax, and prints much better error messages.
  • There's no urgency to migrate the rest of our existing test cases. I migrated the multiplex image node tests only to provide a better reference for future test development.
  • Recommend you use split diff for the multiplex image node tests file since it's changed a lot. The spirit/intention of each test method is preserved.
  • The migration of the multiplex image node tests revealed a minor bug.
  • New syntax is much more readable:
    • Old: [[[mockCache expect] andDo:^(NSInvocation *inv) { … }] imageForImageID:OCMOCK_ANY];
    • New: OCMExpect([mockCache imageForImageID:OCMOCK_ANY]).andDo(^(NSInvocation *inv) { … });
  • NSInvocation+OCMAdditions file is no longer public, so I created a mini version of those methods NSInvocation+ASTestHelpers that is actually safer under ARC.
  • New pattern established in multiplex image node tests is much tighter and cleaner:
    • -tearDown includes OCMVerifyAll of all mocks created in -setUp.
      • Test methods just have to OCMExpect and run their code, validation is done for them.
    • -setUp sets expectationOrderMatters on all mocks. Test methods can clear this flag if they need to, but they shouldn't.
    • Stubbing is used very sparingly (one place), since it doesn't make any assertion about when the method is called. Use OCMExpect instead, unless you're really sure the method sequence is irrelevant.
    • Test case ivars do not have underscores: _mockCache -> mockCache.
      • Breaks from tradition but is readable and reduces boilerplate inside test methods.

Note: The indentation style for chained macros may seem strange to you (I quite like it), but it's the Xcode default:

OCMExpect([mockDataSource multiplexImageNode:imageNode imageForImageIdentifier:highResIdentifier])
.andReturn([self _testImage]);

}

[cv layoutIfNeeded];
XCTAssertEqualObjects(attr, [cv layoutAttributesForSupplementaryElementOfKind:@"SuppKind" atIndexPath:indexPath]);
XCTAssertEqual(view, [cv supplementaryViewForElementKind:@"SuppKind" atIndexPath:indexPath]);
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, under the new OCMock, if an exception is thrown while inside a mock object method, and you later verify, it will fail and report that an exception was thrown. So we will have to avoid verifying after testing exception-throwing, which is sucky but not worth holding back on OCMock 2.2.

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

Nice - And the I really like the new syntax!

@Adlai-Holler Adlai-Holler merged commit 69e6987 into master Jun 11, 2017
@Adlai-Holler Adlai-Holler deleted the AHLatestOCMock branch June 11, 2017 16:41
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…roup#347)

* Update OCMock 2.2 -> 3.4

* Clean up and port ASMultiplexImageNodeTests

* Clean up

* Be stricter about order

* Log change

* Update the licenses #important

* Update the license headers more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants