Multiprotocols mocking and class+protocols combination mocking#309
Multiprotocols mocking and class+protocols combination mocking#309tarbayev wants to merge 2 commits intoerikdoe:masterfrom
Conversation
tarbayev
force-pushed
the
class-and-protocols-mock
branch
from
51227bd to
bfa3496
Compare
tarbayev
changed the title
| } | ||
|
|
||
| - (id)initWithClass:(Class)aClass; | ||
| - (id)initWithClass:(Class)aClass protocols:(NSArray *)protocols; |
There was a problem hiding this comment.
This change is way too invasive. At an absolute minimum we'd have to keep the original initWithClass: method, and make it forward to the proposed new initWithClass:protocols: method. That way, the number of changes in the PR would be greatly reduced.
| } | ||
|
|
||
| + (id)mockForProtocol:(Protocol *)aProtocol | ||
| + (id)mockForProtocols:(Protocol *)aProtocol, ... |
There was a problem hiding this comment.
See above. This change is way too invasive. At an absolute minimum we'd have to keep the original mockForProtocol: method, and make it forward to the proposed new mockForProtocols: method. That way, the number of changes in the PR would be greatly reduced.
|
|
||
|
|
||
| + (id)niceMockForClass:(Class)aClass | ||
| + (id)niceMockForClass:(Class)aClass protocols:(Protocol *)aProtocol, ... |
|
|
||
| #import <XCTest/XCTest.h> | ||
| #import <OCMock/OCMock.h> | ||
| #import "TestProtocol.h" |
There was a problem hiding this comment.
Please do not move classes that are for tests only to their own files. This just increases clutter.
|
|
||
| - (void)testCanMockMultipleProtocols | ||
| { | ||
| id mock = OCMProtocolMock(@protocol(NSLocking), @protocol(TestProtocol)); |
|
I like the general idea. And thank you for putting the effort into including unit tests. That said, the way the change is written now it is way too invasive. My main concern is that existing init methods were changed, which results in a large number of changes. I've commented on some of them. Was there a reason why you didn't keep the old methods (without protocols / with single protocol) and have them forward to the new ones (with protocol / with multiple protocols)? |
Well, it's just better to have single entry point for unit testing instead of multiple ones, the latter increase the amount of paths to be covered. Verification of forwarding methods is tricky (we cannot use OCMock partial mocking for testing OCMock itself) and I always try to avoid that. But I respect your concern, so I'll put old methods back. |
|
My main issue is compatibility. If this was application code, owned by a single team, I would also make the changes as you suggested. However, OCMock is a framework that is used by lots of different people. In that case, I believe keeping the API backwards compatible is the more important concern. |
2 participants
It's now possible to create mocks for multiple protocols and also for class and protocols combination.