#Random in testutils

26 messages · Page 1 of 1 (latest)

hard yarrow
#

In a few places we've used a non-predictable random in the testutils of the SDK.

Places being:

  • Address::random()
  • Bytes::random()
  • Env source account is random

I think we should remove that and either replace it with a fixed seed random.

#

There are two problems I'm facing.

#

The first is that Address::random results in contracts in tests always being deployed to different addresses. This makes it frustrating to write exact quality tests against debug logs, events, or anything that ends up outputting the address in some form.

#

To pre-answer the "why are equality tests important" question: Equality tests mean failing tests output the unexpected result when it isn't equal. So things we can do in our testutils to make it easier to use assert_eq instead of assert, makes debugging failing tests a better experience.

#

The second is that Bytes::random generally causes tests to diverge in behavior. Which will make it harder for folks to naturally write tests where it is possible to observe changing behavior over time.

split oxide
#

hmm, I think we're already doing that from the env side; IIRC we have a testing-only prng

#

(though that's probably used for env tests only)

hard yarrow
#

Yeah this doesn't seem controversial given we're already taking a "write predicatble tests" stand.

solemn epoch
#

Reasons like this are why i prefer running tests in deployment env ea testnet

hard yarrow
#

Also, "unpredictable" tests are basically fuzzing, and we have a separate suite of functionality for contract devs to do fuzz tests.

#

It might even be more convenient if Address::random was just sequential address IDs. 1, 2, 3, 4...

#

But "random" hardly feels like the right term then. What would be a better term?

#

What I'd like to do:

  • Replace source account randomness with a hardcoded [0; 32] account.
  • Add Address::generate, deprecate/remove Address::random.
  • Keep Bytes::random, add a warning "yo creates unpredictable tests".

Alternatively, more drastic:

  • Remove Bytes::random.
  • Remove Address::random.

Wdyt?

split oxide
#

Bytes::random doesn't seem useful anymore

#

for the addresses... we need some utility, right? I'm pretty sure most of the time you don't care about particular address values

#

and it's also a nice tool to make it clear that addresses are an abstract concept and you don't need to worry e.g. about tying your logic to the classic accounts

hard yarrow
split oxide
#

sure, sequential generator also makes sense and also doesn't allow making any assumptions about the addresses

hard yarrow
#

The background context on why is because I'm trying to make it easy to record tests and detect observable changes in behavior over time, such as with an SDK upgrade or a protocol upgrade. Doing that with random inputs to every contract test is difficult.

#

Ok. I'll remove all the randoms. If someone really needs random bytes they can still import their own rand lib and create their own random bytes.

Random Addresses, I agree, don't seem to have utility as long as we replace them with something.

split oxide
#

sound good. FWIW using random has never been the end goal, it was just quick and dirty way to get an arbitrary valid address

sick sonnet
hard yarrow
solemn epoch
hard yarrow
#

Fyi I kept the BytesN::random fn, because I didn't really need to remove it to address the issue of not being able to write predictable tests.

#

Yes BytesN::random will make a test not predictable, but it's optional and if someone needs it they need it.