Use consistent solution to customize tests, suites and values

[olafurpg]

Opening this as an alternative proposal to #61 (cc/ @kubukoz).

Previously, users could override several methods to customize how tests
were transformed, how suites filtered tests and how values got lifted
into futures. While this system worked OK, it wasn't easy to explain
since it was quite inconsistent.

Now, this commit introduces three consistent solutions to transform:

• test cases: a function Test => Test
• test suites: a function List[Test] => List[Test]
• test values: a partial function Any => Future[Any]

Users register new transforms using a similar API as you register fixtures:

  override def munitTestTransforms: List[TestTransform] =
    super.munitTestTransforms ++ List(
      new TestTransform("append scala version", { test =>
        test.withName(test.name + "-" + scalaVersion)
      })
    )

Opening this as a draft PR to see what people think. It's quite a breaking change but at the moment I feel like it's a nice improvement.

[olafurpg]

The idea to register transforms using the same syntax as registering tests comes from JavaScript libraries like Jest/Ava, see https://jestjs.io/docs/en/setup-teardown

I started with the following syntax

override def munitTestTransforms = super.munitTestTransforms :+ {
test => ... }

but it's quite overwhelming to type, it's easy to get wrong and it's not IDE friendly.

[gabro]

How do multiple transforms combine?

transformValue("Lift task to future") {
  case LazyFuture(runFuture()) => runFuture()
}

transformValue("Another one") {
  case LazyFuture(runFuture()) => Future.successful(42)
}

I'm all for expressive APIs, but I'm a bit afraid that the mutable nature of the underlying representation will leak if we allow order-dependent transformations.

Overriding a method has the benefit of ensuring uniqueness of the transformation and I think it's a more natural API in the JVM world, where we have classes (as opposed to JS frameworks where classes are very different and seldom used in public APIs)

[olafurpg]

Transforms are indeed order-dependent, and I agree it may get tricky to reason about the order. Having tried both approaches however, I feel that it's really easy to mess up the override logic and for example accidentally remove the default transforms (lift future). It's quite tricky to debug what's wrong in these cases.

One possible way to avoid the ordering issues would be to allow registering a "priority" alongside transforms. MUnit would sort the transforms according to the priority before applying them.

transformValue("Lift task to future", Priority.High) {
  case LazyFuture(runFuture()) => runFuture()
}

transformValue("Another one", Priority.Low) {
  case LazyFuture(runFuture()) => Future.successful(42)
}

Would that ease your concerns?

[olafurpg]

We can BTW record munit.Location for transforms and print a detailed trace of what transforms get applied in what order under some debug flag. This could help shed light on what's going on if something is not working as expected.

[olafurpg]

After thinking more about it, I am also open to remove the mutable transformValue("name") { .. } helpers but keep the other changes. The override def syntax is not that bad after introducing the SuiteTransform TestTransform ValueTransform classes.

override val valueTransforms = List(
  suiteTransform("a") { ... },
  ...
)

It would be nice to have a splice operator like in JS, however 🤔

[gabro]

I feel like adding priority is just going to make the whole thing even more difficult to reason about (now I need to look at priorities to reconstruct the order in my head).

I like the other changes, but I much prefer to have a single entry point for this API (the override syntax of your last post looks good enough to me)

[olafurpg]

Sounds good, I agree the priorities only complicate things. I'll remove the mutable syntax to register transforms.

[olafurpg]

I removed the mutable transformTest(...) syntax so that it's now override based instead

  override def munitTestTransforms: List[TestTransform] =
    super.munitTestTransforms ++ List(
      new TestTransform("append scala version", { test =>
        test.withName(test.name + "-" + scalaVersion)
      })
    )

I didn't add helper methods to construct transforms because I'm not sure if it's helpful. A plain Scala class felt simple enough to explain and in Scala 3 the new keyword won't be needed.

[olafurpg]

@kubukoz the code to handle a lazy future is now like this

  case class LazyFuture[+T](run: () => Future[T])
  object LazyFuture {
    def apply[T](thunk: => T)(implicit ec: ExecutionContext): LazyFuture[T] =
      LazyFuture(() => Future(thunk))
  }

  override def munitValueTransforms: List[ValueTransform] =
    super.munitValueTransforms ++ List(
      new ValueTransform("LazyFuture", {
        case LazyFuture(run) => run()
      })
    )

This API makes it easier to the right thing and harder to the wrong thing. No more fiddling with => Any or mixing up the order to call super.munitRunTest. What do you think?

[olafurpg]

I'll give this a few days to brew before merging since it's a big breaking change.

[olafurpg]

A side-effect of this change is that value transforms automatically flatten custom Task-like types. Previously, the case below was difficult to handle with a custom munitTestValue but with value transforms the lazy future is automatically flattened (for better or worse)

  test("nested".fail) {
    LazyFuture {
      LazyFuture {
        // Test will fail because  LazyFuture.run()` is automatically called
        throw new RuntimeException("BOOM!")
      }
    }
  }

[olafurpg]

@kubukoz I'm happy to help migrate code to the new APIs if you have already started working on a cats-effect integration or something like that. I really want to stabilize the MUnit API as soon as possible to avoid further breaking changes.

[olafurpg]

FWIW, following up from an offline conversation with @gabro I'm open to host integration modules (cats-effect, hedgehog, scalacheck, zio, ...) in the MUnit repo. I believe that would make it easier to deal with breaking changes like this

[MrPowers]

The override based syntax looks great.

@olafurpg @gabro - great job collaborating & making a great interface!

[kubukoz]

I'll try to read this again in a couple hours, sorry for being late 😅

[olafurpg]

@kubukoz no rush, take your time. I updated the existing docs but didn't add any new docs. Please let me know if there's anything unclear

[kubukoz]

I'm fairly satisfied with overriding, can't foresee any problemws with that at this point, so you have my blessing :)

as for a CE integration, there really isn't that much code and it'll be a low cost migration. It's pretty much what the LazyFuture override does.

And I'm definitely up for having a module in the repo :)

[olafurpg]

Thanks! I'll cut a milestone release to see if there's an unexpected regression

[olafurpg]

0.6.0-M1 is out with these changes.