Add cats-effect support for FunSuite and FunFixtures #134

milanvdm · 2020-05-25T07:05:51Z

Goal

I tried to keep the changes as minimal as possible and reuse as much as possible of the existing code

Remarks

I am not a cats-effect expert and I created this PR with my own use-case in mind. Would love if someone could have a second look and see if this PR makes sense

Still to do

Create a real module structure and a separate artifact
Check cross-compilation and tests

olafurpg

Thank you for this contribution! Just a few quick high-level comments. I'm not at all familiar with cats-effect so it would be great if somebody else could review those bits

olafurpg · 2020-05-25T10:04:15Z

build.sbt

+  ),
+  skip in publish := true
+)
+lazy val tests = crossProject(JSPlatform, JVMPlatform)


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

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.

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?

olafurpg · 2020-05-25T10:31:27Z

munit-cats-effect/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

+
+import scala.concurrent.Promise
+
+trait CatsEffectFunFixtures extends FunFixtures { self: CatsEffectSuite =>


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.

olafurpg · 2020-05-25T10:33:23Z

munit-cats-effect/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

+
+      FunFixture.async(
+        setup = { testOptions =>
+          implicit val ec = munitExecutionContext


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.

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

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

@SystemFw Could you please open an issue here? https://github.com/typelevel/munit-cats-effect/

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 :)

OK, disregard my comment then :)

Kazark

I am certainly hungry for this.

Kazark · 2020-07-15T16:22:47Z

munit-cats-effect/shared/src/main/scala/munit/CatsEffectSuite.scala

+
+import cats.effect.{ContextShift, IO, Timer}
+
+abstract class CatsEffectSuite extends FunSuite {


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

mpilquist · 2020-07-15T17:39:40Z

@milanvdm FYI, I'm using the ValueTransform integration from this PR in FS2 now. I added another for SyncIO integration. https://github.com/functional-streams-for-scala/fs2/blob/main/core/shared/src/test/scala/fs2/Fs2Suite.scala

milanvdm · 2020-08-04T10:10:40Z

@mpilquist Are you fine if I add your SyncIO to this PR as well? :)

mpilquist · 2020-08-04T10:22:28Z

Go for it :)

larsrh · 2020-08-04T21:09:02Z

While I endorse the goal of this PR, I would like to strongly suggest to not add this code as a module to this repository; instead keeping it as a separate project. The reason for this is twofold:

The library ecosystem should stay stratified; that is, foundational libraries such as test frameworks should strive to have as few dependencies as possible.
Concretely, since Preliminary Munit port typelevel/cats#3538 landed, cats has a dependency on munit. This means that when this PR is merged, we now have a cyclic dependency (munit → cats-effect → cats → munit). Such a test framework/library cycle has happened in the past and was resolved by the test framework (specs2) shipping its own copy of the library which I think should be avoided at all costs.

Again, I (and others) believe providing this integration is very worthwhile. To quote a cats-effect maintainer:

I would dearly love these test frameworks to have first-class support for CE

I'm happy to provide any kind of resource or support you need for this integration to happen, as long as it lives in a separate repository with a separate release lifecycle.

milanvdm · 2020-08-05T16:33:30Z

@larsrh Do you want to add this repository to the scalameta/typelevel/... org specifically?

Not sure if I completely understand the reasoning behind having this in a separate repository vs a separate module in munit itself. It feels to me like the arguments you mention are also solved by having it in a separate module but I could be missing something.

One reason to keep it in munit would be to make sure the cats-effect module would always be up-to-date with the latest munit changes.

In the end, I don't care too much how this is organized tbh :) As long as it happens ¯_(ツ)_/¯

Concretely, next steps:

Create a new repo in some org/user
I'll have to check how to structure what you suggest (if someone else doesn't do it before me) as I have very little experience with open-source projects/releases (but would love to get that experience)
Write some code that may or may not do things

larsrh · 2020-08-06T06:26:13Z

Do you want to add this repository to the scalameta/typelevel/... org specifically?

I can offer to host it on typelevel, if you like.

Not sure if I completely understand the reasoning behind having this in a separate repository vs a separate module in munit itself. It feels to me like the arguments you mention are also solved by having it in a separate module but I could be missing something.

In principle the separate module breaks the dependency cycle. But since all the modules in a repository are typically released together, this is still a problem. The release workflow would look like this:

