Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shrinking indication parameter #232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

x4lldux
Copy link
Contributor

@x4lldux x4lldux commented Mar 28, 2020

Set's a shrinking parameter to true when property is being executed
during a shrinking phase. Then sets it to done after the shrinking is
finished. Useful for adjusting generators during shrinking phase or for
on_output printers, for example, so they can suppress printing.

@kostis
Copy link
Collaborator

kostis commented Apr 3, 2020

Thanks for your PR. However, I do not understand what it does, at least on its own.

If this is a stand-alone PR, please supply some test cases that show its intended use and/or functionality (perhaps in conjunction with the on_output parameter as you write in the commit message?). Unfortunately, the current test does not really show something interesting; for example, the current on_output fun does not do any printing; also, it seems strange that all three values for the shrinking indication parameter appear at the same time in the parameter list -- or did I misunderstand the test? In any case, the test needs to be improved.

On the other hand, if this supposed to be just supporting functionality for #230, then please add the changes there.

@x4lldux
Copy link
Contributor Author

x4lldux commented Apr 3, 2020

Thanks for your PR. However, I do not understand what it does, at least on its own.

It introduces a parameter (as in propter_types:parameter/2) that indicates PropEr is in shrinking stage.

On the other hand, if this supposed to be just supporting functionality for #230, then please add the changes there.

This PR can be viewed as part for #230, but for that specific cases mentioned there, it would be sufficeable to just use the ?SHRINK macro. So I opted for a separate PR.

please supply some test cases that show its intended use and/or functionality (perhaps in conjunction with the on_output parameter as you write in the commit message?)

It is a fact, that I intend to use this in PropCheck (Elixir wrapper library for PropEr) to suppress output while in shrinking stage, as mentioned in commit message as an example. But unfortunately, even though EUnit captures test's output, I couldn't find any way to match on it. And there isn't any other test that tests on_output functionality from which I could get an inspiration of how to do it properly. Only a TODO note with a suggestion of using process dictionary, which I used. My test just ensures that every of three states the shrinking parameter can be in, was in fact present in pdict. Maybe I'll put comments inside the test to describe it's actions.

@kostis
Copy link
Collaborator

kostis commented Apr 4, 2020

It introduces a parameter (as in proper_types:parameter/2) that indicates PropEr is in shrinking stage.

Yes, I could see that. My question was asking for an example that shows how/where this new functionality would be used. I.e., some evidence why would one want that in PropEr.

I intend to use this in PropCheck to suppress output while in shrinking stage, as mentioned in commit message as an example. But unfortunately, even though EUnit captures test's output, I couldn't find any way to match on it. And there isn't any other test that tests on_output functionality from which I could get an inspiration of how to do it properly.

First of all, PropEr has a noshrink option that can be used to suppress the shrinking phase altogether, thus providing an indirect way to suppress output from shrinking ;-)

But it seems that you do not want that -- if I understand correctly, you want to suppress printing just while shrinking. The on_output function can actually do that. Here is how:

-module(on_out).
-export([test/1, print_magenta/2, print_not_during_shrinking/2]).

-include_lib("proper/include/proper.hrl").
-include_lib("eunit/include/eunit.hrl").

test(Fun) ->
   proper:quickcheck(prop_me_up_scotty(), [{on_output, Fun}]).

print_magenta(S, L) ->
   io:format("\033[1;35m"++S++"\033[0m", L).

-define(WHATEVER_YOU_DESIRE, no_printing_during_shrinking_please).

print_not_during_shrinking(S, L) ->
   case get(?WHATEVER_YOU_DESIRE) of
     undefined ->
       case string:find(S, "Shrinking") of
         nomatch -> io:format(S, L);
         _ -> put(?WHATEVER_YOU_DESIRE, in_shrinking_phase)
       end;
     in_shrinking_phase ->
       case string:find(S, "time(s)") of
         nomatch -> ok; % no printing during shrinking
         _ -> erase(?WHATEVER_YOU_DESIRE) % shrinking just ended
       end
   end.

