Message from Assertk assertions is not printed when using withArg block.

[CamilYed]

Prerequisites

Please answer the following questions for yourself before submitting an issue.

• 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

The message from AssertionFailedError (which is thrown by Assertk lib) should be printed when assertion is performed in withArg block.

Current Behavior

The message from AssertionFailedError (which is thrown by Assertk lib) exception is not printed when the checking is performed in withArg block.

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.11.0
• OS: macOS Big Sur
• Kotlin version: 1.5
• JDK version: 11
• JUnit version: 5
• Type of test: unit test

Minimal reproducible code (the gist of this issue)

// -----------------------[ GRADLE DEFINITIONS ] -----------------------

project.ext.versions = [
  junit                : '5.7.2',
  assertk           : '0.24',
  mockk            : '1.11.0'
]

dependencies {
  testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-api', version: versions.junit
  testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-engine', version: versions.junit
  testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-params', version: versions.junit
  testImplementation group: 'com.willowtreeapps.assertk', name: 'assertk-jvm', version: versions.assertk
  testImplementation group: 'io.mockk', name: 'mockk', version: versions.mockk
}
// -----------------------[ YOUR CODE STARTS HERE ] -----------------------
package some.example.com

import io.mockk.every
import io.mockk.mockk
import org.apache.kafka.common.KafkaFuture
import kotlin.test.Test

class SomeExampleTest {

    val orderRepository: OrderRepository = mockk()
    val orderService: OrderService = OrderService(orderRepository)

    @Test
    fun test() {
       // given
       thereIsOrder(anOrder())

      // when
      orderService.pay(anOrderPayment())

      // then
      wasSaved(anOrder())
    }
    
    fun wasSaved(order: Order, times: Int = 1) {
      verify(exactly = times) {
         orderRepository.save(withArg{ equalsTo(order) })
      }
    }
    
    private fun MockKAssertScope.equalsTo(order: Order) {
            assertThat(this.actual as Order)
           .hasCommandId(order.commandId)
           .hasOrderId(order.orderId)
           // and so on     
  }

    // walk around
    private fun MockKAssertScope.equalsToAndLog(order: Order) {
       try {
            assertThat(this.actual as Order)
           .hasCommandId(order.commandId)
           .hasOrderId(order.orderId)
           // and so on
       } catch (t: Throwable) {
           logger.error(t) {}
           throw t
       }
  }
}
// -----------------------[ YOUR CODE ENDS HERE ] -----------------------

I found that exception is catched here, and the stacktrace is not logged.

package io.mockk

data class FunctionMatcher<in T : Any>(
    val matchingFunc: (T) -> Boolean,
    override val argumentType: KClass<*>
) : Matcher<T>, TypedMatcher, EquivalentMatcher {
    override fun equivalent(): Matcher<Any> = ConstantMatcher(true)

    override fun match(arg: T?): Boolean {
        return if(arg == null) {
            false
        } else {
            try {
                matchingFunc(arg)
            } catch (a: AssertionError) {
                false
            }
        }
    }

    override fun toString(): String = "matcher<${argumentType.simpleName}>()"
}

[romrell4]

Any updates on this? It's a REALLY nice feature, but without logging the exception (in my case from Google Truth), it makes it very un-maintainable when the matcher fails

[romrell4]

I'll get a PR up for this. I think it's pretty easy (just a log statement inside the catch of the matcher)

    override fun match(arg: T?): Boolean {
        return if(arg == null) {
            false
        } else {
            try {
                matchingFunc(arg)
            } catch (a: AssertionError) {
// TODO: Log the error here
                false
            }
        }
    }

[Raibaz]

Yep that seems like the right place to log the error message, thanks for looking into it!

[romrell4]

@Raibaz (assuming you’re a maintainer) How do you guys do logging? I can obviously add a println, but I’m guessing you have some sort of standard? Do I need to inject a logged somehow?

[romrell4]

Oof. Looks like I can't even run the project (I'm on a work machine and they have VPN constraints that don't allow me to download project dependencies. I'll put up a PR with the println - but I'm guessing someone will want to go in and change it to some sort of logger before merging

