search

jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
578 stars 64 forks source link

Add jqwik.seeds.default = SEED_FROM_NAME configuration #412

Closed lmartelli closed 2 years ago

lmartelli commented 2 years ago

Overview

Some people don't like the idea that each run of the tests uses completely different data (they fear the CI pipeline being flaky). So in order to ease PBT adoption, I propose to allow to generate the seed for each tested property in a deterministic manner, from its name. This, CI tests can be made, if needed, totally reproducible as they are with example based testing.

Details

I added a new configuration, jqiwk.seeds.default, that can the value of "SEED_FROM_NAME" to trigger the aforementioned behaviour. The default value for the new config option is "", which gives the usual behaviour (a new random seed is used each time). You could also pass a numeric value to use a fixed seed for all tests.

I had to make CheckedProperty.configurationWithEffectiveSeed() public in order to be able to test. But maybe we could add the tests on PropertyConfiguration instead ...

I hereby agree to the terms of the jqwik Contributor Agreement.

jlink commented 2 years ago

Hi @lmartelli. Many thanks for using and diving into jqwik. One thing I recommend before creating a PR for jqwik (or any other OSS project for that matter) is to discuss the suggested new functionality with the maintainers of the project first. There may be good reasons why the feature you deem a worthwhile enhancement has not been added before. Or, the feature as such might be a good idea, but the way you want to implement it is not. Having a discussion beforehand, might spare you valuable time.

That said, I’ll have a closer look in the next days and tell you what I think.

jlink commented 2 years ago

Having had a night to sleep about the feature you'd like to implement in this PR I decided to not do it.

My line of argument:

  1. The randomness in generating different test inputs across different test runs is one of the core benefits of PBT. 1000 (or even 10000) different sets of inputs are often not enough to cover all of the potentially erroneous parts of logic in non-trivial programs. Re-running the same properties with different random seeds will provide another order of magnitude higher variation long-term.
  2. The real drawbacks of non-determinism (flaky tests, non-reproducible failures) are mitigated by other mechanisms:
    • If a property fails, its random seed will be fixed until it succeeds again. This holds for development time and CI (if configured correctly).
    • Shrinking takes care that (in many/most cases) even initially different inputs that fail will be shrunk to the same set of values.
  3. Adding the default-seed property makes an already involved algorithm for determining a single try's seed value even more complex. The small benefit does not outweigh the added burden of logic and maintenance IMO.
  4. If you really want a single, fixed random seed for each property, you can implement it rather easily for yourself: Register a global AroundPropertyHook and use context.properties().setSeed(..) to fix the seed value.

@lmartelli: I hope that decision doesn't disappoint you too much, since the approach sketched in 4 will give you an option, if my points in 1 and 2 couldn't convince you of not really needing the feature in the first place.

lmartelli commented 2 years ago

I must say I had not thought of saving the DB in the CI's cache in order to keep the seeds of failing tests.

I will try the AroundPropertyHook.

Thanks for reviewing the PR quickly.