triq icon indicating copy to clipboard operation
triq copied to clipboard

Shrinker is too eager

Open essen opened this issue 10 years ago • 12 comments

I have a generator that looks like this:

token() -> ?SUCHTHAT(B, ?LET(T, list(tchar()), list_to_binary(T)), byte_size(B) > 0).

On error, the shrinker always returns <<>> which is invalid input (fails the byte_size(B) > 0 constraint).

I would probably need some hand holding to help fix that one.

essen avatar Dec 14 '14 18:12 essen

How does this fare?

token() -> ?LET(T, non_empty(list(tchar())), list_to_binary(T)).

notice the non_empty.

krestenkrab avatar Dec 14 '14 19:12 krestenkrab

I also looked briefly at the code, and I don't understand why this happens. Will have to dig a bit deeper, but there was recently a change to do more agressive shrinking which may be what triggered this...

krestenkrab avatar Dec 14 '14 19:12 krestenkrab

I will try and let you know.

I don't know about "recently" as I only just started using Triq. This is the only behavior I've seen.

essen avatar Dec 14 '14 19:12 essen

This seems to be a little better although I still get some empty tokens after shrinking:

{ T , S , P , MediaType } = {<<57>>,<<33>>,[{<<>>,[34,[],34]}],<<57,47,33,44,61,34,34>>}

This is {token(), token(), [{token(), token() | quoted_string()}], binary()}. The first two token() are not <<>> anymore, so that's good. But the token() inside the list is <<>> when it should be non_empty().

Needless to say, the shrunk binary(), which is the flattened representation of the 3 before, is not making any sense and of course trying with these values makes the property fail.

But hey at least the property looks a little better thanks to non_empty(). :-)

essen avatar Dec 14 '14 19:12 essen

I have tried writing a test for this, here is what I got:

binary3_test() ->
    [<<_>>] = triq:counterexample(?FORALL(_,
        ?LET(S,
            ?SUCHTHAT(L, list(char()), length(L) > 0),
            list_to_binary(S)),
        false)).

It fails (as expected) as it tries to shrink to <<>>. This is all new to me so I am not too sure about it though. I would be thankful if you could confirm the test looks good and I will try to fix the bug using it.

essen avatar Dec 16 '14 13:12 essen

I think you need something like

list_test() ->
    triq:check( ?FORALL( B,
                         ?LET(S,
                              ?SUCHTHAT(L, list(char()), length(L) > 0),
                              list_to_binary(S)),
                         byte_size(B) > 0 )).

i.e., the B is tested using erlang:byte_size/1. If you just put false then the test always fails.

krestenkrab avatar Dec 16 '14 18:12 krestenkrab

But this doesn't check that the shrinker won't return me a <<>> does it? This is what I want to test: shrinker shrinks to <<>> when generator disallow it.

essen avatar Dec 16 '14 19:12 essen

Hm, as far as I can see; the above generator for B should not generate binaries of empty size. So the test fails if such are generated. But you're right - this doesn't test shrinking. So we need a test that always fails, and then we want to check that the resulting smallest counter example is not an empty binary. Comprendo.

Your test is correct:

in function triq_tests:binary3_test/0 (test/triq_tests.erl, line 64)
**error:{badmatch,[<<>>]}
  output:<<"Failed!

Failed after 1 tests with false
Simplified:
    _ = <<>>
">>

I would look for a fix by adding a shrink= clause in triq_dom:suchthat/2. It would seem that you need to apply the predicate to the shrunken value also; or just return the value pre-shrinking.

krestenkrab avatar Dec 18 '14 13:12 krestenkrab

Yes. I think my example test is OK though I do not know if the shrunk value can be more than a 1 byte binary. I wouldn't expect it to be more than 1 byte considering the property, but am not familiar enough to be certain it won't return a bigger one.

essen avatar Dec 18 '14 13:12 essen

Hmm. Tried this, but it still fails. The code below applies the predicate after shrinking the embedded value. More hmm.

%% @doc Support function for the ?SUCHTHAT macro.
%% @private
%% @spec suchthat(domain(T),fun((T) -> boolean())) -> domain(T)
-spec suchthat(domain(T),fun((T) -> boolean())) -> domain(T).
suchthat(Dom,Predicate) ->
    #?DOM{kind=#suchthat{dom=Dom,pred=Predicate},
          pick=fun(#?DOM{kind=#suchthat{dom=Dom1,pred=Fun1}},SampleSize) ->
                       suchthat_loop(?SUCHTHAT_LOOPS,Dom1,Fun1,SampleSize)
               end,
          shrink=fun(#?DOM{kind=#suchthat{dom=Dom1,pred=Fun1}},Val1) ->
                      {Dom2,Val2} = shrink(Dom1,Val1),
                      case Fun1(Val2) of
                          true -> { suchthat(Dom2, Fun1), Val2 };
                          _ -> erlang:exit({shrinking_failed, Fun1})
                      end
                 end
         }.

krestenkrab avatar Dec 18 '14 13:12 krestenkrab

Hm I don't understand what shrink function is set to the suchthat #?DOM in the current master code (without your patch). From what I see the default is a function that triggers an error exception. And I can't see anywhere else that sets the shrink function for suchthat. Maybe something else overwrites the shrink function you set here?

essen avatar Dec 18 '14 14:12 essen

Fixed in triqng/triq#26. The pick function has been returning a vanilla domain instead of the suchthat{} version, so the picked value has been being shrinked without applying a predicate.

KrzysiekJ avatar May 09 '17 05:05 KrzysiekJ