Make ScalaTest's CatsResource beforeAll and afterAll methods protected
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.
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?
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.
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.