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

[WIP] Allow ExUnit assert function for forall return #207

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

Conversation

drselump14
Copy link

@drselump14 drselump14 commented Aug 30, 2021

Issue: #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 drselump14 force-pushed the feature/pass_truthy_as_true_to_proper branch from 61441e3 to 8408f7f Compare August 30, 2021 21:22
@drselump14 drselump14 changed the title Allow ExUnit assert function for forall return [WIP] Allow ExUnit assert function for forall return Aug 30, 2021
@alfert
Copy link
Owner

alfert commented Aug 31, 2021

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.

@drselump14
Copy link
Author

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.

@coop
Copy link

coop commented May 17, 2023

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.

@alfert
Copy link
Owner

alfert commented May 18, 2023

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?

@coop
Copy link

coop commented May 18, 2023

@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...

@evnu
Copy link
Collaborator

evnu commented May 19, 2023

Note that the :verbose option (#48) also outputs the results of assertions. One can also use PROPCHECK_VERBOSE=1 to set that option globally.

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