release munit-core
release cats
release cats-effect
release munit-cats-effect

Concretely, next steps:

Create a new repo in some org/user

I'll have to check how to structure what you suggest (if someone else doesn't do it before me) as I have very little experience with open-source projects/releases (but would love to get that experience)

Write some code that may or may not do things

Happy to help with all of that, just let me know. I suggest typelevel/munit-cats-effect.

milanvdm · 2020-08-06T10:16:15Z

@larsrh Thanks a lot for the information :)

A typelevel repo feels the most logical place to add this library.

larsrh · 2020-08-06T16:58:17Z

Excellent! I've created a repository, sent you an invite, and created a ticket for me to set up the project boilerplate (typelevel/munit-cats-effect#1). I'll ping you once that's complete.

olafurpg · 2020-08-08T07:15:12Z

@larsrh thank you for pitching in! I sounds like a good idea to maintain the cats-effect integration in a separate repo 👍

milanvdm · 2020-08-08T09:38:32Z

Closing in favor of https://github.com/typelevel/munit-cats-effect

milanvdm added 4 commits May 24, 2020 10:46

Add new CatsEffect module

5172350

Add CatsEffectSuite

d5b964d

Add CatsEffectFunFictures

c82b6ea

Add Timer

3159810

olafurpg reviewed May 25, 2020

View reviewed changes

Kazark approved these changes Jul 15, 2020

View reviewed changes

milanvdm requested a review from olafurpg August 4, 2020 10:03

milanvdm marked this pull request as ready for review August 4, 2020 10:04

milanvdm closed this Aug 8, 2020

milanvdm mentioned this pull request Aug 9, 2020

Add munit cats-effect integration typelevel/munit-cats-effect#2

Merged

SystemFw mentioned this pull request Jan 19, 2021

Don't use parasitic EC in ce2 typelevel/munit-cats-effect#65

Merged

danicheg mentioned this pull request Mar 5, 2022

Highlight the munitExecutionContext parasitic peculiarity in docs #500

Open

armanbilge mentioned this pull request Jun 3, 2022

Don't use default EC for scalatest typelevel/cats-effect-testing#318

Closed

armanbilge mentioned this pull request Jun 22, 2022

Upgrade to MUnit 1.0.0-M6 typelevel/munit-cats-effect#223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cats-effect support for FunSuite and FunFixtures #134

Add cats-effect support for FunSuite and FunFixtures #134

milanvdm commented May 25, 2020 •

edited

Loading

olafurpg left a comment

olafurpg May 25, 2020

olafurpg May 25, 2020

milanvdm May 27, 2020

olafurpg May 25, 2020

olafurpg May 25, 2020

milanvdm May 27, 2020

SystemFw Jan 19, 2021 •

edited

Loading

larsrh Jan 19, 2021

SystemFw Jan 19, 2021

larsrh Jan 19, 2021

Kazark left a comment

Kazark Jul 15, 2020

milanvdm Jul 15, 2020

mpilquist commented Jul 15, 2020

milanvdm commented Aug 4, 2020

mpilquist commented Aug 4, 2020

larsrh commented Aug 4, 2020

milanvdm commented Aug 5, 2020

larsrh commented Aug 6, 2020

milanvdm commented Aug 6, 2020

larsrh commented Aug 6, 2020

olafurpg commented Aug 8, 2020

milanvdm commented Aug 8, 2020


		import scala.concurrent.Promise

		trait CatsEffectFunFixtures extends FunFixtures { self: CatsEffectSuite =>


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

		abstract class CatsEffectSuite extends FunSuite {

Add cats-effect support for FunSuite and FunFixtures #134

Add cats-effect support for FunSuite and FunFixtures #134

Conversation

milanvdm commented May 25, 2020 • edited Loading

Goal

Remarks

Still to do

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SystemFw Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kazark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpilquist commented Jul 15, 2020

milanvdm commented Aug 4, 2020

mpilquist commented Aug 4, 2020

larsrh commented Aug 4, 2020

milanvdm commented Aug 5, 2020

larsrh commented Aug 6, 2020

milanvdm commented Aug 6, 2020

larsrh commented Aug 6, 2020

olafurpg commented Aug 8, 2020

milanvdm commented Aug 8, 2020

milanvdm commented May 25, 2020 •

edited

Loading

SystemFw Jan 19, 2021 •

edited

Loading