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

Add cats-effect support for FunSuite and FunFixtures #134

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 40 additions & 13 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,23 @@ lazy val plugin = project
)
)

lazy val munitCatsEffect = crossProject(JSPlatform, JVMPlatform)
.in(file("munit-cats-effect"))
.dependsOn(munit)
.settings(
moduleName := "munit-cats-effect",
sharedSettings,
libraryDependencies += ("org.typelevel" %%% "cats-effect" % "2.1.3")
.withDottyCompat(scalaVersion.value)
)
.jvmSettings(
sharedJVMSettings,
skip in publish := customScalaJSVersion.isDefined
)
.jsSettings(sharedJSSettings)
lazy val munitCatsEffectJVM = munitCatsEffect.jvm
lazy val munitCatsEffectJS = munitCatsEffect.js

lazy val munitScalacheck = crossProject(JSPlatform, JVMPlatform, NativePlatform)
.in(file("munit-scalacheck"))
.dependsOn(munit)
Expand All @@ -229,33 +246,43 @@ lazy val munitScalacheckJVM = munitScalacheck.jvm
lazy val munitScalacheckJS = munitScalacheck.js
lazy val munitScalacheckNative = munitScalacheck.native

lazy val tests = crossProject(JSPlatform, JVMPlatform, NativePlatform)
.dependsOn(munit, munitScalacheck)
lazy val testsSettings = List(
buildInfoPackage := "munit",
buildInfoKeys := Seq[BuildInfoKey](
"sourceDirectory" ->
baseDirectory.in(ThisBuild).value / "tests" / "shared" / "src" / "main",
scalaVersion
),
skip in publish := true
)
lazy val tests = crossProject(JSPlatform, JVMPlatform)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the tests project unchanged and introduce instead new source directories like src/test/non-native for the parts of the cross-build that cats-effect supports. Use .jvmConfigure(_.dependsOn(munitCatsEffect)).jsConfigure(_.dependsOn(munitCatsEffect)) to skip the native dependency

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also fine to have a src/test/cats-effect directory if that simplifies things. I want to avoid multiple test projects because it's easiest to reason about the build when you can run testsJVM/test to run all tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely understand what you mean :)

I currently have created a new directory munit-cats-effect at the root of the project.
Would you like to have a new directory in the tests folder? Or where do you see this new non-native source directory in the project?

.dependsOn(munit, munitCatsEffect, munitScalacheck)
.enablePlugins(BuildInfoPlugin)
.settings(
sharedSettings,
buildInfoPackage := "munit",
buildInfoKeys := Seq[BuildInfoKey](
"sourceDirectory" ->
baseDirectory.in(ThisBuild).value / "tests" / "shared" / "src" / "main",
scalaVersion
),
skip in publish := true
testsSettings
)
.nativeConfigure(sharedNativeConfigure)
.nativeSettings(sharedNativeSettings)
.jsSettings(sharedJSSettings)
.jvmSettings(
sharedJVMSettings,
fork := true
)
lazy val testsN = crossProject(NativePlatform)
.dependsOn(munit, munitScalacheck)
.enablePlugins(BuildInfoPlugin)
.settings(
sharedSettings,
testsSettings
)
.nativeConfigure(sharedNativeConfigure)
.nativeSettings(sharedNativeSettings)
lazy val testsJVM = tests.jvm
lazy val testsJS = tests.js
lazy val testsNative = tests.native
lazy val testsNative = testsN.native

