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 munit cats-effect integration #2

Merged
merged 14 commits into from
Aug 9, 2020

Conversation

milanvdm
Copy link
Member

@milanvdm milanvdm commented Aug 9, 2020

Moved and improved the logic from scalameta/munit#134

Let me know what you think :)

@milanvdm milanvdm requested review from mpilquist and larsrh August 9, 2020 13:41
Copy link
Member

@mpilquist mpilquist left a comment

Choose a reason for hiding this comment

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

Awesome! I left a couple very minor comments. Feel free to merge as-is though.

): FunFixture[T] =
fromResource(
resource,
(_, _) => IO.pure(()),
Copy link
Member

Choose a reason for hiding this comment

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

Note you can do IO.unit instead of IO.pure(())

abstract class CatsEffectSuite extends FunSuite {

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

Choose a reason for hiding this comment

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

munitExecutionContext is parasitic -- I've seen issues when running async tests as a result (the parZip tests in FS2 specifically, where the parasitic nature breaks concurrency, which the test relies on).

I guess the solution can be telling folks to override munitExecutionContext if they need true concurrency.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be a good comment/remark to add to the documentation for sure!

@larsrh
Copy link
Contributor

larsrh commented Aug 9, 2020

I have nothing to add since I know very little about fs2 😉

@mpilquist mpilquist merged commit 895b434 into main Aug 9, 2020
@milanvdm milanvdm deleted the add-munit-cats-effect-integration branch August 9, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants