Handle configuration in a consistent manner

[alfert]

Currently we have several approaches for handling configuration, mostly for historical reasons. This is ugly and should refactored to a consistent, logical approach.

Sources of configuration are:

• Add a way to set default options #114
• Verbose ergonomics on exceptions #126
• Enhance testing experience #136

Let's discuss approaches!

[x4lldux]

To sum up, you can pass options to PropEr, using this methods:

1. directly via property running functions property/4 / quickcheck/3 (all options),
2. directly via property definition functions forall/3, exists/2, etc. (all options),
3. setting module-global options via use PropCheck, default_opts: ... (all options,
   a) setting list of options
   b) setting a function that returns a list of options
4. setting global options via config file / application environment (only 2 options supported),
5. setting global options via system environment variables (only 2 options supported).

There's no clear order in what overrides what.

Edit:
updated the list above.

[x4lldux]

Maybe this:
Leaving 1, 2 and 3b. Ditching 3a, 4 and 5. But providing a default implementation for default_opts function (configurable via config file / app env) which would read some options from system environment (similarly as I did in #136 with DefaultOpts.config/0).

Order of priority would be 3b, 1, 2 (outer to inner).

[evnu]

I would be opposed to removing 5. When debugging failing tests, allowing to override module-local configuration using an environment variable is very nice instead of changing the options manually in the source code.

[x4lldux]

@evnu:

But providing a default implementation for default_opts function (configurable via config file / app env) which would read some options from system environment

That's 5 implemented in default_opts function. If user would like to implement their own version, then they would loose 5 unless they also explicitly call our default implementation and merger their opts with the one returned by our implementation.

[alfert]

I like the approach 5 with default_opts. As already discussed somewhere else, we should provide a general approach to collect the options. Default options cry for the Application Environment, since this the reason why this exists at all. Any other kind of storage implementation is superfluous (unless you convince me - the process dictionary is seen as a nasty, unethical option and is only used by not-enlighted developers ;-) ).

[x4lldux]

@alfert might not work when running in async mode. A test module with its own default_opts function might override app env config for all other tests. We would need to create some cache, which stores configuration per test process. I'm not sure it's worth it.

[alfert]

might not work when running in async mode.

I need to be more precise, default options is the wrong name. What I meant are global configurations, which are independent from a specific test case (= global) and should be constant over a test run. They are injected by OS environment, mix or application configuration and the like. These settings should be parsed and then stored in the Application Environment centrally such that any evaluation of these global settings can happen independent from the source of injection. In this way we have only place where parsing of parameters and values happens. E.g. encoding of parameter names and values are different if done in mix and by the os environment.

[alfert]

A test case should never modify the app environment since it is generally not well defined when the values of the environment are read again, since it is used for initiall system configuration. A config function should be stateless, ie only depend on its parameters and the app env and does not modify any global state (we are functional, after all!) Von unterwegs gesendet

…

Am 05.12.2019 um 10:14 schrieb x4lldux ***@***.***>:  @alfert might not work when running in async mode. A test module with its own default_opts function might override app env config for all other tests. We would need to create some cache, which stores configuration per test process. I'm not sure it's worth it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[x4lldux]

I'm not sure I understand you correctly.
There is a hierarchy of option overriding in PropCheck:
global default opts (currently set by 5, 4 and some hard coded defaults) -> options returned by default_opts function (run on each test run by property/3 macro) -> options set on property/3 -> options set on forall/3. Each step can override previous options. And because options passed to forall are built on top of those passed to property there needs to be a some per process cache (like process dictionary) unless we drop setting options on property and just allow setting them only on forall.

[alfert]

Hmm, let me try to explain my thoughts. What I understand from your remarks, that we really need a good semantics how the various levels are detected and how they override each other such that

• the user is clear about the settings applied
• there is a good developer workflow for using defaults and overriding if required.

With this in mind, let us think about the settings

• Outer level is the os environment, mapped during startup into the App Env (mode 5). It should only contain the variables PROPCHECK_VERBOSE and PROPCHECK_DETECT_EXCEPTIONS for controlling the overall test process with changing any code or configuration file or similar. I don't think that we would need mode 4 (config file).
• Inside, we have the default_opts function handling the App Env properly, i.e. AppEnv overrides the respective values defined in the local options.
• Finally, we have the options set directly at the property and the forall macro. They override the default _opts. There are two exceptions: verbosity and exception handling shall be set from the outside.

To handle this properly, we need a function for proper merging of three lists of options: per property, per module and the App Env. This is at least partially available in #126.

Regarding options on the property and forall and the like, I would suggest to allow it only on the property level. Currently, our docs only discuss :quiet and :verbose as an option, so we would not miss much.

This should also remove the necessity of the process dictionary, since in the property macro the module-local and the global configurations are accessible and the merged options are fed downwards into the property execution. We can provide an undocumented complex data structure for our properly managed options (e.g as tuple: {:propcheck_internal_options, opts}, so that we can detect options set directly on the forall macro to warn the user that this is no longer the intended use (i.e. a deprecation warning).

What do you think?