Skip to content

Make ScalaTest's CatsResource beforeAll and afterAll methods protected #632

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: series/1.x
Choose a base branch
from
Open

Make ScalaTest's CatsResource beforeAll and afterAll methods protected #632

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26f8c1a
Make ScalaTest's CatsResource beforeAll and afterAll methods protecte…
bpholt Feb 14, 2025
5fa0322
call super.beforeAll and super.afterAll to support other mixed-in tes…
bpholt Feb 14, 2025
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
11 changes: 7 additions & 4 deletions scalatest/shared/src/main/scala/cats/effect/testing/scalatest/CatsResource.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ trait CatsResource[F[_], A] extends BeforeAndAfterAll { this: FixtureAsyncTestSu
@volatile
private var shutdown: F[Unit] = ().pure[F]

override def beforeAll(): Unit = {
override protected def beforeAll(): Unit = {
super.beforeAll()
Copy link
Member

Choose a reason for hiding this comment

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

Do we definitely need these super calls? Is this maybe why we get the MiMa issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

The super calls are why the MiMa failures exist, yes. I'm guess I'm not sure if they're required; I can play around with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think as long as the traits are mixed in in the right order, they're unneeded, but it's kind of a silent trap at the moment. (Luckily ours mixes something in after CatsResource that does call super.afterAll(), so it seems to work. If I reverse the ordering, though, it does not do the right thing.)

Maybe the weaker access privileges in overriding error is sort of a feature, then… it at least warns the user that something is amiss? We could add a // TODO comment on those methods asking a future dev to make these changes if there's ever a major version bump for another reason.

We're testing during an interim phase where we have an Akka testkit and Cats Effect resources in the same tests, which is how this comes up. Eventually the Akka testkit will be phased out and we'll just be using CatsResource. I'm hesitant to push for a major version bump to fix a potential footgun in what's probably a fairly esoteric use of the library, and maybe we shouldn't even fix the overridden access privileges either?


val toRun = for {
d <- Deferred[F, Unit]
_ <- Sync[F] delay {
Expand All @@ -68,16 +70,17 @@ trait CatsResource[F[_], A] extends BeforeAndAfterAll { this: FixtureAsyncTestSu
_ <- d.complete(())
} yield ()

UnsafeRun[F].unsafeToFuture(toRun, finiteResourceTimeout)
()
val _ = UnsafeRun[F].unsafeToFuture(toRun, finiteResourceTimeout)
}

override def afterAll(): Unit = {
override protected def afterAll(): Unit = {
UnsafeRun[F].unsafeToFuture(shutdown, finiteResourceTimeout)

gate = None
value = None
shutdown = ().pure[F]

super.afterAll()
}

override type FixtureParam = A
Expand Down
Loading