[romrell4]

I believe I gave access for maintainers to push directly to my fork. Feel free to have at it if you know how to do the logging @Raibaz :)

[Raibaz]

First of all, thanks a lot for looking into this.

On second thought, though, I see an issue with your test implementation: argument matchers are meant just for matching arguments, not to perform any logic, or assertions, on the arguments themselves.

What you are actually doing in your test is definitely better achieved by capturing the argument and then performing assertions on it, something like:

val slot = slot<Order>()
verify(exactly = times) {
     orderRepository.save(capture(slot))
}

assertThat(slot.captured)
.hasCommandId(order.commandId)
.hasOrderId(order.orderId)
// ...

Note that by using a slot, you can also log the argument values if needed, or perform any other sorts of logic onto it.

A matcher is definitely not the right place to perform assertions; perhaps we should document this better.

Also, if you check issue #389, which was the reason why the catch block was added, I was mentioning the same thing about capturing the argument being a better way of achieving the same result :)

Thinking again about it, I don't think catching AssertionError and failing silently is the best choice here; we should probably warn users that they should not be performing assertions inside matchers.

[romrell4]

Ah bummer. I get your point, but it sure was nice to be able to run assertions as part of the verification... (e.g. verify that the function was called with an argument that matches these assertions, rather than verify the method was called with anything, followed by assertions).

So is your only concern the fact that this is muddying the waters for the matcher? Is there any way to rearchitect this so that it only applies to the withArg function? It seems that there are a number of articles out there recommending for people to use withArg to run assertions (not that we’re supposed to cater to them, but to show that users do like using the withArg to avoid the slot approach):
https://notwoods.github.io/mockk-guidebook/docs/matching/with/
https://sonique6784.medium.com/pure-kotlin-unit-testing-mocking-part-2-e13857014ea6

[Raibaz]

It's not just a matter of muddying waters, it's really that matchers are meant for matching parameters, not to execute code, and thus they live in a module where there is neither logging enabled or actually any other dependencies.

Adding a logging dependency just for this case seems really overkill to me, given that there is a better, clearer way to achieve the same behavior.

[romrell4]

@Raibaz so then what's supposed to be the difference between match and withArg? It seems like the mockk community has been using withArg to run assertion code (e.g. it passes unless an exception is thrown), while match is being used to return a boolean value to determine if the verification passes. Was that not the original intention?

Don't get me wrong, I completely understand the hesitation to add a logger if it's not needed - but from a consumer point of view, I think that being able to run assertions as part of the verification is way more developer-friendly than having to capture a slot and run separate assertions

val slot = slot<MyType>()
val slot2 = slot<AnotherType>()
verify {
  someDependency.someFunction(
    arg1 = capture(slot), 
    arg2 = capture(slot2)
  )
}
assertThat(slot.captured.someValue).isEqualTo("someValue")
assertThat(slot.captured.someOtherValue).isEqualTo("someOtherValue")
assertThat(slot2.captured.anotherValue).isEqualTo("anotherValue")

vs

verify {
  someDependency.someFunction(
    arg1 = withArg {
      assertThat(someValue).isEqualTo("someValue")
      assertThat(someOtherValue).isEqualTo("someOtherValue")
    }, 
    arg2 = withArg {
      assertThat(anotherValue).isEqualTo("anotherValue")
    }
  )
}

You can see how the the withArg function allows for better scoping, removes the need for creating a ton of slot variables in your test just so that you can run an assertion or two on them, and seems to make better sense from the verification perspective as well (e.g. when you use a slot, it means that the verify always passes, whereas if you use withArg, you're able to tell the verification to only pass if it was called with the proper values)

Just food for thought. It just seems like a major win from a library perspective that would be sad to lose simply because a logger wasn't injected into the right place

[Raibaz]

I totally agree with your point about withArg being more developer-friendly and concise than slots.

This being said, I looked back at #389, which was the issue that made me introduce the specific catch for the AssertionError, and I am wondering: in that case, the same function is being verified twice with different arguments.

This means that the assertion logic is being used to drive the matching logic too: if all assertions on the arguments pass, then the argument is matching, otherwise it's not, and that is the way MockK matches the two different calls and passes the test.

The case when a user wants to verify two different calls to the same method in the same verify block, thus using the assertions to match the arguments, seems to me a bit less common than the general use case for withArg, which is to perform assertions on an argument, so I'm thinking it may be better to revert the change I made to withArg and have AssertionErrors bubble up (and be logged) rather than being swallowed silently and making the matcher fail.

I don't have any usage details on this, but it seems to me I fixed a less-common use case and broke a more common one :(

What do you think?

[Raibaz]

(also, thanks for taking the time to investigate this and for the suggestions!)

[romrell4]

Of course :) Thanks for listening.

