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

Support calling objective-c methods and closures with nil #374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dekpient
Copy link

@dekpient dekpient commented Dec 8, 2020

Hello, I'm trying to fix 2 issues I found when trying to use Cuckoo/OCMock in a project. I'm currently unable to cover a few code branches because of them.

  1. Calling mock.say(nil) currently fails with -[_NSArrayM insertObject:atIndex:]: object cannot be nil (NSInvalidArgumentException) because NSInvocation+OCMockWrapper doesn't do a null check.
  2. Using objectiveArgumentClosure and calling the closure with nil fails with -[NSNull count]: unrecognized selector sent to instance 0x7fff8002ebb0 (NSInvalidArgumentException). I think it's cause TrustMe doesn't support nullable types. I propose objectiveOptionalArgumentClosure (or maybe objcArgumentClosureWithOptional ?). I can add more of them to support closures with more args if you think this is alright.

I also removed the reference to ObjectiveExamplesTest.swift. It doesn't exist in the project so it's making the build fail.

Cheers

@dekpient dekpient changed the title Support calling methods and closures with nil Support calling objective-c methods and closures with nil Dec 8, 2020
@MatyasKriz
Copy link
Collaborator

Hey, @dekpient. Thanks for the PR and thanks for using the OCMock integration!

It's a nice find with the nil, I'll take a look at the code and see what we can do with the second problem you're proposing a solution to.

@MatyasKriz MatyasKriz added the bug label Jan 11, 2021
@@ -31,6 +31,19 @@ public func objectiveArgumentClosure<IN1, IN2>(from: Any) -> (IN1, IN2) -> Void
}
}

// TODO add more variants
public func objectiveOptionalArgumentClosure<IN1, IN2>(from: Any) -> (IN1?, IN2?) -> Void {
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 we'll have to try coming up with a more complex solution to this, because as the number of arguments go up, the combinations of optional and non-optional arguments will skyrocket.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I agree. I ended up defining this in my test project just for the specific test cases that I need. Should I split this into 2 PRs so the 1st problem can be fixed? I really need that fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Splitting this up sounds like the better call at the moment. Would you please add the remaining combinations for 2-parameter closures, i.e. (optional, non-optional), (non-optional, optional) as well as an optional counterpart to the 1-parameter closure. We'll need to come up with a more generic solution, but for now this can help make the closures more accessible by showing users how to implement them (only using the right amount of arguments). Thanks!

Also, would you mind creating a test or two for the closures with optional parameters? Just to make sure they work and when replacing with the generic solution to know nothing has been broken.

Afterwards, we can probably merge this and release.

}

mock.say(nil) { result, messages in
XCTAssertEqual("nil now behaves", result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good nil.

@MatyasKriz MatyasKriz force-pushed the master branch 2 times, most recently from d2d44b9 to a4618f6 Compare April 22, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants