add implicit ADT for skipping serialization tests #129
Conversation
| case object SkipSerialization extends SerializationCheck | ||
|
|
||
| object SerializationCheck { | ||
| implicit val check: SerializationCheck = SkipSerialization |
There was a problem hiding this comment.
I don't think we want to not check by default.
There was a problem hiding this comment.
yeah, my bad. Pushed early.
sritchie
force-pushed
the
feature/skip_serialization
branch
from
ae8e75c
to
62f787c
Compare
sritchie
force-pushed
the
feature/skip_serialization
branch
from
62f787c
to
35a69f4
Compare
|
@johnynek changed the name and added the type parameter. Do we still want to thread implicit params through all of the test methods if we're only supporting an actual definition of |
| "serializable" -> (implicitly[IsSerializable[M]] match { | ||
| case Is() => | ||
| Prop(_ => Result(status = Proof)) | ||
| case IsNot() => |
There was a problem hiding this comment.
Shouldn't we only be checking the serializability if it Is serializable instead of if it IsNot? :)
There was a problem hiding this comment.
Ugh, sorry about that. Second time I goofed those cases. Fixed.
| def serializable[M](m: M): (String, Prop) = "serializable" -> Prop { _ => | ||
| Result(status = Proof) | ||
| } | ||
| def serializable[M: IsSerializable](m: M): (String, Prop) = |
There was a problem hiding this comment.
This seems a little bit confusing to me, because it looks like M must be serializable in order to call this, when in fact the IsSerializable instance could be IsNot. I'm at a loss for names that seem any better though.
|
I'm not sure I quite understand #129 (comment). I wouldn't think that this approach would require the instance to be defined in the companion object. Would it? In most cases I don't think we'll have the instance in the companion object because 1) Future is the instance where we've wanted this and 2) tests are probably more likely to rely on the laws module than the main code where classes are defined. |
|
I think we have to thread the implicit all the way through to take this approach (even if there is an I haven't really looked at the hierarchy to see if that will be a disaster. I agree now with @ceedubs even though I suggested the name. The problem is this is purely type-level function. We aren't going to do anything with it except skip a test at runtime. One way out of the naming thing would be to use I would suggest, to see if this approach works, we set aside the naming question and see how clean it is to plumb it all the way through and have a test for a non-serializable class (maybe something that explicitly throws if it is serialized). |
|
@johnynek I think that's a good suggestion. And I now understand why you were saying it would need to be threaded all the way through. I had forgotten that other tests call the serializability test, but that's of course why this is needed in the first place :) |
|
|
||
| def additiveCommutativeSemigroup(implicit A: AdditiveCommutativeSemigroup[A], | ||
| check: IsSerializable[CommutativeGroup[A]], | ||
| addCheck: IsSerializable[AdditiveCommutativeGroup[A]]) = |
There was a problem hiding this comment.
lots of required implicits.
|
@johnynek, @ceedubs - take a look and see if you guys can stomach the required changes in Also - it looks like to reuse these tests, users are going to extend |
| @@ -0,0 +1,17 @@ | |||
| package algebra.laws | |||
|
|
|||
| sealed trait IsSerializable[+T] | |||
There was a problem hiding this comment.
Should this be covariant? I don't think so. For instance, if Int has a given IsSerialializable instance, I can't in general use that same thing for Any.
On the other hand, if I want to test a given type M1 which I know is a subclass of M2, then if I have a Gen[M1] that I use for all the tests for M2 (note Gen is covariant), then in that case the IsSerializable instance will work to "upcast". I don't immediately see how to formalize that in the type system. Maybe something like:
def upcast[M2, M1 <: M2](g: Gen[M1], isser: IsSerializable[M1]): (Gen[M2], IsSerializable[M2]) =
(g, isser.asInstanceOf[IsSerializable[M2]])But that is only safe if you use the IsSerializable instance with the Gen. It doesn't really glue them together.
There was a problem hiding this comment.
Agreed that this shouldn't be covariant.
|
I really appreciate you running this idea down, @sritchie . @ceedubs What do you think? do you have any other ideas? I guess a default parameter that is threaded through is the other idea. Is there any fancy scalacheck trick that we could change an argument within a scope at the scalacheck level so we don't have to litter the code with threading all the way through (that may be ugly in its own way). |
|
Annoying that we can't just add a method at the bottom of the trait heirarchy (inside If we do that, will any trait that extends the base and overrides that method also pass its new default implicit on to calls to Then users can just make a new class with an overridden def for types that can't support serialization. Happy to try it. (My mental ability to anticipate implicit scoping's still rusty :) |
|
Ah, I guess not, since |
|
Let's close this one up for now. The changes required for this approach are insane. Let me know what you think of the simpler approach here: |
First stab at #123.