search

jmgilman / beancount-hypothesis

A package which provides hypothesis strategies for generating beancount types
MIT License
0 stars 1 forks source link

Avoid using `random` module inside Hypothesis strategies #1

Closed Zac-HD closed 2 years ago

Zac-HD commented 2 years ago

Hey there! I was looking through packages that use Hypothesis, and noticed something pretty weird:

https://github.com/jmgilman/beancount-hypothesis/blob/e6501171f2e1b0e879f67fd1812f6da45d465d39/beancount_hypothesis/account.py#L93-L101 https://github.com/jmgilman/beancount-hypothesis/blob/e6501171f2e1b0e879f67fd1812f6da45d465d39/beancount_hypothesis/common.py#L33-L41

These strategies don't actually use Hypothesis to generate data! Instead, you're using the global random module, which Hypothesis seeds before every example to avoid flakiness. That's really really not intended to be used like this though - you'll be "generating" more or less the same example each time, and if it does fail shrinking won't work very well.

On a lighter note, you also have one that just doesn't need to be a composite strategy: instead of

https://github.com/jmgilman/beancount-hypothesis/blob/e6501171f2e1b0e879f67fd1812f6da45d465d39/beancount_hypothesis/data.py#L64-L72

you could return st.builds(inv.Inventory, s.lists(position(), max_size=3))

jmgilman commented 2 years ago

Hi,

Thanks for raising an issue. This was my one and only time using hypothesis, so I appreciate the feedback.

On a lighter note, you also have one that just doesn't need to be a composite strategy

Fixed in 002b8b895de40764a852cdcf57a66a9ed2f67e1b.

Closing this out since the other issue is addressed in #2.