-
Notifications
You must be signed in to change notification settings - Fork 49
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.
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
base: master
Are you sure you want to change the base?
Allow loading GlobalResources from libraries on the classpath. #494
Conversation
This is useful if you want to package GlobalResource up as a subproject and re-use in a multi-module build.
|
@@ -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]]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I'd also appreciate :
|
As discussed in #479.
This is a minimal implmentation to allow GlobalResources to be reused in a submodule / external jar across projects.
I've tested on a local project and it looks to work as expected.