fix wrong match in shrinker (fixes #909) #910
Conversation
AurelienRichez
force-pushed
the
fix/fix-909
branch
from
e743e27 to
70a1526
Compare
AurelienRichez
force-pushed
the
fix/fix-909
branch
from
70a1526 to
2ffd200
Compare
| @@ -269,6 +269,30 @@ object PropSpecification extends Properties("Prop") { | |||
| }) | |||
| } | |||
|
|
|||
| property("shrinking process always take the first value returned by the Shrink[T]") = { | |||
There was a problem hiding this comment.
This test reproduces the issue.
With the Shrink provided, normally the shrinking process will go like 12345678 -> 1234 -> 12 -> 1.
With my previous PR, it checks something like12345678 -> 1234 -> 5678, gets to the end of the stream provided by the custom Shrink and then finally returns 12345678. So it's not really the test hanging, but it shows a change in the way the value is shrinked.
Modifying the stream to be infinite will create a test that never ends. I think that even with finite stream you can see some symptoms as every test will run through the entire stream provided by Shrink instead of choosing the first value.
| val originalException = Some(r.status).collect { case NonFatal(e) => e.getClass() } | ||
| val originalException = Some(r.status).collect { case Exception(e) => e.getClass } |
There was a problem hiding this comment.
I stared at this yesterday and today and pulled up the PR on my local and added printlns before I finally understood why such an innocent looking change makes a huge difference 😂
This is very confusing. The Exception here is NOT in the Throwable hierarchy. That's why NonFatal(_) was effectively useless.
Oddly enough I don't think scalac warns for this?
There was a problem hiding this comment.
Perhaps if you write it as Prop.Exception that will help with readability? Perhaps the reviewer in #838 (comment) was confused as well 😝
Also I now realize you explained exactly this in #838 (comment) but I obviously didn't understand 😂 sorry about that!
There was a problem hiding this comment.
I think we were all confused by the name 😝. Good point about using Prop.Exception. It will make clear we're not talking about the usual Exception. I'll update it (it's not the only match on Prop.Exception in the file though).
I think scalac infer { case NonFatal(e) => e.getClass() } as a PartialFunction[Any, Class[_ <: Throwable]] since that's what makes it compile, so it does not give a warning :-( . Intellij points to the error though. Unfortunately I was not using it in my first PR.
AurelienRichez
force-pushed
the
fix/fix-909
branch
from
2ffd200 to
4df1e67
Compare
This fixes the problem introduced by #838 reported in #909.
The collect I introduced in my previous PR matched on nothing instead of saving the original exception type. As a consequence the the shrink process reads the whole stream provided by the shrinker. I imagine on a shrink with a very long stream (or infinite, but not sure if that exists in the wild) it would just hang.
cc @armanbilge