use dynamic variable to control serialization tests

[sritchie]

Here's a much simpler (in terms of required implementation changes and ease of use) solution to #123. To skip serialization for a block of tests, just pass a thunk to withoutSerialization:

withoutSerialization {
   laws[OrderLaws, Boolean].check(_.order)
}

I think this should solve our immediate issue of making the tests reusable for non-serializable instances.

[sritchie]

@ceedubs, @johnynek - any thoughts?

[johnynek]

This solution looks like the best way to go forward to me.

[sritchie]

@johnynek Boom, done.

[ceedubs]

It makes me cringe a bit that we solved a problem of verbosity with global mutable state :). I can't think of a practical objection as long as this is scala.js-friendly though - is DynamicVariable okay to use in that context?

[ceedubs]

Hmm actually couldn't this be an issue when testing an instance for something like Future where the law-checking might start on one thread and end on another? Edit - hmm maybe it's not. I'm not familiar with DynamicVariable.

[sritchie]

@ceedubs I think the state applies in the context created inside withoutSerialization; so any thread spawned inside that grabs that state of the variable.

[sritchie]

@ceedubs pretty sure it's fine with scala.js. The tests all pass, anyway. And it's not really "global mutable state" unless someone toggles runTests, which is an abuse of the interface since we've marked the object as private.

I share your general cringe, but the alternative is too horrifying. Just pretend it's an implicit parameter that every function gets :)

[ceedubs]

@sritchie okay and Sébastien even confirms DynamicVariable isn't a problem for scala.js.

I'm not crazy about this approach, but it seems to get the job done with minimal impact. Thanks for all of your work on this! :+1: (note: I'm not an algebra committer so this isn't an official thumbsup)

[sritchie]

Sounds great! Let me know if you guys need any changes, or feel free to merge away. On Wed, Jan 6, 2016 at 2:03 PM Cody Allen notifications@github.com wrote:

@sritchie https://github.com/sritchie okay and Sébastien even confirms https://groups.google.com/d/msg/scala-js/LYj4FgZ_ACQ/p7K7vi8dM9wJ DynamicVariable isn't a problem for scala.js.

I'm not crazy about this approach, but it seems to get the job done with minimal impact. Thanks for all of your work on this! [image: :+1:](note: I'm not an algebra committer so this isn't an official thumbsup)

— Reply to this email directly or view it on GitHub https://github.com/non/algebra/pull/141#issuecomment-169421972.

[non]

After a four month hiatus I think this is basically fine. 👍

[johnynek]

I still think this is the best solution we have to give a workaround for non-serializable types, but also check by default all the typeclasses being serializable.