cats-effect-testing icon indicating copy to clipboard operation
cats-effect-testing copied to clipboard

Make ScalaTest's CatsResource beforeAll and afterAll methods protected

Open bpholt opened this issue 10 months ago • 3 comments

This syncs them with the original definition and avoids compiler errors when other things that use BeforeAndAfterAll are mixed in.

[error] file.scala:31:7: weaker access privileges in overriding
[error] override def afterAll(): Unit (defined in trait CatsResource)
[error]   override should be public

I'm not sure if this has MiMa implications; when I ran it locally I did get a couple errors, but I didn't see them in the builds in our GHAs. We'll see what happens in this PR.

bpholt avatar Feb 14 '25 22:02 bpholt

Unfortunately the bincompat check failed:

* abstract synthetic method cats$effect$testing$scalatest$CatsResource$$super$beforeAll()Unit 
  in interface cats.effect.testing.scalatest.CatsResource is present only in current version
* abstract synthetic method cats$effect$testing$scalatest$CatsResource$$super$afterAll()Unit 
  in interface cats.effect.testing.scalatest.CatsResource is present only in current version

I still think these changes fix a problem and should still be merged, but that would lead to a v2.0 for the ScalaTest library, at least, which is unfortunate enough, but with the current structure it would also result in a major version bump for the other test frameworks too. @armanbilge do you have any suggestions?

bpholt avatar Feb 18 '25 18:02 bpholt

Maybe it's not worth making this change now, since it would require a major version bump and possibly splitting out the ScalaTest library into a separately versioned repo. I was able to work around the problem by including

override def afterAll(): Unit = super.afterAll()

in the tests that mix in another trait that also overrides afterAll (but as a protected method).

In my case, that ends up being in an intermediate trait that sets some other things up too, so it's not too overwhelming. It would be more frustrating if I had to add it to dozens (or more) of test classes.

bpholt avatar Feb 21 '25 20:02 bpholt

Actually, maybe adding a new trait to do that would be a reasonable compromise. e.g. adding

trait MixInPublicBeforeAndAfterAll extends org.scalatest.BeforeAndAfterAll { this: org.scalatest.Suite =>
  override def afterAll(): Unit = super.afterAll()
}

with a note for the future indicating that should be removed in favor of what I proposed above if eventually there's a major version bump.

bpholt avatar Feb 21 '25 20:02 bpholt