lazy val docs = project
.in(file("munit-docs"))
.dependsOn(munitJVM, munitScalacheckJVM)
.dependsOn(munitJVM, munitCatsEffectJVM, munitScalacheckJVM)
.enablePlugins(MdocPlugin, MUnitReportPlugin, DocusaurusPlugin)
.settings(
sharedSettings,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package munit

import cats.effect.{IO, Resource}
import cats.syntax.flatMap._

import scala.concurrent.Promise

trait CatsEffectFunFixtures extends FunFixtures { self: CatsEffectSuite =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could somehow support suite-local and test-local resources via Fixture[T]. The problem is that Fixture[T] doesn't support async before/after methods but I wonder if that is something we should consider adding to improve this use-case 🤔

I am concerned it would have to be a breaking change unless we introduce something like AsyncFixture[T], but I would love to avoid having separate types for sync/async fixtures.

I personally prefer Fixture[T] over FunFixture[T] because 1) it's easier to compose them when you have many and 2) they require less boilerplate per test case. It's also useful to have suite-local resources like a database connection.

This doesn't have to be blocking for this PR, I'm just thinking out loud here. We could go ahead and merge this change for now and add Fixture[T] support separately.


object CatsEffectFixture {

def fromResource[T](
resource: Resource[IO, T]
): FunFixture[T] = fromResource(
resource,
(_, _) => IO.pure(()),
_ => IO.pure(())
)

def fromResource[T](
resource: Resource[IO, T],
setup: (TestOptions, T) => IO[Unit],
teardown: T => IO[Unit]
): FunFixture[T] = {
val promise = Promise[IO[Unit]]()

FunFixture.async(
setup = { testOptions =>
implicit val ec = munitExecutionContext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK/expected that munitExecutionContext is parasitic by default? Users can always override it if they want something else. I felt that a parasitic EC was the best default to 1) have deterministic test behaviors and 2) keep stack traces clean.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issue with the parasitic EC :)

Copy link

@SystemFw SystemFw Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as it turns out, this causes deadlock so you are forced to override it. It also doesn't make for a great default imho (at least not for cats-effect), because the failure mode is "oh no there is a deadlock", and you don't expect it to come from your test framework so it can be baffling

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's where I'm coming from typelevel/munit-cats-effect#65

I don't expect munit to take any action at this point (I would have opened a PR), but I thought it was interesting to share if someone else comes across the discussion :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, disregard my comment then :)


val resourceEffect = resource.allocated
val setupEffect =
resourceEffect
.map {
case (t, release) =>
promise.success(release)
t
}
.flatTap(t => setup(testOptions, t))

setupEffect.unsafeToFuture()
},
teardown = { argument: T =>
implicit val cs = munitContextShift

teardown(argument)
.flatMap(_ => IO.fromFuture(IO(promise.future)).flatten)
.unsafeToFuture()
}
)
}

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package munit

import cats.effect.{ContextShift, IO, Timer}

abstract class CatsEffectSuite extends FunSuite {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have some open questions on this before I can continue :)


def munitContextShift: ContextShift[IO] =
IO.contextShift(munitExecutionContext)

def munitTimer: Timer[IO] =
IO.timer(munitExecutionContext)

override def munitValueTransforms: List[ValueTransform] =
super.munitValueTransforms ++ List(munitIOTransform)

final def munitIOTransform: ValueTransform =
new ValueTransform("IO", { case e: IO[_] => e.unsafeToFuture() })

}
61 changes: 61 additions & 0 deletions tests/jvm/src/test/scala/munit/IOFixtureSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package munit

import cats.effect.{IO, Resource}

import scala.concurrent.{Future, Promise}

class IOFixtureSuite extends CatsEffectSuite with CatsEffectFunFixtures {
val latch: Promise[Unit] = Promise[Unit]
var completedFromTest: Option[Boolean] = None
var completedFromTeardown: Option[Boolean] = None

var completedFromResourceAcquire: Option[Boolean] = None
var completedFromResourceRelease: Option[Boolean] = None

val latchOnTeardown: FunFixture[String] =
CatsEffectFixture.fromResource[String](
resource = Resource.make[IO, String](
IO {
completedFromResourceAcquire = Some(true)
"test"
}
)(_ =>
IO {
completedFromResourceRelease = Some(true)
}
),
setup = { (_: TestOptions, _: String) =>
IO {
completedFromResourceAcquire = Some(false)
}
},
teardown = { _: String =>
IO {
completedFromResourceRelease = Some(false)
completedFromTeardown = Some(latch.trySuccess(()));
}
}
)

override def afterAll(): Unit = {
// resource was created before setup
assertEquals(completedFromResourceAcquire, Some(false))
// resource was released after teardown
assertEquals(completedFromResourceRelease, Some(true))
// promise was completed first by the test
assertEquals(completedFromTest, Some(true))
// and then there was a completion attempt by the teardown
assertEquals(completedFromTeardown, Some(false))
}

latchOnTeardown.test("teardown runs only after test completes") { _ =>
import scala.concurrent.ExecutionContext.Implicits.global
Future {
// Simulate some work here, which increases the certainty that this test
// will fail by design and not by lucky scheduling if the happens-before
// relationship between the test and teardown is removed.
Thread.sleep(50)
completedFromTest = Some(latch.trySuccess(()))
}
}
}
41 changes: 41 additions & 0 deletions tests/jvm/src/test/scala/munit/IOSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package munit

import cats.effect.IO

import scala.concurrent.duration._

class IOSuite extends CatsEffectSuite {

test("nested fail".fail) {
IO {
IO.sleep(2.millis)(munitTimer)
.flatMap { _ =>
IO {
IO.sleep(2.millis)(munitTimer)
.flatMap { _ =>
IO {
IO.sleep(2.millis)(munitTimer)
.map(_ => assertEquals(false, true))
}
}
}
}
}
}
test("nested success") {
IO {
IO.sleep(2.millis)(munitTimer)
.flatMap { _ =>
IO {
IO.sleep(2.millis)(munitTimer)
.flatMap { _ =>
IO {
IO.sleep(2.millis)(munitTimer)
.map(_ => assertEquals(true, true))
}
}
}
}
}
}
}