test: expand stateful tests to test all stateful RPCs#259
test: expand stateful tests to test all stateful RPCs#259JonathanOppenheimer wants to merge 5 commits intomainfrom
Conversation
JonathanOppenheimer
requested review from
ARR4N and
StephenButtolph
as code owners
JonathanOppenheimer
added
the
DO NOT MERGE
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/nonce-rpcs
branch
2 times, most recently
from
daea4bb to
97f08b5
Compare
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/stateful-rpc-tests
branch
from
c36f3fa to
b0a9eb6
Compare
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/nonce-rpcs
branch
from
97f08b5 to
eac6cee
Compare
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/stateful-rpc-tests
branch
from
b0a9eb6 to
82407b0
Compare
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/nonce-rpcs
branch
from
204b3cd to
e8c58cf
Compare
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/stateful-rpc-tests
branch
from
82407b0 to
54ee789
Compare
| assert.Equal(t, uint256.NewInt(val).PaddedBytes(32), got) | ||
| blockNum := big.NewInt(int64(tt.num)) | ||
|
|
||
| t.Run("eth_call", func(t *testing.T) { |
There was a problem hiding this comment.
Does it make sense to have all of these subtests in the same test? I think the answer is no, since a lot of these do not need the infrastrucure required by others
There was a problem hiding this comment.
wdym? these all require the sut. are you referring to the last two?
There was a problem hiding this comment.
Talked in person. I personally think leaving all of the appropriate tests in the loop so they can share the block test cases make sense, but I will split out the two outliers. Would appreciate other opinions.
There was a problem hiding this comment.
Actually, hm I still don't like doing this because, then I need to literally 1-1 triply duplicate escrowAddr, recv, sign, and b.
Thoughts?
There was a problem hiding this comment.
The single-test approach is the right call here I think. All 7 RPCs need identical expensive setup (VM init -> deploy -> execute -> settle -> evict), and the block-number (in-memory vs on-disk) is the shared property being tested. Splitting would either duplicate the setup code or if we do a shared helper it would carry around 10 values back via a struct which is more indirection for no gain, since each test function would collapse to a trivial few lines wrapper. Seeing all RPCs side by side is easier to read. The two outliers (eth_estimateGas, eth_createAccessList) are correctly outside the loop with a clear comment.
There was a problem hiding this comment.
Got it! Appreciate the vote of confidence haha.
JonathanOppenheimer
removed
the
DO NOT MERGE
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
|
Replaced by ava-labs/avalanchego#5283 |
3 participants
This PR add RPC-level tests for 6 state query methods:
eth_getBalance,eth_getCode,eth_getStorageAt,eth_getProof,eth_estimateGas, andeth_createAccessList. I decided it would be cleanest if all of these RPC's just inhabited the same test, which I renamed toTestStatefulRPCs. This extension allows us to ensure each method is tested against both in-memory and on-disk block paths.Closes ava-labs/avalanchego#5244