Keep the same error type while shrinking #838
Conversation
AurelienRichez
force-pushed
the
keep-same-error-while-shrinking
branch
from
9711aaf
to
2e81b64
Compare
|
@SethTisue @rossabaker @ashawley @rickynils Have you seen this? (seems like you are the maintainers based on the github information) While it doesn't solve completely the #129 issue, it significantly improves UX in such cases. |
|
Looking back at this PR, I think my unit tests are a bit weird (the second one at least does not have a different type for the exception). I'll change it later on to fix that, but I would love to have some feedback if the idea itself is worthwhile to you. |
AurelienRichez
force-pushed
the
keep-same-error-while-shrinking
branch
from
2e81b64
to
cc7e7e8
Compare
|
Awesome 🎉. Thanks a lot @rossabaker |
|
@AurelienRichez if you have some time to investigate #909 it would be much appreciated, appears to be related to this PR. Thank you very much! |
|
That's interesting. I'll take a look. |
| @@ -784,7 +788,8 @@ object Prop { | |||
| def shrinker(x: T, r: Result, shrinks: Int, orig: T): Result = { | |||
| val xs = shrink(x) | |||
| val res = r.addArg(Arg(labels,x,shrinks,orig,pp(x),pp(orig))) | |||
| if(xs.isEmpty) res else getFirstFailure(xs) match { | |||
| val originalException = Some(r.status).collect { case NonFatal(e) => e.getClass() } | |||
There was a problem hiding this comment.
@armanbilge I think the culprit is here.
Initially it was case Exception and I changed it to NonFatal. But I got confused by the name and thought for a moment I was matching on a regular Exception (which does not really make sense in hindsight 😅), and just replaced it by NonFatal.
Except it was actually Exception as in Status variant. So originalException is always empty because this never matches and case (_, Result(Exception(e), _, _, _)) => !exceptionFilter.contains(e.getClass) always return true.
I'm not exactly sure why it hangs in the issue, but I imagine that if for some reason the stream of shrink is infinite (or long enough given some combination ?) then it will hang because as it is in case of an exception it just drops everything in the stream.
I cannot continue right now but I'll open a draft PR with the fix, and see if I can come up with more robust tests.
There was a problem hiding this comment.
@AurelienRichez thank you very much for looking into this so quickly! Please ping me when you put up your PR and I will take a closer look :)
This is a proposal to change the way
Propis running the shrinking process, which I think can mitigate #129.Currently, when scalacheck finds an input that either proves that the property is false or throws an exception it start the shrinking process and find the first input in the list which cause any failure.
My proposal is to change that so that we find the first input in the list which cause the same failure status as before (meaning an
Exceptionwith the same class or aFalse).Motivation
The current behaviour might be confusing because scalacheck finds an error and can hide it behind some other error after shrinking.
As a nice side effect it will mitigate #129 (shrinking disregards any filters on an explicit Gen). This actually what I find more interesting here.
Every time I had a problem with shrinking not respecting the generator constraints, the constraint was to avoid things such as division by 0 or other invalid input. Preventing the shrinking process from changing the type of error would have fixed the problem in every instance.
I took a division by 0 as an example in the tests. Currently a failing test will show and
ArithmeticExceptionbecause of the shrinking. With the proposed change it won't because the input0would transform the statusFalseinto anException.Limitations
It does not completely solves #129 when the test throws the same exception for every case (like the first example in the issue). My experience makes me think that this is rarely the case in the real world.
What do you think ?