Generated mock auto-triggers passed closures

[LeonidKokhnovich]

I have a protocol like:

protocol TestType {
    func set(model: Any, for app: Any, onSuccess: (() -> Void)?, onFailure: ((Error) -> Void)?)
}

for which I try to generate a mock. As a result I get this code:

class Test: TestType {
    var invokedSet = false
    var invokedSetCount = 0
    var invokedSetParameters: (model: Any, app: Any)?
    var invokedSetParametersList = [(model: Any, app: Any)]()
    var stubbedSetOnFailureResult: (Error, Void)?
    func set(model: Any, for app: Any, onSuccess: (() -> Void)?, onFailure: ((Error) -> Void)?) {
        invokedSet = true
        invokedSetCount += 1
        invokedSetParameters = (model, app)
        invokedSetParametersList.append((model, app))
        _ = onSuccess?()
        if let result = stubbedSetOnFailureResult {
            _ = onFailure?(result.0)
        }
    }
}

I'm thinking if it would make more sense not to call the onSuccess automatically?

I found that if I change the protocol like:

protocol TestType {
    func set(model: Any, for app: Any, onSuccess: ((Void) -> Void)?, onFailure: ((Error) -> Void)?)
}

then I get a correct mock:

class Test: TestType {
    var invokedSet = false
    var invokedSetCount = 0
    var invokedSetParameters: (model: Any, app: Any)?
    var invokedSetParametersList = [(model: Any, app: Any)]()
    var stubbedSetOnSuccessResult: (Void, Void)?
    var stubbedSetOnFailureResult: (Error, Void)?
    func set(model: Any, for app: Any, onSuccess: ((Void) -> Void)?, onFailure: ((Error) -> Void)?) {
        invokedSet = true
        invokedSetCount += 1
        invokedSetParameters = (model, app)
        invokedSetParametersList.append((model, app))
        if let result = stubbedSetOnSuccessResult {
            _ = onSuccess?(result.0)
        }
        if let result = stubbedSetOnFailureResult {
            _ = onFailure?(result.0)
        }
    }
}

[seanhenry]

Hi. Thanks for raising this issue. I can definitely see why you don’t want to always call the closure now, especially with this example. How would you feel about having a Bool when a closure has no arguments?

var shouldInvokeSetOnSuccess = false

And then inside the method:

if shouldInvokeSetOnSuccess {
  onSuccess()
}

[LeonidKokhnovich]

Thanks for looking into it! The flag would definitely help.

I would try to follow the existing convention like:

...
var stubbedSetOnSuccessResult: (Void)?
...
if let result = stubbedSetOnSuccessResult {
            _ = onSuccess?()
        }

but if there is no parameter needed, that might be confusing...

[seanhenry]

I think I'm leaning towards using the Bool even though it breaks the existing convention. I think it will be easier to understand that way. I'm planning to look at this issue next :)

[LeonidKokhnovich]

@seanhenry The other option is not to call blocks automatically but instead store the block in the params list. That introduces a requirement to clean up the memory, but also might be worth considering.

[seanhenry]

We have to give the option to call the closures automatically because it needs to support non-escaping closures but I can definitely see the benefit of storing the escaping closures. And you’re right with the potential memory issues. You would likely have to set the mock to nil in the tear down which isn’t ideal. I think this might need some more thought.

[seanhenry]

I've uploaded a release which no longer calls empty closures automatically. I've also made steps towards supporting customised mock templates as we discussed earlier which will allow you create your own template to capture closures if you like. Stay tuned for that feature.

[LeonidKokhnovich]

Great, thanks a lot!