prop_me_up_scotty() ->
  ?FORALL(I, integer(), I < 42).

Here is what I get when using it -- colors are not shown properly here:

Eshell V10.3.4  (abort with ^G)
1> on_out:test(fun on_out:print_magenta/2).
..............................................................!
Failed: After 63 test(s).
47
 
Shrinking .(1 time(s))
42
false
2> on_out:test(fun on_out:print_not_during_shrinking/2).
.............................................................!
Failed: After 62 test(s).
114
42
false

If you want something other/more than the above, please supply an example.

@x4lldux
Copy link
Contributor Author

x4lldux commented Apr 4, 2020

First of all, PropEr has a noshrink option that can be used to suppress the shrinking phase altogether, thus providing an indirect way to suppress output from shrinking ;-)

Nope 😁


While this would work for some cases, it wouldn't work for all. It fails if for example, while shrinking, a more complex test would return a non boolean, or a ?SUCHTHAT generator wouldn't be able to produce a valid case, etc. In such cases nothing would be printed, because proper doesn't print "time(s)" string, just reports the error.

Also, it isn't an optimal solution to depend on quasi-syntax of output for semantics and inferring the internal state of PropEr. Not a one I would feel comfortable to put in a PropCheck library.

@kostis
Copy link
Collaborator

kostis commented Apr 4, 2020

First of all, PropEr has a noshrink option that can be used to suppress the shrinking phase altogether, thus providing an indirect way to suppress output from shrinking ;-)

While this would work for some cases, it wouldn't work for all. It fails if for example, while shrinking, a more complex test would return a non boolean,

I do not understand how telling PropEr to not do any shrinking at all can result in failing during shrinking... Unless of course you are replying in another part of the above message.

@x4lldux
Copy link
Contributor Author

x4lldux commented Apr 4, 2020

Let me rephrase that. I didn't wanto to quote the code example.

@kostis
Copy link
Collaborator

kostis commented Apr 4, 2020

Can you please not edit / change the messages you have written, thus making the discussion here unreadable for future readers?

More importantly, can you please provide some example of what you really want to do and show me/us why you cannot achieve that with the machinery (on_print, ?SHRINK, with_parameter, etc.) that PropEr currently provides?

Adding stuff to the process dictionary, besides being ugly, may complicate other PropEr efforts that are currently going on. For example, the parallelization work.

@x4lldux
Copy link
Contributor Author

x4lldux commented Apr 4, 2020

More importantly, can you please provide some example of what you really want to do and show me/us why you cannot achieve that with the machinery (on_print, ?SHRINK, with_parameter, etc.) that PropEr currently provides?

So usecase is to reduce noisy printing in PropCheck by stop PorpEr from printing while shrinking.

This can't be done with on_print and inspecting the output, like in your example, because inspecting can give just an approximation of PropEr's state. Also, the output generated by PropEr is not part of it's public interface (there is no docs for what is printed when, and a lot of it is hidden in private functions) and can change at any moment.

This can't be done with ?SHRINK or with_parameter, because tests are not written by us, but by PropCheck's users. We can't force users to boilerplate their tests themselves just to let us reduce the printing noise.

Adding stuff to the process dictionary, besides being ugly, may complicate other PropEr efforts that are currently going on. For example, the parallelization work.

Do note, that I'm not introducing any new process dictionary keys, but reusing the existing one, which is set by with_parameter/3 and read by parameter/2. This PR is effectively reusing an existing feature to add a builtin parameter shrinking (like ECQ does).

Set's a `shrinking` parameter to `true` when property is being executed
during a shrinking phase. Also sets it to `done` after the shrinking is
done. Useful for adjusting generators during shrinking phase or for
`on_output` printers.
@x4lldux x4lldux force-pushed the shrinking_parameter branch from 67faa71 to ca41234 Compare April 4, 2020 15:34
@codecov-io
Copy link

