single() assertion causes test that should fail to pass instead (example inside)

[MartinHaeusler]

I've witnessed a very strange behavior of strikt 0.29.0 (as well as 0.30.0) today. Please consider the following self-contained test:

import org.junit.jupiter.api.Test
import strikt.api.expectThat
import strikt.assertions.hasSize
import strikt.assertions.single

class StriktTest {

    @Test
    fun thisWillFailAsIntended(){
        val list = listOf("hello", "foo")

        expectThat(list).hasSize(3).and { // fails the "hasSize" condition, as it should.
            get { this.filter { it == "foo" } }.single()
        }
    }

    @Test
    fun thisWillActuallyPass() {
        val list = listOf("hello", "foo")

        expectThat(list) {
            hasSize(3) // we have 2 items in the list, yet the test passes
            get { this.filter { it == "foo" } }.single()
        }
    }

    @Test
    fun thisWillActuallyPassToo() {
        val list = listOf("hello", "foo")

        expectThat(list) {
            get { this.filter { it == "foo" } }.single()
            get { this.filter { it == "bar" } }.single() // "bar" is not even in the list - yet the test passes!
        }
    }

}

To my amazement, the second and third test will actually pass. Even though the list certainly doesn't match all of the given criteria.

I can't explain why that's the case. Did I do something horribly wrong with the API? Did I mis-interpret some of the methods?

[MartinHaeusler]

Upon further investigation: the single() assertion seems to do something odd. Consider this modified version:

    @Test
    fun thisWillActuallyPass() {
        val list = listOf("hello", "foo")

        expectThat(list) {
            hasSize(3)
            get { this.filter { it == "foo" } }.hasSize(1)
        }
    }

... which uses hasSize(1) instead. This test fails, as it should:

strikt.internal.opentest4j.CompoundAssertionFailure: ▼ Expect that ["hello", "foo"]:
  ✗ has size 3
       found 2
  ▼ this.filter { it == "foo" }:
    ✓ has size 1

So single() seems to be the culprit.

[MartinHaeusler]

Unfortunately, the same holds true if we use .hasSize(1).get(0):

    @Test
    fun thisWillActuallyPassToo() {
        val list = listOf("hello", "foo")

        expectThat(list) {
            get { this.filter { it == "foo" } }.hasSize(1)[0]
            get { this.filter { it == "bar" } }.hasSize(1)[0] // "bar" is not even in the list - yet the test passes!
        }
    }

This passes :(

[MartinHaeusler]

@robfletcher any chance you could have a look at this? This one is really critical for my team...

[MartinHaeusler]

I'm doing some investigations on this, using the code on your current main branch.

The following test case:

  @Test
  fun thisWillActuallyPass() {
    val list = listOf("hello", "foo")

    expectThat(list) {
      hasSize(3) // we have 2 items in the list, yet the test passes
      get { this.filter { it == "foo" } }.single()
    }
  }

... produces the following output when I log the "status" of the tree and the two chains during evaluate(context):

Assertion Node strikt.internal.AssertionChain@4a807a83 reports status: Failed(description=null, comparison=null, cause=null) 
Assertion Node strikt.internal.AssertionChain@3ae3cfdc reports status: strikt.api.Status$Pending@529b9921 
TREE STATUS: strikt.api.Status$Pending@529b9921

I think the key observation here is that single() doesn't count as a full assertion; it is treated as "pending". Regardless of whether or not that is true, the consequence is that the status getter combines the child states as follows:

  override val status: Status
    get() = when {
      children.isEmpty() -> Pending
      children.any { it.status is Pending } -> Pending
      children.any { it.status is Failed } -> Failed()
      else -> Passed()
    }

Note that "pending" is checked first. And we do have one pending child. However, one child has also already failed. Yet, here we return Status.Pending. Aaaand on top level in the AssertionStrategy, we have:

 if (tree.status is Failed) {
        throw CompoundAssertionFailure( ... )

Well, the status isn't "failed", it's "pending". Unfortunately this lets our test pass.

This raises several questions:

• Is single() a full-fletched standalone assertion? I would say so, because if there is no follow-up, it's a shorthand for hasSize(1).
• If the status of the assertion tree is pending at the time of evaluation, doesn't that mean that the assertions are misconfigured, and we should throw a MisconfiguredAssertionException instead of simply letting the test pass?
• Shouldn't we check for failure first and then for "pending" in the state propagation?

[robfletcher]

single() is not really intended as a standalone assertion, no. It's a mapping / traversal function that has a side-effect of halting the assertion chain if the collection has the wrong size. However, this still seems like a bug.

The intention is that it's something you'd use to get an Assertion.Builder for the single element from a size 1 collection in order to then make further assertions on that. As such it should fail when the collection has any other size. I strongly suspect this is to do with the fact you're using it inside of an expectation block.

I'll see if I can free up some time to take a look at this ASAP.

[robfletcher]

Thanks for the analysis. This was pretty straightforward to fix. v0.30.1 is releasing now.

[MartinHaeusler]

@robfletcher thanks for looking into it. The way I read your commit is that fail on any child now simply takes precedence over any pending child, correct? I totally agree with this change, in fact I was wondering why it was handled the other way around before.

That leaves one weird case: assume that you only have one child, and the child is pending. Then the whole assertion tree is pending, but the expectThat(...) method will not complain about pending assertions (i.e. it won't throw an exception), right? expectThat(...) only checks for failure. Is there any reason not to throw an exception (e.g. MisconfiguredAssertionException) in this case?

[robfletcher]

Yep, I certainly need to put some more thought into this. First wanted to get a quick fix out.

[MartinHaeusler]

@robfletcher thank you for taking care of this! I think that throwing an exception when the assertion status ends up as Pending during the evaluation is the best course of action.