Make ScalaTest's CatsResource beforeAll and afterAll methods protected

[bpholt]

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]

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]

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]

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.

[armanbilge]

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

[bpholt]

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.

[bpholt]

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?