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

chore: Tune Hypothesis configuration #125

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

aiven-anton
Copy link
Collaborator

@aiven-anton aiven-anton commented Feb 2, 2024

Note! Please review first commit, and just verify fall-out on code generation in the second.

This addresses the issue that Java compatibility tests and other roundtrip tests were configured with wildly different parameters. It removes the explicit @setting(max_examples=1) from all round trip tests, and now instead only configures this on a global level.

Adds two separate Hypothesis profiles and support for choosing them with the HYPOTHESIS_PROFILE env var (I use direnv to set exhaustive in my dev env).

The deterministic is used by default, and it limits max_examples to 5 which seems like a reasonable amount to start with. It enables Hypothesis' own "deterministic" feature, which means it promises to always generate the same data for a test function, as long as the test function itself doesn't change. This is appropriate for CI.

Deadlines and the "too slow" healthcheck are disabled in both profiles, as it's more likely to trigger on VM pauses than actual slow data generation.

Fixes #113.

This addresses the issue that Java compatibility tests and other
roundtrip tests were configured with wildly different parameters. It
removes the explicit `@setting(max_examples=1)` from all round trip
tests, and now instead only configures this on a global level.

Adds two separate Hypothesis profiles and support for choosing them with
the `HYPOTHESIS_PROFILE` env var (I use [direnv] to set `exhaustive` in
my dev env).

[direnv]: https://direnv.net/

The `deterministic` is used by default, and it limits `max_examples` to
`5` which seems like a reasonable amount to start with. It enables
Hypothesis' own "deterministic" feature, which means it promises to
always generate the same data for a test function, as long as the test
function itself doesn't change. This is appropriate for CI.

Deadlines and the "too slow" healthcheck are disabled in both profiles,
as it's more likely to trigger on VM pauses than actual slow data
generation.
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e064cad) 96.57% compared to head (98b251c) 96.57%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #125   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files          14       14           
  Lines         994      994           
  Branches      138      138           
=======================================
  Hits          960      960           
  Misses         26       26           
  Partials        8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aiven-anton aiven-anton marked this pull request as ready for review February 5, 2024 08:03
@aiven-anton aiven-anton requested review from a team as code owners February 5, 2024 08:03
Copy link

@eliax1996 eliax1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I'm wondering if make sense to remove the tests from the git repo and always generate the sources from the definition at runtime, its really a huge diff and its hard to navigate from the history :)
Not something I will fight for, just understand if could make sense having out of the history

@eliax1996 eliax1996 added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit b65592d Feb 13, 2024
12 checks passed
@eliax1996 eliax1996 deleted the aiven-anton/tune-hypothesis-configuration branch February 13, 2024 15:10
@aiven-anton
Copy link
Collaborator Author

I'm wondering if make sense to remove the tests from the git repo and always generate the sources from the definition at runtime, its really a huge diff and its hard to navigate from the history :)
Not something I will fight for, just understand if could make sense having out of the history

Yeah, it's a good discussion to have. The current state is a bit of an experiment. However, it was a very deliberate decision to begin with and the reason is because I think it's super helpful from a library consumer's perspective. Being able to jump to definitions, and to link to an actual line is valuable (I can link you here and we can discuss around AddOffsetsToTxnRequest.transactional_id).

If we were to turn that into something that's just generated at packaging time, I believe it becomes much more abstract in the sense that there is no longer concrete lines of code to navigate through and to reference.

I agree status quo is not optimal when reviewing code generation changes, but I'm also somewhat hoping that these changes will become more and more rare as the library stabilizes (so far this has not been true 🙈).

An other aspect is that it might not actually become easier to review if we lazy-generate the models, it might actually become harder to determine what the actual impact on generated code is for a given PR, when the delta is not explicitly displayed.

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.

Balance property test configuration
3 participants