Use more appropriate asserts #259

cerihughes · 2016-12-22T14:31:01Z

Always use XCTAssertEqualObjects when comparing objects for equality. XCTAssertEqual was causing some issues with NSIndexPath equality on 32 bit architectures (possibly because 64-bit architectures pool instances that are equal?)

spotify-ci-bot · 2016-12-22T14:53:14Z

	3 Messages
📖	Executed 339 tests, with 0 failures (0 unexpected) in 7.275 (8.472) seconds
📖	Executed 336 tests, with 0 failures (0 unexpected) in 7.320 (8.682) seconds
📖	Executed 9 tests, with 0 failures (0 unexpected) in 147.943 (147.952) seconds

Generated by 🚫 danger

codecov-io · 2016-12-22T14:54:00Z

Current coverage is 93.43% (diff: 100%)

Merging #259 into master will increase coverage by 0.07%

@@             master       #259   diff @@
==========================================
  Files            72         72          
  Lines          5015       5015          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4682       4686     +4   
+ Misses          333        329     -4   
  Partials          0          0

Powered by Codecov. Last update b9852cb...bb26062

cerihughes · 2016-12-22T14:56:00Z

@spotify/objc-dev

JohnSundell

Most of the time we actually want to make sure that the same instance of an object is used, not just that they're equal. So I'd update only the index path cases that were failing (and should use XCTAssertEqualObjects).

JohnSundell · 2016-12-22T14:55:45Z

tests/HUBActionRegistryTests.m

@@ -58,7 +58,7 @@ - (void)testRegisteringFactoryAndCreatingAction
 HUBActionFactoryMock * const factory = [[HUBActionFactoryMock alloc] initWithActions:@{actionIdentifier.namePart: action}];
 [self.actionRegistry registerActionFactory:factory forNamespace:actionIdentifier.namespacePart];

- XCTAssertEqual([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action);
+ XCTAssertEqualObjects([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action);


Here we actually want to make sure that the objects are the same instance so XCTAssertEqual should be used.

I wonder if we should use XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action) for that then, just to be explicit about it.

Hamcrest has a "same instance" matcher that is good for these scenarios. XCTest doesn't seem to though :(

Isn't it more explicit to use XCTAssertEqual that does exactly that? 😅

Yeah, possibly. I have a problem with XCTAssertEqual as it's often misused, and people often aren't asserting what they think. If you do want to test instance equality, then using XCTAssertEqual just feels wrong as actually tests "more" than equality.

If we are testing instance equality XCTAssertTrue(a == b) is far more explicit. XCTAssertEqual is just too vague and has different behaviour depending on what you pass it, and that irks me :)

JohnSundell · 2016-12-22T15:21:23Z

tests/HUBActionRegistryTests.m

- XCTAssertEqual([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action);
+
+ // These should be the same instance.
+ XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action);


I really fail to understand why this is better than XCTAssertEqual which is meant for exactly these type of comparisons.

I agree with Ceri; XCTAssertEqual is a smell for non-scalar types.

I guess it's because in a test, if I look at a line that says XCTAssertEqual(objA, obj2), I immediately think "do we really want to check equality here, or is this a typo".

Admittedly XCTAssertTrue(obj1 == obj2) doesn't do a great deal to fix that ambiguity, which is why I like Hamcrest's approach of providing a "same instance" matcher for objects so that it's obvious by looking at it whether you want to check for equality or instance equivalence.

I think the comment is the most useful thing here. I'll revert the assert and keep the comment.

How about adding a description to the assert? That is:

XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action, @"The action should be the same instance as the factory mock vends");

That way the comment will be directly attached to the assert. Making sure it also shows up when the test fails.

cerihughes · 2016-12-22T15:50:04Z

@JohnSundell : I would assume that by default we'd want to be checking object equality in tests, unless we have a requirement that something returns the same instance under certain conditions. Only then would we want to test the actual instances.

