dmcg / minutest

Simple, Expressive, Extensible Testing for Kotlin on the JVM
Apache License 2.0
102 stars 11 forks source link

Experimental SKIP feature unnecessary context evaluation #33

Open mkobit opened 5 years ago

mkobit commented 5 years ago

Version: 1.7.0

I was trying out the dev.minutest.experimental.SKIP feature and noticed that it looks like deriveFixture/modifyFixture/probably other pieces are executed for a nested context that is skipped. I was using Mockito mocks in a fixture and was modifying the behavior in each nested context. At the most nested context, I had a handful of tests that I wanted to skip because they were not implemented yet, so I thought I would try out SKIP. I saw test failures due to using strict mocks, so the expectations set on the mocks were throwing org.mockito.exceptions.misusing.UnnecessaryStubbingException after the test because it looks like the behavior settings were called even though the nested context was skipped.

My expectation is that the whole buildup would not be executed for skipped contexts.

Here is an example that I think demonstrates my expectations better than my explanation:

import dev.minutest.experimental.SKIP
import dev.minutest.experimental.minus
import dev.minutest.junit.JUnit5Minutests
import dev.minutest.rootContext

internal class Mk : JUnit5Minutests {
    fun tests() = rootContext<Int> {
        fixture { 0 }
        context("l1") {
            deriveFixture {
                println("deriveFixture: ${it.name}")
                this + 1
            }
            context("l2") {
                test("l2 test") {
                    println(it.name)
                }
            }

            SKIP - context("l2 skipped") {
                test("l2 skipped test") {
                    println(it.name)
                }
            }
        }
    }
}

Output:

deriveFixture: l2 test
l2 test
deriveFixture: l2 skipped

Test ignored.

dev.minutest.experimental.MinutestSkippedException
    at dev.minutest.experimental.InexcludingKt$skipped$1.invoke(inexcluding.kt:47)
    at dev.minutest.experimental.InexcludingKt$skipped$1.invoke(inexcluding.kt)
    at dev.minutest.Test.invoke(Node.kt)
    at dev.minutest.Test.invoke(Node.kt:37)
    at dev.minutest.internal.PreparedContext$runTest$1.invoke(PreparedContext.kt:21)
    at dev.minutest.internal.PreparedContextKt.tryMap(PreparedContext.kt:61)
    at dev.minutest.internal.PreparedContextKt.access$tryMap(PreparedContext.kt:1)
    at dev.minutest.internal.PreparedContext.runTest(PreparedContext.kt:21)
    at dev.minutest.internal.TestExecutor$andThen$1$runTest$testletForParent$1.invoke(TestExecutor.kt:25)
    at dev.minutest.internal.TestExecutor$andThen$1$runTest$testletForParent$1.invoke(TestExecutor.kt:17)
    at dev.minutest.internal.PreparedContext$runTest$1.invoke(PreparedContext.kt:21)
    at dev.minutest.internal.PreparedContextKt.tryMap(PreparedContext.kt:61)
    at dev.minutest.internal.PreparedContextKt.access$tryMap(PreparedContext.kt:1)
    at dev.minutest.internal.PreparedContext.runTest(PreparedContext.kt:21)
    at dev.minutest.internal.TestExecutor$andThen$1$runTest$testletForParent$1.invoke(TestExecutor.kt:25)
    at dev.minutest.internal.TestExecutor$andThen$1$runTest$testletForParent$1.invoke(TestExecutor.kt:17)
    at dev.minutest.internal.RootExecutor.runTest(TestExecutor.kt:36)
    at dev.minutest.internal.TestExecutor$andThen$1.runTest(TestExecutor.kt:28)
    at dev.minutest.internal.TestExecutor$andThen$1.runTest(TestExecutor.kt:28)
    at dev.minutest.internal.TestExecutor$DefaultImpls.runTest(TestExecutor.kt:12)
    at dev.minutest.internal.TestExecutor$andThen$1.runTest(TestExecutor.kt:17)
    at dev.minutest.junit.JUnit5MinutestsKt$toDynamicNode$1.execute(JUnit5Minutests.kt:59)

...

My expectation is that

deriveFixture: l2 skipped

would not need to be called since those tests are skipped.

dmcg commented 5 years ago

Hmm good point.

I'm not sure that there's an easy fix for this one. Because of the nature of the JUnit5 runner, skip is implemented as throwing an exception in the test method execution. In order to start that execution, the chain of fixture setup is invoked.

Does plain old JUnit, instantiate an instance of the test class just in order to not invoke its method I wonder? Probably not, but it has separate discovery and execution phases that the JUnit5 runner, based as it is on Streams, does not.

As you can probably tell by this stream of consciousness, I'm thinking this through. I'll let this issue mull for a while.

mkobit commented 5 years ago

I'm not sure that there's an easy fix for this one. Because of the nature of the JUnit5 runner, skip is implemented as throwing an exception in the test method execution. In order to start that execution, the chain of fixture setup is invoked.

That makes sense. I do like the current flexibility of minutest in that it just uses the dynamic test feature of JUnit under the hood, but dynamic tests don't support the various extensions APIs yet (https://github.com/junit-team/junit5/issues/378). @TestTemplate or a test engine could possibly solve this, but that is an entirely different track of code to support. I wonder if this issue could be solved if https://github.com/junit-team/junit5/issues/378 was complete (just pondering).