propcheck icon indicating copy to clipboard operation
propcheck copied to clipboard

[WIP] Allow ExUnit assert function for forall return

Open drselump14 opened this issue 2 years ago • 6 comments

Issue: https://github.com/alfert/propcheck/issues/187

Currently, propcheck forall function requires a boolean value return, which bit inconvenient when using ExUnit assert function. This PR fixes the issue and allow the assert function with non-boolean result

TODO:

  • write a proper test
  • handle tuple assertion

drselump14 avatar Aug 30 '21 21:08 drselump14

Hi @drselump14 ,

thank you for your PR, but I guess you are solving the problem at a wrong place: PropEr and thus PropCheck require a boolean return value. assert requires a boolean assertion. We handle assertion errors as a handy special case of false.

But if I understand your approach correctly, then you kind of change the semantics of assert to handle non-boolean arguments differently - but only inside a forall. If you find a counterexample and put that into a regular test, your pimped assert will not work any longer. That is confusing and does not help anybody. The correct way of solving this is to convince the Elixir core team to make assert more flexible, in which case we can benefit in PropCheck as well.

Did I got your intention right?

Cheers, Klaus.

alfert avatar Aug 31 '21 07:08 alfert

Hi @alfert , thanks for the comment. You're right about PropEr and thus PropCheck require a boolean return value. However, I think assert requires a boolean assertion is not a true statement. The assert evaluate truthy value as mentioned in docs.

The assert compatibility issue with PropCheck/PropEr occurs when we evaluate non-boolean truthy value with assert. Because assert return the evaluated value, it'll return the non-boolean truthy value which PropEr doesn't like.

For example:

property "returns a map" do
  forall i <- pos_integer() do
    assert %{val: ^i} = make_map(i)      # This doesn't work and raises an error {:error, :non_boolean_result}
  end
end

It happens because PropEr forall only pattern matching the boolean result value, and tuple result value (from willfail, etc), and reject any other kind of value

https://github.com/proper-testing/proper/blob/9f6a6501430479bed66d08cd795cd34d36ec83aa/src/proper.erl#L1769

TBH I'm not sure if this PR is the best approach to solve the issue. I think the best approach is to convince the PropEr team to allow a truthy value to be passed to the forall method. However, I don't think they will accept it because it's a requirement from elixir world.

drselump14 avatar Aug 31 '21 14:08 drselump14

I am probably projecting my own wishes on this PR but ExUnit.Assertions are great due to how much information is available when the assertion fails. One of the hardest things I've found while using PropCheck is the ability to debug "when something goes wrong" - ex unit's assertions on the other hand provide very handy diffs.

It's incredibly likely I just don't have PropCheck/proper experience to show better diffs - typically when a post condition fails I have to rerun the test with a bunch of IO.puts to track why the postcondition failed, if instead I got the diff from assert (and friends) the IO.puts might be unnecessary.

coop avatar May 17 '23 12:05 coop

Yes, reporting facilities from assert are really really nice. Perhaps we can tackle this the other way around: Perhaps we can replicate the code of assert (or even call it? - may be difficult due to macros) to show the values of a failing property in a nicer way?

alfert avatar May 18 '23 07:05 alfert

@alfert I can't imagine the assert code changing any time soon so copy pasting seems fine given that it is implemented as macros. I'm more unsure how'd you go about doing this given proper expects boolean return values...

coop avatar May 18 '23 10:05 coop

Note that the :verbose option (https://github.com/alfert/propcheck/pull/48) also outputs the results of assertions. One can also use PROPCHECK_VERBOSE=1 to set that option globally.

evnu avatar May 19 '23 09:05 evnu