Yeah, if we remove the try/catch and the errors bubble up, that definitely solves the use case that this issue is specifically addressing. If you feel comfortable with that fix, I'll modify my PR to do that instead

#776

Once again, I can't run the test suite locally, but feel free to push anything to my branch if you know of any other changes that would need to be made to support this

However, I'm not familiar with the original issue - so if you think we still need to rethink things, let me know

[Raibaz]

Yep your PR was perfectly fine, thanks a lot!

This issue can be closed then.

[romrell4]

Thanks! So how do release cycles work? Is every PR merged to master a new version of mockk? If so, where would I find the new build version? Or do you cut releases on a cadence?

[Raibaz]

I usually do releases manually once there are enough new changes.

I already did some changes to add support for Kotlin 1.6.* and remove support for 1.3.*, so I may actually make a release pretty soon.

[Raibaz]

Meanwhile, you can obviously build your own snapshot from master :)

[romrell4]

We may need to revert this change. One unintended consequence is that in cases when multiple calls are made to the same mock, this will fail when matching the first one...

Test that succeeded before 12.3.1:

  @Test
  fun test() {
      class Tester {
          fun run(str: String) {
              println(str)
          }
      }

      val mockTester = mockk<Tester>(relaxed = true)
      mockTester.run("1")
      mockTester.run("2")

      verify {
          mockTester.run(withArg {
              assertThat(it).isEqualTo("2")
          })
      }
  }

now fails...

[romrell4]

#792 Here's a PR to revert, if you choose to. We might also want to revert the subsequent commit that you added to remove the failing test that solved #389

[romrell4]

I didn't realize that the Matcher was being run for each call, and having an exception thrown would fail it immediately. I thought that the exception would get bubbled up, and that the matcher would move on to the next one - only printing the exception if the full matcher failed. I probably should have looked at #389 a little closer before submitting the PR :(

[romrell4]

So @Raibaz we're kinda back at square one... 😂 The original "Add a logger" solution would be slightly better, but would give false positives (it would log failures, even if it will eventually find a call that succeeds).

The best solution that I can imagine (from a naive perspective) is kinda what you're already doing with verify (where it shows all the calls that were made, and what didn't match), but ALSO print the assertion error at that point for each call. So that way, the assertion error is only printed when the overall verification fails, but it still allows it to check through all calls. So it would look kinda like this:

Verification failed: call 1 of 1: Tester(#11).run(matcher<String>())). No matching calls found.

Calls to same method:
1) Tester(#11).run(1)
Nested Error Details:
expected: 3
but was : 1
	at com.walmart.glass.amends.edit_items.ui.EditItemsViewModelTest.test(EditItemsViewModelTest.kt:354)
2) Tester(#11).run(2)
Nested Error Details:
expected: 3
but was : 2
	at com.walmart.glass.amends.edit_items.ui.EditItemsViewModelTest.test(EditItemsViewModelTest.kt:354)

Thoughts? I'm not sure how difficult that would be to implement... Essentially, capturing the assertion errors and adding them to the resulting failure

[weilbith]

Is there any update on this? Or is the only way to make it work to use a capture? Having no messages is especially cumbersome when using recursive AssertJ statements inside a withArg block.

[twadzins]

One workaround with slot() for common situations that is a little more concise and also scoped is the following:

// then
slot<String>().apply {
    verify { something.someFunction(any(), capture(this@apply)) }
    captured shouldBe "xyz" // kotest assertion here but could be whatever assertion lib you want
}