Pull up laws #69
Pull up laws #69
Conversation
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
==========================================
+ Coverage 84.59% 84.91% +0.32%
==========================================
Files 18 18
Lines 370 358 -12
Branches 22 20 -2
==========================================
- Hits 313 304 -9
+ Misses 57 54 -3 |
|
Can one of the maintainers review this? @alexandru @mpilquist @pchlupacek @rossabaker @tpolecat |
| "repeated sync evaluation not memoized" -> forAll(laws.repeatedSyncEvaluationNotMemoized[A] _), | ||
| "propagate errors through bind (suspend)" -> forAll(laws.propagateErrorsThroughBindSuspend[A] _)) | ||
|
|
||
| val jvmProps = Seq( |
There was a problem hiding this comment.
Running stack safety tests on top of Javascript is still valuable and what I'd do here is to use a different count, e.g. 1000 instead of 10000 binds.
This is because we might end up with projects implementing Sync that don't care about the JVM at all.
There was a problem hiding this comment.
What would be the best way to empirically determine the appropriate count?
There was a problem hiding this comment.
I have no idea; basically you pick a value, then see if the tests run in a reasonable amount of time for cats.effect.IO.
There was a problem hiding this comment.
I would set it to 1000. It's better than nothing and should be manageable on top of Node.js
There was a problem hiding this comment.
In the interest of not stalling this on the petard of my lack of time, I'm going to merge this PR and open a new issue for reenabling the stack safety tests on JS with lower limits.
There was a problem hiding this comment.
The threshold for stack-safety is in my experience closer to 70000 function compositions than 10000, let alone 1000. That's what every cats stack-safety test uses. Does this test fail as is with a stack-unsafe monad?
There was a problem hiding this comment.
I've seen tests fail for a stack depth of 1000, because it's usually multiplied by whatever else you're doing for each of those frames.
But indeed the number is chosen arbitrarily.
We should do a quick / simple Thunk-based implementation and measure with that.
There was a problem hiding this comment.
@edmundnoble It does fail as it is with a stack-unsafe monad. I think it's just because there are more stack frames floating around. Or maybe it's just because my machine is allocating less stack space. Either way, it did seem to be pretty consistent when I was playing with it.
There was a problem hiding this comment.
Alright nice, thanks :) I bet we could cut it down for cats too.
Fixes #52
Moves stack-safety laws up into
SyncLaws, as well as some of the error propagation laws. Also adds a law which stipulates that errors in therunAsynchandler effect must be discarded (previously this was unspecified).I had to remove the
Async[Kleisli[F, R, ?]]instance with the laws moving up, becauseKleisliis stack-unsafe on left-associated binds (see: typelevel/cats#1733). This is a shame, but not entirely unexpected.