-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
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 ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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
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 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. |
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 setexhaustive
in my dev env).The
deterministic
is used by default, and it limitsmax_examples
to5
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.