Codecov Report

Merging #232 into master will increase coverage by 0.79%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   82.24%   83.04%   +0.79%     
==========================================
  Files          13       13              
  Lines        3166     3167       +1     
==========================================
+ Hits         2604     2630      +26     
+ Misses        562      537      -25     
Impacted Files Coverage Δ
src/proper.erl 85.09% <100.00%> (+2.85%) ⬆️
src/proper_target.erl 87.50% <0.00%> (-1.74%) ⬇️
src/proper_fsm.erl 80.32% <0.00%> (-1.64%) ⬇️
src/proper_typeserver.erl 73.40% <0.00%> (-0.12%) ⬇️
src/proper_types.erl 91.77% <0.00%> (ø)
src/proper_statem.erl 92.17% <0.00%> (+0.43%) ⬆️
src/proper_shrink.erl 95.74% <0.00%> (+0.53%) ⬆️
src/proper_gen_next.erl 77.41% <0.00%> (+1.20%) ⬆️
src/proper_sa.erl 92.15% <0.00%> (+2.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b66abe...ca41234. Read the comment docs.

@kostis
Copy link
Collaborator

kostis commented Apr 5, 2020

Can you please provide some example of what you really want to do and show me/us why you cannot achieve that with the machinery (on_print, ...) that PropEr currently provides?

So use case is to reduce noisy printing in PropCheck by stop PropEr from printing while shrinking.

This can't be done with on_print and inspecting the output, like in your example, because inspecting can give just an approximation of PropEr's state. ...

I've given the issue some more thought and I am still not convinced that this is the PropEr(TM) solution to this particular use case. To control the amount of printing that users see, PropEr currently provides options verbose (default) and quiet. These are all-or-nothing, and perhaps there are users/tools out there that desire finer control over the amount of output they get. For example, output on running tests and no output on shrinking (your/PropCheck's use case), or perhaps the other way around (silent/less verbose running of tests and output on shrinking).

My suggestion would be to extend the options with something along the lines of

-type phase() :: testing | shrinking.
-type phase_output() :: {phase(), quiet | laconic | verbose | output_fun()}.

which also allows users to supply their own output functions for the two different phases, if they so wish. For the laconic option, I imagine just a line with a counter (e.g. Run test #N, Shrunk #N time(s)) that continuously gets updated.

Comments?

@x4lldux
Copy link
Contributor Author

x4lldux commented Apr 23, 2020

I like your proposal! I like the idea of exposing phase instead of just shrinking and it's something similar to what I was thinking when considering semantic printing for PropEr. Though instead of adding another output function, I suggest extending current one with more information (and do arity recognition to stay backwards-compatible). And instead of just passing format and data, also pass some metadata like current phase and type of printout. So for example instead of Print("!", []) in perform_search/8, there'd be something like Print({search_fail, Phase}, "!", []). That would systematize output and make it easier for any one wanting to create a wrapper around PropEr for their BEAM language and have printouts in their syntax.

@kostis
Copy link
Collaborator

kostis commented Apr 23, 2020

NOTE: I've slightly edited the above comment and moved a part which is related to a different issue (#246) so that it can be discussed on its own.

@david-kubecka
Copy link

I came here to discuss slightly different use case but at the end I think it's very much related.

In my property test I'm constructing huge terms which produce a lot of noise when printed out by PropEr on assertion failure. So I would like to disable printing of these generated terms while keeping the other PropEr output. I can imagine that there could be some new formatting option similar to existing {on_output, Fun/2}. The Fun would this time take 3 arguments:

fun (MessageType, Format, Data) ->
    <do whatever you want>
end

where Format and Data are the same as in on_output while MessageType is an atom describing current message type (or phase or... TBD). So the values would include at least:

progress_indicator
stacktrace
test_input
shrink_input

In my case I would then pattern match on test_input to ignore it while printing everything else.

What do you think? Is my way somehow aligned with the use case of @x4lldux ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants