typelevel / weaver-test

A test framework that runs everything in parallel.
https://typelevel.org/weaver-test/
Other
46 stars 8 forks source link

Add explicit unique ResourceTag-like keys #107

Open zainab-ali opened 5 days ago

zainab-ali commented 5 days ago

This issue was copied over from: https://github.com/disneystreaming/weaver-test/issues/266 It was opened by: nigredo-tori


This is inspired by the vault library.

At the moment global resources are indexed by ResourceTags and labels. This has its issues:

  1. The default ResourceTag implementation derives its identity from ClassTag. This leads to a possibility of collisions, as stated in the documentation. The suggested workarounds are monomorphic wrappers or custom ResourceTag instances.
  2. Labels mitigate the collision problem somewhat (or, rather, leave it up to discipline). But the labels are not tied to resource types, so it's easy to mix up which labels correspond to which resource.

Instead, we can create objects that are guaranteed to be unique, and which are associated with a specific type. Consider something like this:

// default `equals` - different objects are never equal.
final private class UniqueResourceTag[A](
  val description: String
) extends ResourceTag[A] {
  def cast(obj: Any): Option[A] = Some(obj.asInstanceOf[A])
}
def unsafeCreateUniqueTag[A](description: String): ResourceTag[A] =
  new UniqueResourceTag[A](description)

This can be used as follows:

// A "constant" somewhere in a common object
val fooTag = unsafeCreateUniqueTag[Foo]("")
// Inside a suite
global.getR()(fooTag)

The current API is obviously not perfect for this, but the general idea should be clear enough. Note that there's no way for fooTag to collide with anything else (assuming no broken equals implentations), and there's no way to mistake the type of the resource it points to.

zainab-ali commented 5 days ago

This comment was copied over from: https://github.com/disneystreaming/weaver-test/issues/266#issuecomment-823089644 It was written by: Baccata


Hello, and thank you for raising an issue

But the labels are not tied to resource types

As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

This leads to a possibility of collisions, as stated in the documentation.

Yes, and for this very reason we are protecting against it via forcing implicit ambiguity when collisions are possible (ie compile error rather than surprising runtime behaviour), and we've made the conscious decision to leave the API flexible enough so that users can bring in their own patterns (exactly like what you suggest if they want to use that).

The reason why I don't want to support your suggestion out of the box is that, unlike vault's examples, it's impossible to pass the values around logically, from the creation of a key, to its use. So such instances have to be instantiated globally, and for it to work, they have to be assigned to a val and not a def (otherwise you get a new instance on every callsite and referential equality can't kick-in). So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

So I'm more inclined to let this problem be solved by users when they encounter it, than introducing a new construct which is inherently risky without macros, and I don't really want to have to maintain more macros than what we already have. The existing solution might not be perfect, but at least mis-using classtag-based resourceTags lead to compile errors.

However, I'd be willing make a version of global.getR (and its putR dual) that'd take the tag explicitly rather than implicitly, so that solving this in userland wouldn't look as weird.

zainab-ali commented 5 days ago

This comment was copied over from: https://github.com/disneystreaming/weaver-test/issues/266#issuecomment-823124072 It was written by: nigredo-tori


As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

I guess I didn't phrase it right. Let's say I have putR[Foo](foo, "something".some) as my global setup, and getOrFailR[Foo]("something".some) in my test suites. Now let's say I have decided that instead of Foo I want to share a Bar. I change the first part to putR[Bar](bar, "something".some). Now all the getOrFailR parts compile fine, but fail at runtime!

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

Fair point. I can't say for sure whether guarding this with macros would be warranted, but if you think it would be, then this becomes a non-trivial undertaking. Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library. :smile:

zainab-ali commented 5 days ago

This comment was copied over from: https://github.com/disneystreaming/weaver-test/issues/266#issuecomment-823132087 It was written by: Baccata


Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library.

👍 that sounds like a good work environment 😄

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So would a method that takes tags explicitly be a satisfying middle-ground ?

zainab-ali commented 5 days ago

This comment was copied over from: https://github.com/disneystreaming/weaver-test/issues/266#issuecomment-823140636 It was written by: nigredo-tori


So would a method that takes tags explicitly be a satisfying middle-ground ?

Yes.