Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested "before" lifecycle hook #2792

Open
BryanDonovan opened this issue Jan 22, 2022 · 43 comments · May be fixed by #3867
Open

Nested "before" lifecycle hook #2792

BryanDonovan opened this issue Jan 22, 2022 · 43 comments · May be fixed by #3867
Assignees
Labels
enhancement ✨ Suggestions for adding new features or improving existing ones. framework 🏗️ Pertains to the core structure and components of the Kotest framework. pinned 📌 Issues of high importance or that need to remain visible.
Milestone

Comments

@BryanDonovan
Copy link

I'm trying to use Kotest to write nested tests that have hooks that only run once per nest level. This is how mocha works in JavaScript and how RSpec works in ruby. In mocha the function is called before. I tried using beforeContainer but it runs once for each nested container instead of just once for the entire container including any nested containers within that parent container. This is hard to explain, so below is a set of tests that I would expect to pass, if a before method existed in kotest that behaved like mocha's or RSpec's. In short, each before method should only be called once, and only when tests in its container run (e.g., the before method inside "nested level 2" shouldn't run until after the tests in "nested level 1" have run).

package foo.bar

import io.kotest.core.spec.style.DescribeSpec
import io.kotest.matchers.shouldBe

class WidgetTest : DescribeSpec({
    var foo = "initial"
    var counterOne = 0
    var counterTwo = 0
    var counterThree = 0

    before {
        foo = "bar"
        counterOne++
    }

    it("foo should be bar") {
        foo shouldBe "bar"
        counterOne shouldBe 1
    }

    it("beforeSpec should have been called only once") {
        foo shouldBe "bar"
        counterOne shouldBe 1
    }

    it("counterTwo should be 0") {
        counterTwo shouldBe 0
    }

    it("counterThree should be 0") {
        counterThree shouldBe 0
    }

    describe("nested level 1") {
        before {
            foo = "buzz"
            counterTwo++
        }

        it("foo should be buzz") {
            foo shouldBe "buzz"
        }

        it("and counterOne should be 1") {
            counterOne shouldBe 1
        }

        it("and counterTwo should be 1") {
            counterTwo shouldBe 1
        }

        describe("nested level 2") {
            before {
                foo = "jazz"
                counterThree++
            }

            it("foo should be jazz") {
                foo shouldBe "jazz"
            }

            it("and counterOne should be 1") {
                counterOne shouldBe 1
            }

            it("and counterTwo should be 1") {
                counterTwo shouldBe 1
            }

            it("and counterThree should be 1") {
                counterThree shouldBe 1
            }
        }
    }
})

I can get close to what I want if I make the first before a beforeSpec and the other two beforeContainer (and move them above each nested container), but then counterTwo gets incremented twice: once for "nested level 1" and once for "nested level 2". I'm not sure if I'm just doing something wrong or if this capability simply doesn't exist in kotest.

A somewhat more realistic example:

import io.kotest.core.spec.style.DescribeSpec
import io.kotest.matchers.shouldBe
import io.kotest.matchers.shouldNotBe
import kotlinx.coroutines.delay

class WidgetApiTest : DescribeSpec({
    describe("CRUD operations") {
        var widget: Widget? = Widget(0, "")

        describe("when we create a widget") {
            before { // This should run exactly once
                createWidget(1, "widget-a")
            }

            describe("when we fetch the widget") {
                before { // This should run exactly once
                    widget = getWidget(1) // assume this is a slow HTTP or database request
                }

                it("the widget should not be null") {
                    widget shouldNotBe null
                }

                it("and the widget should have a name") {
                    widget!!.name shouldBe "widget-a"
                }

                describe("when we update the widget's name") {
                    before { // the above "before" functions should not have run again in this test container
                        widget?.let { w ->
                            w.name = "new-name"
                            updateWidget(w)
                        }
                    }

                    it("then the re-fetched widget should be updated") {
                        widget = getWidget(1) // assume this is a slow HTTP or database request
                        widget!!.name shouldBe "new-name"
                    }
                }
            }
        }
    }
})

data class Widget(val id: Int, var name: String?)

val widgetContainer: HashMap<Int, Widget> = hashMapOf()

