kotest / kotest

Powerful, elegant and flexible test framework for Kotlin with assertions, property testing and data driven tests.
https://kotest.io
Apache License 2.0
4.44k stars 647 forks source link

The "be" matcher failing with incorrect message for Jackson TextNode #3542

Open albertlatacz opened 1 year ago

albertlatacz commented 1 year ago

Which version of Kotest are you using 5.6.2

When I run the following test

@Test
fun `should fail with correct message`() {
    TextNode.valueOf("foo2").should(be(TextNode.valueOf("foo")))
}

I get

expected:<[]> but was:<[]>
Expected :[]
Actual   :[]

It's comparing Jackson TextNodes

ClaudenirFreitas commented 1 year ago

hi @albertlatacz , That's a very interesting scenario... TextNode is an Iterable of JsonNode (see the code) but the implementation is "a bit different" as you can see below.

    @Test
    fun `should fail with correct message`() {
        val foo2 = TextNode.valueOf("foo2")
        println("Is iterable? ${foo2 is Iterable<JsonNode>}") // Is iterable? true
        println("Size: ${foo2.size()}") // Size: 0
        println("Output: ${foo2[0]}") // Output: null
        foo2.should(be(TextNode.valueOf("foo")))
    }

When generating the message, the foo2 variable is empty. 🤔

BTW, I am still investigating...

ClaudenirFreitas commented 1 year ago

We can update the IterableEq object e.g:

   fun isValidIterable(it: Any): Boolean {
      return when (it) {
         is String -> false
         is Array<*>, is Iterable<*> -> true
         else -> false
      }
   }

   fun asIterable(it: Any): Iterable<*> {
      check(it !is String)
      return when (it) {
         is Array<*> -> it.asList()
         is Iterable<*> -> it
         else -> error("Cannot convert $it to Iterable<*>")
      }
   }

But this change is not enough, the test will pass because foo2 is empty when it should fail. I have a feeling that Kotest should not cover this scenario because the TextNode implementation is wrong (I believe).

Suggestion:

    @Test
    fun `should fail with correct message`() {
        val foo2 = TextNode.valueOf("foo2").textValue()
        val expected = TextNode.valueOf("foo").textValue()
        foo2.should(be(expected))
    }

    /*
    expected:<"foo"> but was:<"foo2">
    Expected :"foo"
    Actual   :"foo2"
    */
albertlatacz commented 1 year ago

Looking bit more turns out that when using beEqual instead of be I get correct message. How are those different? Shouldn't both matchers have same behaviour?

ClaudenirFreitas commented 1 year ago

I agree @albertlatacz. beEqual and be have different implementations. 🤔 Maybe the Core Committers can help you here... sorry

sksamuel commented 1 year ago

shouldBe allows Iterables of the same type, and TextNode implements Iterable, so its comparing the contents of the iterator - which is null for text nodes as they don't contain fields.

I am minded to say we shouldn't do that, but it has the possibility of breaking existing tests.

Kantis commented 1 year ago

@sksamuel I think we could consider breaking this with next major? There's been quite a few issues around Iterables

sksamuel commented 1 year ago

6.0 will be a while as that's going to be when compiler plugins are stable. We could break this in 5.7 with a flag to revert if a user wants.