Reusable suite-local fixtures is allocated after suite's beforeAll()

[kamilkloch]

Update: munit v1.0.0-M6.

The following code

class TestMUnitResource extends munit.FunSuite {
  val resource = new Fixture[Double]("Double resource") {
    private var d: Double = Double.NaN

    def apply() = d

    override def beforeAll(): Unit = {
      println("Acquiring resource")
      d = 4.0
    }

    override def afterAll(): Unit = {
      println(s"Releasing resource $d")
      d = Double.NaN
    }
  }

  override def munitFixtures = List(resource)

  test("test1") {
    println(s"Using resource [${resource()}]")
  }

  override def beforeAll(): Unit = {
    println(s"Suite beforeAll: ${resource()}")
  }

  override def afterAll(): Unit = {
    println(s"Suite afterAll: ${resource()}")
  }
}

produces

Suite beforeAll: NaN
Acquiring resource
Using resource [4.0]
Suite afterAll: 4.0
Releasing resource 4.0

It looks that suite-local fixture is allocated after beforeAll and released after afterAll . Is this the desired behavior?

[armanbilge]

Is this matching the behavior described in the breaking changes for munit 1.x? https://github.com/scalameta/munit/releases/tag/v1.0.0-M1

[kamilkloch]

Is this matching the behavior described in the breaking changes for munit 1.x? https://github.com/scalameta/munit/releases/tag/v1.0.0-M1

It does indeed.

[valencik]

@kamilkloch Because this behaviour changes in v1.0, would you please update your issue with the explicit version you're using. Thanks :)

But yes, this is the desired behaviour in the v1.0 release line. Copying from https://github.com/scalameta/munit/releases/tag/v1.0.0-M1

-- old order
++ new order
  MySuite.beforeAll()
  myFixture.beforeAll()
  MySuite.beforeEach(test-1)
  myFixture.beforeEach(test-1)
+ MySuite.afterEach(test-1)
  myFixture.afterEach(test-1)
- MySuite.afterEach(test-1)
+ MySuite.afterAll()
  myFixture.afterAll()
- MySuite.afterAll()

The Suite runs beforeAll first, then the fixture runs beforeAll. At the end, the Suite runs afterAll first, and then the fixture runs afterAll

[kamilkloch]

Done.

[kamilkloch]

But yes, this is the desired behaviour in the v1.0 release line.

What is the rationale of such change, if I may ask? Example against such behavior: I would like to use suite-local fixture to create a shared api client, login some users in beforeAll, logout the users in afterAll. Not possible, as beforeAll runs before the fixture is ready.

[mzuehlke]

Looking at the "issue" I realized that the fixtures (including the suite-local fixture) are teared town in the exact same order they are initialized.

Wouldn't it make more sense to call the afterAll and afterEach function on the reverse order of the fixtures ? So that the last created resources are destroyed first ?

So that FixtureOrderFrameworkSuite would look like (for only 1 test):

beforeAll(ad-hoc)
beforeAll(a)
beforeAll(b)
beforeEach(ad-hoc, 1)
beforeEach(a, 1)
beforeEach(b, 1)
test(1)
afterEach(b, 1)
afterEach(a, 1)
afterEach(ad-hoc, 1)
afterAll(b)
afterAll(a)
afterAll(ad-hoc)

[olafurpg]

@kamilkloch The motivation for this behavior is based on the (perhaps false?) assumption that fixtures should be independent of each other and it was therefore a nice simplification to remove the special-case for the override def beforeAll/afterAll methods. With the new logic, suite-local (override def beforeAll/afterAll) methods are implemented as regular fixtures. You have full control over the order of fixtures by overriding override def munitFixtures, and override def beforeAll/afterAll are pure convenience methods to implement a normal fixture.

So that the last created resources are destroyed first ?

@mzuehlke the motivation for this change was based on the assumption that fixtures are independent and the order matter should therefore ideally not matter. If you have multiple fixtures that dependent on each then you have two options:

• merge the fixtures into a single fixture
• separate each fixture into two fixtures that each contain only the before/after parts, and then explicitly specify the reverse order

Is that a suitable solution for your use-case? I'm on the fence about merging your PR since it introduced a third ordering that has never been used before. In the 0.x series, there was only a special case for the suite-local fixtures that made before* methods runs first and after* methods run last.

Writing this comment, I realized that we can avoid the v1.x breaking change by using the "separate each fixture into two fixtures" trick for suite-local methods. This would preserve the ordering from the 0.x series while giving users full-control over how to define the ordering of fixtures.

[mzuehlke]

I like your approach, of keeping munit simple on the one hand and having multiple fixtures that rely on each other is probably not leading to more readable test.

The 2 options you mention are in my eyes 2 ways to "fix" this problem. Maybe they are worth to be but somewhere in the Fixture documentation.

When preparing my PR especially one comment nearly stopped me:

https://github.com/scalameta/munit/blob/a2f1b913c7b04532572cde32af39f7e88329bb59/tests/jvm/src/test/scala/munit/AsyncFixtureSuite.scala#L82-L88

[valencik]

I've opened a new PR to revert to the v0.7.x style ordering: https://github.com/scalameta/munit/pull/724

By reverting we avoid a breaking change from v0.7.x to v1.0.x (once released). However, the v1 milestones have been around for so long, this is effectively a second breaking change for the ecosystem that did migrate to the v1 milestones. That is unfortunate, no doubt.

However, in this situation, I believe there have been enough reports about this being surprising behaviour, that it is worth reverting to the old, and perhaps more intuitive, ordering.

[tgodzik]

Should we close this? Are we ok having the change for 1.0.0?

[valencik]

I think we can close this as completed in https://github.com/scalameta/munit/pull/724