suspend fun createWidget(id: Int, name: String?): Widget {
    delay(200) // slow HTTP request or Database call
    val widget = Widget(id, name)
    widgetContainer[id] = widget
    return widget
}

suspend fun getWidget(id: Int): Widget? {
    delay(200) // slow HTTP request or Database call
    return widgetContainer[id]
}

suspend fun updateWidget(widget: Widget): Widget {
    delay(200) // slow HTTP request or Database call
    val id = widget.id
    widgetContainer[id] = widget
    return widget
}
@BryanDonovan BryanDonovan added the enhancement ✨ Suggestions for adding new features or improving existing ones. label Jan 22, 2022
@sksamuel
Copy link
Member

sksamuel commented Jan 22, 2022 via email

@BryanDonovan
Copy link
Author

Yes, exactly! I guess that was an easier way of saying it :)

@sksamuel
Copy link
Member

sksamuel commented Jan 22, 2022 via email

@BryanDonovan
Copy link
Author

e.g,. this javascript suite passes using mocha:

const assert = require('assert');

describe("Nesting", () => {
    var foo = "initial";
    var counterOne = 0;
    var counterTwo = 0;
    var counterThree = 0;

    before(() => {
        foo = "bar";
        counterOne++;
    });

    it("foo should be bar", () => {
        assert.equal(foo, "bar");
        assert.equal(counterOne, 1);
    });

    it("before() should have been called only once", () =>  {
        assert.equal(foo, "bar");
        assert.equal(counterOne, 1);
    });

    it("counterTwo should be 0", () =>  {
        assert.equal(counterTwo, 0);
    });

    it("counterThree should be 0", () =>  {
        assert.equal(counterThree, 0);
    });

    describe("nested level 1", () => {
        before(() => {
            foo = "buzz";
            counterTwo++;
        });

        it("foo should be buzz", () => {
            assert.equal(foo, "buzz");
        });

        it("and counterOne should be 1", () => {
            assert.equal(counterOne, 1);
        });

        it("and counterTwo should be 1", () => {
            assert.equal(counterTwo, 1);
        });

        describe("nested level 2", () => {
            before(() => {
                foo = "jazz";
                counterThree++;
            });

            it("foo should be jazz", () => {
                assert.equal(foo, "jazz");
            });

            it("and counterOne should be 1", () => {
                assert.equal(counterOne, 1);
            });

            it("and counterTwo should be 1", () => {
                assert.equal(counterTwo, 1);
            });

            it("and counterThree should be 1", () => {
                assert.equal(counterThree, 1);
            });
        });
    });
});

@BryanDonovan
Copy link
Author

We could add this and call it beforeScope to make it clear it only runs
before that particular scope.

That would be awesome. Thanks!

@sksamuel
Copy link
Member

We can put this in 5.2, which is due in a month or so.

@sksamuel sksamuel added this to the 5.2 milestone Jan 22, 2022
@sksamuel sksamuel added the framework 🏗️ Pertains to the core structure and components of the Kotest framework. label Jan 22, 2022
@Reevn
Copy link
Contributor

Reevn commented Jan 23, 2022

I'd suggest to think carefully about choosing a name for this particular callback. We already have some ambiguity with callback names for historic reasons. We currently have:

Historically:

  • beforeTest - runs before all containers and leaf tests

Newish:

  • beforeEach - runs before all leaf tests (naming suboptimal as beforeTest already existed, but this naming is used in a lot of other frameworks for the specified behavior)
  • beforeContainer - runs before all containers
  • beforeAny - runs before all containers and leaf tests (same as beforeTest and meant as a replacement in the long run)

Instead of introducing yet another concept with scope, I would throw something quite literal like beforeThisContainer into the mix. Might not be as elegant of a name, but it describes pretty clearly what the difference to the standard beforeContainer is.

@BryanDonovan
Copy link
Author

I was thinking the same thing, @Reevn. Maybe even beforeThisContainerOnce but that's getting a little too verbose.

@BryanDonovan
Copy link
Author

