Skip to content
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

Allow loading GlobalResources from libraries on the classpath. #494

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −2
Open

Allow loading GlobalResources from libraries on the classpath. #494

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions modules/framework/src-jvm/RunnerCompat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import sbt.testing.{ Task, TaskDef }
import CECompat.Ref
import CECompat.Semaphore

import java.util.ServiceLoader
import scala.collection.mutable.ListBuffer

trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>

protected val suiteLoader: SuiteLoader[F]
Expand Down Expand Up @@ -106,9 +109,9 @@ trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>
makeTasks(taskDef, mkSuite)
}.unzip

val globalResources = tasksAndSuites.collect {
val globalResources = (tasksAndSuites.collect {
case (_, suiteLoader.GlobalResourcesRef(init)) => init
}.toList
}.toSet ++ spiGlobalResources).toList

// Passing a promise to the FP side that needs to be fulfilled
// when the global resources have been allocated.
Expand Down Expand Up @@ -184,6 +187,13 @@ trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>
globalResources.traverse(_.sharedResources(map)).void
}

private def spiGlobalResources: List[GlobalResourceF[F]] = {
val result = ListBuffer.empty[GlobalResourceF[F]]
val loader: ServiceLoader[GlobalResourceF[F]] = ServiceLoader.load(classOf[GlobalResourceF[F]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @aevanszen, thanks for the PR.

I gave it some long thought, and the problem with doing it this way is type-erasure. Even if it's unlikely that people would use different effect types in a single codebase, I'd be keen to protect against it. Because GlobalResourceF is parameterised, the load call would return everything, whether it's using cats-effect, monix or zio, and obviously loading monix globals in a CE context wouldn't be great.

So what I think we should do instead, would be for the ServiceLoader call to be abstract away. We have a SuiteLoader abstraction, and we could add a def loadGlobalResources to it.

The implementation of SuiteLoader lives there, at a place that is aware of the classes that the test-framework is supposed to understand. Those are effect-type specific (there is one set defined for CE, one for monix, one for zio etc).

Now the thing is that you'll need one other layer of indirection to support cross-compilation. IE you could have a PlatformServiceLoader object defined in a src-jvm, containing a spiGlobalResources[T](cl : ClassLoader, cls: Class[T]) : List[T] method (similar to what you've implemented here), and a similar object defined in src-js , that'd have a method with the same signature but would return an empty list.

This way, type-erasure would be accounted for.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the detailed reply @Baccata, the suggestions are very helpful.

I had the same reservations around type-erasure but wasn't sure the best way to work around it in the framework just yet. Your suggestion is a good starting point, and I'll look into it.

I'll add the tests and documentation when the implementation is ok and update the PR with the changes.

The GlobalResoure classpath isn't an urgent blocker for me, so updating this PR may be slow as I work on it when I get the spare time. I will try to work on it, though, as it'll be useful to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem whatsoever, I certainly appreciate the contribution, even if it takes time. There is no rush :) let us know if you get stuck or if you want us to have a look at some commits

loader.iterator().forEachRemaining(result += _)
result.toList
}

private case class IOTask(
fqn: String,
mkSuite: MakeSuite,
Expand Down