BenWoodworth/Parameterize

Replace contextual `parameterize` configuration with a better alternative

BenWoodworth opened this issue · 2 comments

The current approach to sharing a configuration via context in v0.2 can be awkward to use, and prone to error.

It was originally introduced as a way to nicely share a configuration contextually/implicitly to all parameterize calls in scope. Especially with the idea of scope properties being proposed for Kotlin, allowing all calls in the same class or file (or maybe project-wide, if something like C#'s implicit global imports were ever added, though that was just me being hopeful).

Leaning into future/undecided language features has been something I've been iffy about, especially since a new context proposal was introduced 4 days after this issue was created, and especially especially since the scope properties are only a "potential future enhancement" to the proposal.

Given all that though, even if context receivers and scope properties were already in Kotlin, I've started to lean away from a contextual parameterize overload being a good idea after having used it for a bit now. The original proposal advised against it, and it's starting to become clear why. Because the context is implicit and the signatures are the same, it's very easy to miss if the context isn't in scope resulting in the non-contextual overload being used. Whether that's because the context wasn't setup correctly, or the contextual parameterize call was copied somewhere else expecting it to work the same, it's an easy for hard-to-debug problems to occur. It also doesn't have the most ergonomic tooling with IntelliJ, since the IntelliSense completion will insert a fully-qualified non-contextual com.benwoodworth.parameterize.parameterize {} call when a ParameterizeContext is in scope (which won't be a problem when once the signature's changed from using an extension receiver to a context receiver, but for now, it's an issue).

Encouraging devs to wrap parameterize themselves

abstract class TestingContext {
    private val parameterizeTestConfiguration = ParameterizeConfiguration {
        decorator = { iteration ->
            if (!isFirstIteration) beforeTest()
            iteration()
            if (!isLastIteration) afterTest()
        }

        onFailure = {
            recordFailure = failureCount <= 10
        }
    }
    
    protected inline fun parameterizeTest(block: ParameterizeScope.() -> Unit): Unit =
        parameterize(parameterizeTestConfiguration, block)


    // ...
}
  • Pros:
    • More control to the library user, since they may not want calling code to be able to override the decorator, for example.
  • Cons:
    • Would add boilerplate if library users do want to have all the same parameters, and having them be overridable
    • ^plus, if new parameters are added, the wrapping function will need to be manually updated

Pre-configured invoke-able class

Instead, having a pre-configured parameterize val that can be invoked would be more flexible

  • can be named appropriately
  • vs. composing, since options don't need to be overridden

Example:

abstract class TestingContext {
    protected override val parameterizeTest = Parameterize {
        // Insert before/after calls around each parameterized
        // test case, except when already invoked by kotlin.test
        decorator = { iteration ->
            if (!isFirstIteration) beforeTest()
            iteration()
            if (!isLastIteration) afterTest()
        }

        // ...other shared configuration
    }

    open fun beforeTest() {}
    open fun afterTest() {}


    // The annotations would be lost when overriding beforeTest/afterTest,
    // so hook in here instead of relying on the subclass to apply them.
    @BeforeTest
    fun beforeTestHook(): Unit = beforeTest()

    @AfterTest
    fun afterTestHook(): Unit = afterTest()
}

class KotlinTestSpec : TestingContext() {
    override fun beforeTest() {
        // ...
    }

    override fun afterTest() {
        // ...
    }

    @Test
    fun a_string_should_contain_its_own_substring() = parameterizeTest {
        val substring = "substring"
        val prefix by parameterOf("prefix-", "")
        val suffix by parameterOf("-suffix", "")

        val string = "$prefix$substring$suffix"
        assertTrue(string.contains(substring), "\"$string\".contains(\"$substring\")")
    }
}

I'm leaning towards the wrapping approach, since can't think of a use case for pre-configuring and needing all options to be potentially overridable. If a good use case ever does arise, I can add in a solution that addresses that need later, instead of defensively providing an API for that now. It also wouldn't be hard to almost match fully overridable configuration options without much boilerplate. For example:

internal abstract class TestingContext {
    private val parameterizeTestConfiguration = ParameterizeConfiguration {
        decorator = { iteration ->
            if (!isFirstIteration) beforeTest()
            iteration()
            if (!isLastIteration) afterTest()
        }

        onFailure = {
            recordFailure = failureCount <= 10
        }
    }

    protected inline fun parameterizeTest(
        noinline configure: (ParameterizeConfiguration.Builder.() -> Unit)? = null,
        block: ParameterizeScope.() -> Unit
    ) {
        // Avoid building new configuration if no options are changed
        val configuration = if (configure != null) {
            ParameterizeConfiguration(parameterizeTestConfiguration, configure)
        } else {
            parameterizeTestConfiguration
        }
        
        parameterize(configuration, block)
    }


    // ...
}

And then with only slightly less appealing syntax with {} wrapping the options:

parameterizeTest({
    // Override parameterizeTest's default onFailure
    onFailure = { recordFailure = true }
}) {
    // ...
}

Composing a function with parameterize is proving to actually be a really powerful pattern. Maybe even more powerful than using a configuration because it can manage its own state (e.g. the skipped count, which parameterize doesn't track).

Take the example I ended up endorsing for kotlin.test, for instance:

protected inline fun parameterizeTest(
    recordFailures: Long = someDefault, // Example of how `parameterize` could get wrapped,
    maxFailures: Long = Long.MAX_VALUE, // exposing options according to the testing needs.
    block: ParameterizeScope.() -> Unit
): Unit = parameterize(
    // Inserts before & after calls around each test case,
    // except where already invoked by the test framework.
    decorator = { testCase ->
        if (!isFirstIteration) beforeTest()
        testCase()
        if (!isLastIteration) afterTest()
    },

    onFailure = { failure ->
        recordFailure = failureCount <= recordFailures
        breakEarly = failureCount >= maxFailures
    }
) {
    block()
}

Conceivably, it could be re-written without a configuration at all, plus have its own state which isn't possible with the ParameterizeConfiguration:

protected inline fun parameterizeTest(
    recordFailures: Long = someDefault,
    maxFailures: Long = Long.MAX_VALUE,
    block: ParameterizeScope.() -> Unit
){
    var passed = 0
    var failed = 0
    var skipped = 0

    val recordedFailures = mutableListOf<Throwable>()
    var isFirstTestCase = true
    
    run {
        parameterize {
            try {
                if (!isFirstTestCase) {
                    afterTest()
                    beforeTest()
                }

                isFirstTestCase = false
                block()
                passed++
            } catch (failure: Throwable) {
                when (failure) {
                    is ParameterizeFailedException -> throw failure
                    is TestSkippedException -> skipped++
                    else -> {
                        failed++
                        if (failed <= recordFailures) recordedFailures += failure
                        if (failed >= maxFailures) return@run
                    }
                }
            }
        }
    }
    
    if (failed > 0) {
        throw ParameterizeFailedError()
    }
}

Of course there are some things missing, like the current parameters/values aren't exposed through the ParameterizeScope's API, and the configuration does help to greatly reduce the boilerplate, but this does show just how powerful this approach is.