I'm new to Kotest and Kotlin, so please forgive my naivety, but I honestly don't get why there aren't just two "before" callbacks: a beforeAll and a beforeEach. I think they cover every possibility I've ever run into in the 15 years I've been using a "describe" style to write tests. Perhaps it's something to do with running tests in parallel? Either way, I really appreciate the new feature.

@Reevn
Copy link
Contributor

Reevn commented Jan 23, 2022

It's a combination of trying to make a really flexible test framework and historical reasons. I've had some of the same questions in the past and have been made aware that people are writing tests in pretty different styles. Some people like to put test code inside a "container", but without wrapping it in a "leaf test". For those people for example it is important that something like beforeTest executes before the "container" as well (and not just for tests inside the container).

I personally did not expect that and needed the classical beforeEach behavior that only executes before a "leaf test", and beforeTest executes twice in this case. That's when beforeEach and beforeContainer was introduced, to have more flexibility on when exactly you want to execute setup code.

Your issue adds another use-case to the mix, so probably another callback will be born 🙂. Offering this kind of flexibility is the reason why there are so many different callbacks and styles of writing tests.

@BryanDonovan
Copy link
Author

Thanks for detailed response, @Reevn!

@sksamuel
Copy link
Member

beforeTest is a confusing name but as @Reevn says, history. I'd love it if beforeTest was only leaf, but alas.

beforeScope is a good name I think because the lambdas are called scopes in kotest - container scope, test scope. But I agree that we need to tread carefully.

Another option is simply overloading beforeContainer to have a paramaeter

beforeContainer(inherit = false) { }

@BryanDonovan
Copy link
Author

I'd personally like to see this new callback be called from within the container. Main reason is I like to set up empty variables inside the container and then assign values to them inside the before* function. Basically setting up a SUT and possibly related variables for the tests within the container. I guess I'm thinking of this as not "before container" but as "before all tests in container."

@sksamuel
Copy link
Member

Yeah it's just a matter of choosing a good name.

beforeScope / setup / teardown / etc,

Something like beforeThisContainerOnly is descriptive but ugly.

@tibtof
Copy link
Contributor

tibtof commented Feb 16, 2022

I think that beforeRootContainer would be semantically equivalent to beforeThisContainerOnly, but nicer.

@sksamuel sksamuel modified the milestones: 5.2, 5.3 Mar 12, 2022
@sksamuel
Copy link
Member

What about just before ?

@BryanDonovan
Copy link
Author

BryanDonovan commented Mar 28, 2022

I think before is best (for me anyway) because it aligns with some other test frameworks. For some reason I was under the impression that before couldn't be used in Kotest, but not sure why I thought that.

@Reevn
Copy link
Contributor

Reevn commented Mar 29, 2022

With the current names I think just before does not differentiate itself from the existing lifecycle callbacks. It would be the most generic before* function, but it does something pretty specific. I don't think a new user would see beforeEach, beforeTest, beforeAny, beforeContainer and then before, and intuitively understand that before is just beforeContainer but only for the current container.

@BryanDonovan
Copy link
Author

Good point @Reevn

@sksamuel
Copy link
Member

sksamuel commented Mar 29, 2022 via email

@Kantis
Copy link
Member

Kantis commented Mar 29, 2022

What if we used @DslMarker to restrict access to the root Spec when using beforeTest, beforeEach, beforeAny, beforeContainer, and before could be made available only on ContainerScope. Would that make it intuitive?

@Reevn
Copy link
Contributor

Reevn commented Mar 31, 2022

@Kantis I don't think that restricting the function to the ContainerScope helps to make users understand that it is not inherited to other children ContainerScopes. That's my personal opinion at least.

I'll +1 @sksamuel's suggestion earlier in the thread:

beforeContainer(inherit = false) { }

It already uses the function that is responsible for setting up a container scope, so no new concept or naming introduced, while making it configurable via a parameter to not inherit this callback to children containers. Looks like a pretty good addition to existing API without adding a lot more complexity to the mix (from the public API perspective).

@jarhoax
Copy link

jarhoax commented Mar 31, 2022

Following the thread and seeing all the suggestions I also think

beforeContainer(inherit = false) { } 

