Issues with std.TestXYZ functions

[leohhhn]

While writing the concept & reference docs, I was playing around with many of the std functions, which in also include the functions to handle mocking of parameters within the test context used with gno test. For more context on the issue, gno test creates a test Gno Machine with a specific blockchain context here.

Below is a compilation of issues, inconsistencies, and naming choices that I found unintuitive, and that I suggest we refactor.

All of the functions I mention below can be found in the following files:
gnovm/tests/stdlibs/native.go
gnovm/tests/stdlibs/std/std.go
gnovm/tests/stdlibs/std/std.gno

TestCurrentRealm:

• Inconsistency: as opposed to the std.CurrentRealm() function, which returns a Realm object with both an address and path, std.TestCurrentRealm() only returns the string path of the realm.
• If called from a test within a /p/ package, the return value is nil. When reading the result, this causes a nil pointer dereferencepanic. This is because of how the function is implemented - it only fetches the Realm from the testing context, which in this case is non-existent as we're calling it in a package test. This is a design choice and it's fine, but we need more clear error messages on this - ie, the function could panic with "called in non-realm context" in case of being called from a package.

AssertOriginCall:

• IMO, this function is useless in the testing context - the assertions happen in the realm runtime context, and not in the testing context. I propose we completely remove this function from the testing context, ie from here.

TestSkipHeights:

• Can skip back into history, ie with negative numbers.
• No overflow checks, overflows with max int64 values.

TestAddress:

• This function is actually not a part of the std package. It is part of the testutils on-chain package. This could be migrated to the same place as the other functions for consistency, as it is a commonly used function.

TestSetOrigPkgAddr:

This function is super weird. It is supposed to set the address of the current realm. Currently there is no way to get the address of the current realm in the testing context, and the std.GetCurrentRealm() returns an address that differs from the one set with TestSetOrigPkgAddr. Here's a test demonstrating this in the playground.

IsOriginCall:

Minor, but it would be great if this function was refactored to remove the hardcoded frame values. Also, not sure we really need it, just like AssertOriginCall.

---

I am currently making a couple more issues relating to some findings, and will also post a general renaming discussion that will suggest how we should refactor the std API for mocking (or if we should move it to a different namespace altogether).

Can we also discuss changing how gno test works by implementing the testing through the VM Keeper? This would also enable us to use the Banker API without issues in tests.

cc @moul @zivkovicmilos @thehowl @waymobetta

[thehowl]

In the std refactor, I'm making some changes in how these test functions could work. This would also change the functioning of TestSetRealm you love so much.

Basically, the idea is to have test functions set up a context, then call Run() with a callback to run the callback having that context.

That way, everything can be cleanly simulated without having to set up 10 different TestSetX calls first.

gno/gnovm/stdlibs/testing/context.gno

Lines 9 to 21 in ef3b264

	type Context struct {
	IsOrigin bool
	CurrentRealm chain.Realm
	Deposit chain.Coins
	ChainID string
	Height uint64
	Time time.Time
	Banker chain.Banker
	Logger bool // TODO
	}
	func (c Context) Run(f func()) {
	}

To answer your points in the OP:

• TestCurrentRealm has been recently removed
• AssertOriginCall exists in the testing stdlibs to change its behaviour when testing; this is necessary so that functions calling AssertOriginCall can be tested; same reason why there is a second PrevRealm implementation
• TestSkipHeights: lol @ overflow, I'm trying to change the whole premise by having end-users be able to pass and modify both the height and the timestamp.
• TestAddress: let's see further on whether we want to add this; I'd prefer if we had the underlying function used by DerivePkgAddr to do this. (In feat: std re-organization #2425, I'm trying to move the bech32 implementation into its own package in crypto/bech32, so this will likely happen)
• TestSetOrigPkgAddr: yeah, it's weird, I'll probably remove it.
• IsOriginCall: with the discussions on AssertOriginCall, this is likely to be removed. Also in scope for feat: std re-organization #2425.

[thehowl]

centralizing on #1475