The NSIndexPath case is a good example - on some platforms (for some reason) calls to factory methods will return different instances, and on other platforms they return the same instance (I haven't verified this, but it's one explanation for what's happening). Ultimately though, it shouldn't matter, as long as we use object equality, not instance equality as isEqualTo: will take care of all the differences for us.

Only fixing the cases with NSIndexPath because we know there are problems with that right now seems a bit broken, as the same issue could arise with other classes in future OS releases (as an example).

JensAyton · 2016-12-22T15:51:51Z

@cerihughes NSIndexPaths of two elements are tagged pointers on 64-bit systems. Same sort of fun happens with short strings.

rastersize · 2017-01-04T20:54:39Z

tests/HUBViewModelRendererTests.m

@@ -108,8 +108,8 @@ - (void)testTwoSubsequentRenders
 [self waitForExpectationsWithTimeout:2 handler:nil];

 XCTAssertEqual([self.collectionViewLayout numberOfInvocations], 2u);
- XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel);
- XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel);
+ XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel);


Shouldn’t we use XCTAssertTrue([self.collectionViewLayout capturedViewModelAtIndex:0] == firstViewModel) here as well? As well as adding an assert description stating that we really do want to check for instance equality.

Just like at https://github.com/spotify/HubFramework/pull/259/files#diff-51a93e5f57aabdf96df5027e93c75d58R62

rastersize · 2017-01-04T20:54:53Z

tests/HUBViewModelRendererTests.m

- XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel);
- XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel);
+ XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel);
+ XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel);


Same as https://github.com/spotify/HubFramework/pull/259/files#r94663198

rastersize · 2017-01-04T20:59:34Z

@cerihughes I’m in favor of the changes made in this PR. Unless we explicitly want to check that two objects are the same instance we should use XCTAssertEqualObjects(obj1, obj2, desc, …).

Further I think we should, like you’re doing here, use XCTAssertTrue() with a description (or comment) when we actually want to check for instance equality. I’d also be fine with adding a function or macro like HUBAssertEqualInstances(ins1, ins2, desc, …) to make the code more self-explanatory.

Use more appropriate asserts

bb26062

JohnSundell suggested changes Dec 22, 2016

View reviewed changes

Check for the same instance.

fd6d6ed

JohnSundell suggested changes Dec 22, 2016

View reviewed changes

cerihughes mentioned this pull request Jan 3, 2017

Don’t use XCTAssertEqual for NSIndexPath equivalence. #268

Merged

rastersize reviewed Jan 4, 2017

View reviewed changes

rastersize added bug fix enhancement labels Jan 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more appropriate asserts #259

Use more appropriate asserts #259

cerihughes commented Dec 22, 2016

spotify-ci-bot commented Dec 22, 2016 •

edited

codecov-io commented Dec 22, 2016 •

edited

cerihughes commented Dec 22, 2016

JohnSundell left a comment

JohnSundell Dec 22, 2016

cerihughes Dec 22, 2016

cerihughes Dec 22, 2016

JohnSundell Dec 22, 2016

cerihughes Dec 22, 2016

cerihughes Dec 22, 2016 •

edited

JohnSundell Dec 22, 2016

JensAyton Dec 22, 2016

cerihughes Dec 22, 2016

cerihughes Dec 22, 2016

rastersize Jan 4, 2017 •

edited

cerihughes commented Dec 22, 2016

JensAyton commented Dec 22, 2016

rastersize Jan 4, 2017

rastersize Jan 4, 2017

rastersize commented Jan 4, 2017

Use more appropriate asserts #259

Are you sure you want to change the base?

Use more appropriate asserts #259

Conversation

cerihughes commented Dec 22, 2016

spotify-ci-bot commented Dec 22, 2016 • edited

codecov-io commented Dec 22, 2016 • edited

Current coverage is 93.43% (diff: 100%)

cerihughes commented Dec 22, 2016

JohnSundell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerihughes Dec 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rastersize Jan 4, 2017 • edited

Choose a reason for hiding this comment

cerihughes commented Dec 22, 2016

JensAyton commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rastersize commented Jan 4, 2017

spotify-ci-bot commented Dec 22, 2016 •

edited

codecov-io commented Dec 22, 2016 •

edited

cerihughes Dec 22, 2016 •

edited

rastersize Jan 4, 2017 •

edited