is pretty much on point. It is clear from reading it what will happen. Also as @Reevn mentioned the concept of beforeContainer already exists, with this change having the possibility to "limit" with an additional parameter. Awesome.

@BryanDonovan
Copy link
Author

BryanDonovan commented Mar 31, 2022

My issue with using beforeContainer is that's outside the container. I want something that is before the tests within a container -- in that case it makes more intuitive sense (and is consistent with other frameworks that Kotest's nesting was based on) to put the new hook inside the container that contains the tests. Having it outside the container makes the test code confusing to me.

However, I just realized that the following works. Basically removing the before hooks from my first example and putting the code directly inside the container. Unless there's a drawback I'm not seeing, I think this actually fixes the issue for me. I probably didn't realize this when I filed this issue in the first place since I had only been using kotest for a week or so at the time. It would still be nice to have a before<something> hook for consistency/clarity.

EDIT: The below works as a "before" hook, but there's no clean equivalent "after" hook.

package foo.bar

import io.kotest.core.spec.style.DescribeSpec
import io.kotest.matchers.shouldBe

class WidgetTest : DescribeSpec({
    var foo = "initial"
    var counterOne = 0
    var counterTwo = 0
    var counterThree = 0

    foo = "bar"
    counterOne++

    it("foo should be bar") {
        foo shouldBe "bar"
        counterOne shouldBe 1
    }

    it("beforeSpec should have been called only once") {
        foo shouldBe "bar"
        counterOne shouldBe 1
    }

    it("counterTwo should be 0") {
        counterTwo shouldBe 0
    }

    it("counterThree should be 0") {
        counterThree shouldBe 0
    }

    describe("nested level 1") {
        foo = "buzz"
        counterTwo++

        it("foo should be buzz") {
            foo shouldBe "buzz"
        }

        it("and counterOne should be 1") {
            counterOne shouldBe 1
        }

        it("and counterTwo should be 1") {
            counterTwo shouldBe 1
        }

        describe("nested level 2") {
            foo = "jazz"
            counterThree++

            it("foo should be jazz") {
                foo shouldBe "jazz"
            }

            it("and counterOne should be 1") {
                counterOne shouldBe 1
            }

            it("and counterTwo should be 1") {
                counterTwo shouldBe 1
            }

            it("and counterThree should be 1") {
                counterThree shouldBe 1
            }
        }
    }
})

@jarhoax
Copy link

jarhoax commented Apr 1, 2022

My issue with using beforeContainer is that's outside the container. I want something that is before the tests within a container

I see. That would mean if you have following code example:

class TestSpec : UnitSpec({

    beforeContainer {
        println("beforeContainer: ${it.displayName}")
    }

    describe("first container") {
        beforeContainer {
            println("Another beforeContainer: ${it.displayName}")
        }

        describe("with nested") {
            it("does something") {

            }
        }

        it("does something else") {

        }
    }
})

it will print:

beforeContainer: first container
beforeContainer: with nested
Another beforeContainer: with nested

So by introducing a parameter inherit = false it will only honor the first level child containers, but not the actual current container scope. This would probably require something like beforeThisContainer like @Reevn said, acting similar than beforeSpec, but for the current container scope.

@BryanDonovan
Copy link
Author

True, but it's also unconventional :)

@sksamuel
Copy link
Member

sksamuel commented May 2, 2022

Yep, that I agree with.

@stale
Copy link

stale bot commented Jun 4, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale 🏚️ Issues that were lost to time and are no longer up-to-date label Jun 4, 2022
@BryanDonovan
Copy link
Author

Bump for staleness

@stale stale bot removed the stale 🏚️ Issues that were lost to time and are no longer up-to-date label Jun 5, 2022
@sksamuel sksamuel added the pinned 📌 Issues of high importance or that need to remain visible. label Jun 6, 2022
@akefirad
Copy link

akefirad commented Sep 10, 2022

Just to give more ❤️ to the feature, here's my use case:

