Verbose ergonomics on exceptions

[evnu]

This pull request implements a crude exception detection for PropCheck. It adds an agent started within property to capture additional output which we might want to display in the error message.

This is an attempt to fix #110.

[evnu]

@alfert I found a workaround to put the output where it belongs. By gathering the output in an extra process, started within the properties macro, we can make forall write its output there. The property macro is then able to retrieve the additional output from that process.

This is obviously rather hacky and building on us sharing information between property and forall using the process dictionary. But maybe this attempt can help us figure out how to proceed here.

Note that this PR does not yet implement a silence_exceptions flag. I will implement that if we come to the conclusion that we actually want to handle the detection like this.

[evnu]

I use a MapSet here to reduce the verbosity on shrinking. Otherwise, if we hit the exception on every attempt to shrink, the output will be large. Note however that the stacktraces in the assert_test.exs are apparently not always the same, so there is still more output than there should be.

[evnu]

@alfert

If you want to test this locally, you can run mix test test/assert_test.exs.

[alfert]

This is obviously rather hacky and building on us sharing information between property and forall using the process dictionary.

If I read your code correctly, then there is no process dictionary involved. I follow Joe Armstrong here and would avoid it under nearly any cost.

[alfert]

Silly github question: Is the force push really required all the time? I usually don't do this unless I amend something.

[evnu]

If I read your code correctly, then there is no process dictionary involved. I follow Joe Armstrong here and would avoid it under nearly any cost.

The process dictionary is involved, but the change is hidden in this PR. The property macro already writes the opts to the process dictionary (we needed that before for sharing verbose between property and forall, see #111). This change extends the opts in property, so those extended opts are then added to the process dictionary. forall retrieves the opts from the process dictionary here by calling into PropCheck.Utils.get_opts() (this was part of #122).

[evnu]

Silly github question: Is the force push really required all the time? I usually don't do this unless I amend something.

I amended the commit where if output_agent do was introduced in propcheck.ex, that's why I force pushed.

[alfert]

The process dictionary is involved, but the change is hidden in this PR.

I see, but this was already ok and its only configuration data. Go ahead!

[evnu]

I see, but this was already ok and its only configuration data. Go ahead!

I still don't like using the process dictionary too much, but I also did not find any other way yet to share options between different macros :(

I will make this PR work with travis. What do you think of the hack clean_opts to make the options work with PropEr? PropEr expects a certain set of options and complains if it finds :output_agent. I could also separate the options if you'd prefer that.

[alfert]

What do you think of the hack clean_opts ...

I have mixed feelings. The clean up - if required - should be done in our Util module since where we know exactly which options we introduce (might be additional ones in the future). For this particular case, isn't the verbose flag all what we need? The output agent can run under a stable address(= fix local name, its a singleton), so no need for using the agent's name. But I might oversee something.

[evnu]

For this particular case, isn't the verbose flag all what we need?

We currently support other options there as well, e.g. numtests.

Placing the clean up in Util sounds reasonable.

The output agent can run under a stable address(= fix local name, its a singleton)

I did not do that in order to avoid spill-over output between different tests, and to allow tests running concurrently. If tests are run concurrently and if the output agent is a singleton, the output agent must take care to split the outputs of different tests. This should be doable by passing in an identifier (e.g. a hash) for a test. I can give that a try if you prefer that to cleaning up the options.

[alfert]

Then let's place the cleanup in Util and we should extract all Elixir specific values.

[...] and if the output agent is a singleton [...]

I see and I did not get this detail last night. Let's running several agents in parallel (which makes really sense) and we profit from the BEAM architecture, yeah! Please go ahead!

[evnu]

Some more thoughts on a flag to enable/disable this:

• I think detect_exceptions might be a better name than silence_exceptions. What do you think?
• Similarly to verbosity, I think an environment variable such as PROPCHECK_DETECT_EXCEPTIONS would be nice.

[alfert]

I agree to both Von unterwegs gesendet

…

Am 24.10.2019 um 20:17 schrieb Magnus ***@***.***>:  Some more thoughts on a flag to enable/disable this: I think detect_exceptions might be a better name than silence_exceptions. What do you think? Similarly to verbosity, I think an environment variable such as PROPCHECK_DETECT_EXCEPTIONS would be nice. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[evnu]

And one more thought: If an assert fails in a forall, I would think that detect_exceptions should lead to this being printed similarly to the other failures, even if verbose is not set.

[evnu]

I implemented the changes (but did not yet change the behaviour on assert for now).

Note that the output can grow arbitrarily large if the detected error is changing during shrinking. For example, the following results in a lot of exceptions being detected:

# Shrinking might take a lot of steps to find a counter example
  property "lengthy output?", [numtests: 10000] do
    forall n <- integer(1, :inf) do
      if n > 500 && rem(n, 2) == 0 do
        raise "#{n}"
      else
        true
      end
    end
  end

I therefore set detect_exceptions to false by default. It seems hard to sanitize the errors further than a simple MapSet.

[alfert]

Looks like we done, aren't we? It's a cool addition to the feature set.

[evnu]

Yes, I am pretty happy about this change now.