-
Notifications
You must be signed in to change notification settings - Fork 359
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 doobie-munit package to integrate Doobie with the shiny new MUnit #1233
Conversation
MUnit is a the new Scala testing framework on the block. Seems reasonable for Doobie to support it along with the other two it already supports. I copied off `scalatest` because it is fairly similar to MUnit. Hopefully I didn't forget to change anything after the copy and paste, particularly in the documentation (the test is green, at least).
@@ -99,7 +99,7 @@ def biggerThan2(minPop: Int) = | |||
biggerThan2(0).check.unsafeRunSync | |||
``` | |||
|
|||
**doobie** supports `check` for queries and updates in three ways: programmatically, via YOLO mode in the REPL, and via the `doobie-specs2` and `doobie-scalatest` packages, which allow checking to become part of your unit test suite. We will investigate this in the chapter on testing. | |||
**doobie** supports `check` for queries and updates in four ways: programmatically, via YOLO mode in the REPL, and via the `doobie-specs2`, `doobie-scalatest` and `doobie-munit` packages, which allow checking to become part of your unit test suite. We will investigate this in the chapter on testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take or leave it: might a bulleted list be easier to view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a reasonable idea; the general principle of the PR was to try to stick closely with what's there. We'll see what @tpolecat says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idm what's currently here, since it's featured prominantly in 13-Unit-Testing.md :)
```scala mdoc:silent | ||
import munit._ | ||
|
||
class AnalysisTestScalaCheck extends FunSuite with doobie.munit.IOChecker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does FunSuite
come from munit, @Kazark ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
This looks fine, sorry for the delay. I'll merge it if you bring it up to date. |
Thanks @alejandrohdezma |
@tpolecat done! I've included an update of the |
Hey @tpolecat, I have conform |
So when can we expect this one released? |
There are a few thing left that we need to release 0.11 so I've asked Rob to look at a few important PRs (including this one). No ETA sorry. |
val report = analyzeIO(args, transactor).unsafeRunSync() | ||
if (!report.succeeded) { | ||
fail( | ||
formatReport(args, report, colors) | ||
.padLeft(" ") | ||
.toString | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't levering https://github.com/typelevel/munit-cats-effect be a better choice?
Something like.
val report = analyzeIO(args, transactor).unsafeRunSync() | |
if (!report.succeeded) { | |
fail( | |
formatReport(args, report, colors) | |
.padLeft(" ") | |
.toString | |
) | |
} | |
val report = analyzeIO(args, transactor) | |
report.flatMap{ | |
case r if !r.succeeded => | |
IO(fail( | |
formatReport(args, report, colors) | |
.padLeft(" ") | |
.toString | |
)) | |
case _ => IO.unit | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented the munit-cats-effect integration at
https://github.com/ValdemarGr/doobie/tree/munit
Can someone give any insight or consideration for the idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea. It'll also make CE3 migration easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation also changes the semantics of check; it is no longer eager, is this desired?
The scalatest and specs2 test utilities seem to be eager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If execution thread gets to check(..)
then we're running the test already right? (This PR's impl seems eager to me, unless I'm missing something)
munit (and all other test frameworks that I know) all use call-by-name to allow skipping tests, so test("asdf") { check(..) }
is still skippable even if check
is eager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the evaluation of the check effect.
In the initially proposed solution check runs eagerly which fails the following test:
https://github.com/ValdemarGr/doobie/blob/6ad3f9414bc7cb6f1a1bbe1e65789315bc38dd08/modules/munit/src/test/scala/doobie/munit/CheckerTests.scala#L31-L34
In the version I published the test combinator is wrapped here (I think):
https://github.com/typelevel/munit-cats-effect/blob/65604236d2a8c18bf077b5c1269ff02ce54027b7/ce3/shared/src/main/scala/munit/CatsEffectSuite.scala#L38
Thus the test succeeds in the munit-cats-effect variant:
https://github.com/ValdemarGr/doobie/blob/ebbd0ccc475bf21dd5ebee4967741c4ff0263992/modules/munit/src/test/scala/doobie/munit/CheckerTests.scala#L33-L36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I think the other integrations (scalatest/specs) run it eagerly. Perhaps it is worth keeping that behaviour to make it easier for people to migrate between test libs. (I know that in my codebase I have a few check(..)
being called as if they're statements)
So I guess cats-effect munit won't help us here. Sorry!
at $WORK we would really like to leave scalatest in the dust, so this would help a lot. Can we get this going again? |
Sorry just saw that I needed to approve the CI. |
Thanks for the PR and sorry for the delay! I'll get this merged into the cats-effect 3 (1.x) branch too. |
MUnit is a the new Scala testing framework on the block. Seems reasonable for Doobie to support it along with the other two it already supports.
I copied off
scalatest
because it is fairly similar to MUnit. Hopefully I didn't forget to change anything after the copy and paste, particularly in the documentation (the test is green, at least).