class TestingTest : BehaviorSpec({
    beforeSpec {
        dbHelper.createTableIfNotExists()
    }

    beforeContainer {
        dbHelper.deleteAll()
    }

    Given("Feature X with dataset Y in DB") {
        // prepare fixtures
        // add some data to DB
        // do other stuff

        When("subject does something") {
            subject.doSomething()

            Then("foo should be foo") {
                "foo" shouldBe "foo"
            }
            Then("bar should be bar") {
                "bar" shouldBe "bar"
            }
        }
    }
})

Now, as discussed, beforeContainer is gonna delete all the data added in Given container while executing When container which is not what we want. The only solution I can think of is to move "given" statements to "when" container which is a bit weird, considering the terminology.
This limitation makes this style unusable (at least for me) with respect to readability. The fact that no one pointed this out earlier, tells me maybe there is a better way to do this 🤷? (happy to learn 🙂)
Anyhow, would be wonderful if someone can give us a guesstimation on the availability of the feature. Thanks.
PS: I'm new to Kotlin, not sure how much, but let me know if I can help with anything.

@BryanDonovan
Copy link
Author

Any chance this will be added soon? I'm fine with any name really.

@BryanDonovan
Copy link
Author

This feature might go a long ways toward convincing co-workers to use this framework.

@dondod
Copy link

dondod commented Feb 4, 2023

I agree, this would help convince coworkers to adopt kotest. I'm rather new to kotlin, but would be willing to try doing a pull request, but I need a bit of guidance on how to do that. I'm having trouble understanding how all the hooks work.

I think kotest would be perfect if it had this feature.

@sksamuel
Copy link
Member

sksamuel commented Feb 4, 2023

I think the name before and after without qualifier might be the right terminology. @Kantis thoughts?

@sksamuel sksamuel added this to the 5.6 milestone Feb 4, 2023
@sksamuel sksamuel self-assigned this Feb 4, 2023
@Kantis
Copy link
Member

Kantis commented Feb 4, 2023

how about beforeCurrent, afterCurrent? I think it helps distinguish itself from other hooks. But I wouldn't say no to just before/after either. :)

class TestingTest : BehaviorSpec({
    beforeSpec {
        dbHelper.createTableIfNotExists()
    }

    beforeContainer {
        dbHelper.deleteAll()
    }

    Given("Feature X with dataset Y in DB") {
        beforeCurrent { prepareFixtures() }
        afterCurrent { teardownDb() }
        afterEach { truncateTables() }

        When("subject does something") {
            subject.doSomething()

            Then("foo should be foo") {
                "foo" shouldBe "foo"
            }
            Then("bar should be bar") {
                "bar" shouldBe "bar"
            }
        }
    }
})

@sksamuel
Copy link
Member

sksamuel commented Feb 4, 2023

Hmmm I'm not keen on that personally :)

@Kantis
Copy link
Member

Kantis commented Feb 4, 2023

Alright, go with before, after then. You're the native english-speaker after all.😁

@sksamuel
Copy link
Member

sksamuel commented Feb 4, 2023

I communicate in grunts.

@monosoul
Copy link

monosoul commented Mar 3, 2023

Was about to start using kotest in a project, but then stumbled upon this hook missing. Very much looking forward to this feature. Thanks!

@monosoul
Copy link

monosoul commented Mar 3, 2023

My workaround for BehaviorSpec for now:

abstract class BeforeEachCapableBehaviorSpec(body: BehaviorSpec.() -> Unit = {}) : BehaviorSpec(body) {

    final override suspend fun beforeContainer(testCase: TestCase) {
        if (testCase.parent == null) {
            beforeEach()
        }
    }

    fun beforeEach(block: () -> Unit) {
        beforeContainer {
            if (it.parent == null) {
                block()
            }
        }
    }

    open fun beforeEach() {}

    final override suspend fun afterContainer(testCase: TestCase, result: TestResult) {
        if (testCase.parent == null) {
            afterEach()
        }
    }

    open fun afterEach() {}

    fun afterEach(block: () -> Unit) {
        afterContainer { (testCase, _) ->
            if (testCase.parent == null) {
                block()
            }
        }
    }
}

Method names are like that to better match JUnit's annotation names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ Suggestions for adding new features or improving existing ones. framework 🏗️ Pertains to the core structure and components of the Kotest framework. pinned 📌 Issues of high importance or that need to remain visible.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants