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

In 1.12.3 verify { mock.method( withArg {}) } fails if second call matches. Works in 1.12.2 #796

Closed
3 tasks done
piotrplazienski opened this issue Mar 10, 2022 · 8 comments

Comments

@piotrplazienski
Copy link

piotrplazienski commented Mar 10, 2022

Prerequisites

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Expected Behavior

When calling verification using withArg{}, if there are multiple calls to mock method, and one of calls matches withArg, the verification should pass.

Current Behavior

If there are multiple calls to method verification will fail. It passed in 1.12.2

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

  • MockK version: 1.12.3
  • OS: Win/Linux
  • Kotlin version: 1.6.10
  • JDK version: 17-azul
  • JUnit version: 5.8.2
  • Unit Test

Minimal reproducible code (the gist of this issue)

@Test
fun `mockk can assert one of calls`() {
    open class Mocked {
        fun called(i: Int) {}
    }
    val mock = mockk<Mocked>()
    every { mock.called(any()) } just Runs
    mock.called(1)
    mock.called(3)
    verify {
        mock.called(withArg { assert(it == 3) })
    }
}

I would guess this is caused by #776, as #394 fixed #389 that seemed similar, and #776 reverted changes.

If this is intentional change, please provide intended way of asserting with many calls.

@Raibaz
Copy link
Collaborator

Raibaz commented Mar 10, 2022

I don't really understand why you are using withArg in this case.

Isn't it the same as saying

verify { mock.called(3) } 

?

And yes, by using simpler matchers you can match multiple calls of the same method.

@piotrplazienski
Copy link
Author

Well, this is simplest example that reproduces the issue, I prepared it specifically for this submission. In real code I have much more complex condition :D.

And as for simpler matchers - in 1.12.2 this worked fine, and if this change is intentional please confirm it. But I doubt it, as withArgs intent seems to be just another matcher, so it should behave like one. If I'm wrong please correct me.

@AnninaO
Copy link

AnninaO commented Mar 11, 2022

Facing the same problem here. Tests which contain "withArg" running fine on 1.12.2 and fail on 1.12.3

@Raibaz
Copy link
Collaborator

Raibaz commented Mar 13, 2022

@piotrplazienski as you guessed, this behavior was introduced with #776.

If you read the discussion in #707, IMHO it is not correct to use withArg to perform assertions on parameter values, because that is exactly what slots are for.

I understand that the usage of assertions inside withArg blocks is pretty widespread, so I don't really have a strong opinion on this (at least, I do, but it seems like there's a widespread usage that goes in another direction :)).

Perhaps I will just add better documentation to discourage this improper usage of withArg and try directing users towards using the right matcher for the job they need, or slots for complex argument verification.

@piotrplazienski
Copy link
Author

piotrplazienski commented Mar 14, 2022

Thank you for explanation. I think I get the intention now.

I also don't have any strong opinion on this, but I also understand why withArgs with assertions was so widespread :). Using slots for very simple assertions is much more cumbersome than writing this assert in withArgs - define slot, capture, then assert on slot instead of passing the lambda. But I also get why catching this assertion is problematic, and cannot be really solved without breaking intention behind matchers.

So to conclude: I think this should be closed now, but I would appreciate some docs on withArgs on this - since this is quite widespread. But if there is an other option to have shortcut for asserting on captured arg using lambda it would be appreciated a lot :D.

@Raibaz
Copy link
Collaborator

Raibaz commented Mar 14, 2022

As a side note, most of the code that I have seen doing

verify {
    mock.method(withArg {
        assertEquals("someValue", it.property)
    })
}

Can also be written as

verify {
    mock.method(match {
        it.property == "someValue"
    })
}

Which is, IMHO, pretty concise for a few properties to be checked.

In case someone needs to verify more than a few properties, I'd probably still go for a slot.

@piotrplazienski
Copy link
Author

Yes, I think using predicate is better practice and that is where I will go with rework of my tests.

I will close the issue now.

@jlussagnet
Copy link

jlussagnet commented Jan 15, 2024

Perhaps I will just add better documentation to discourage this improper usage of withArg and try directing users towards
using the right matcher for the job they need, or slots for complex argument verification.

Hi @Raibaz, IMO it would greatly help to mention this in the documentation of withArg, as it took me quite some time to investigate the issue in some of my tests, and more time again to finally find your explanation hidden here.
For what I get from the current doc it is not very precise about any encouraged or discourage practise:
withArg { code } | matches any value and allows to execute some code

Although I don't see any example about the recommended usage. I guess it would no show any usage of assertion in the code block and that could also give a hint.
Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants