chore(state-transition): cleaned up state in state-transition UTs #2518
Conversation
| @@ -58,23 +58,24 @@ type ( | |||
| TestStateProcessorT = core.StateProcessor | |||
| ) | |||
|
|
|||
| type testKVStoreService struct { | |||
| ctx sdk.Context | |||
There was a problem hiding this comment.
this was a first attempt at creating state but it is wrong.
We should use the context provided by the transactions and extract the state frorm there, not use and reuse a single context.
It worked so far because we never handled rejected blocks or multiple blocks for the same height, which we need in upcoming tests.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2518 +/- ##
=======================================
Coverage 32.30% 32.30%
=======================================
Files 350 350
Lines 15673 15674 +1
Branches 20 20
=======================================
+ Hits 5063 5064 +1
Misses 10247 10247
Partials 363 363
|
| @@ -135,7 +138,7 @@ func SetupTestState(t *testing.T, cs chain.Spec) ( | |||
| ) | |||
|
|
|||
| ctx := transition.NewTransitionCtx( | |||
| context.Background(), | |||
| ms, | |||
There was a problem hiding this comment.
this is important because the state passed to state_processor is extract from an sdk.Context. If we use any ordinary context.Context tests would panic.
| beaconState := statedb.NewBeaconStateFromDB(kvStore, cs) | ||
|
|
||
| ms := sdk.NewContext(cms.CacheMultiStore(), true, log.NewNopLogger()) | ||
| beaconState := statedb.NewBeaconStateFromDB(kvStore.WithContext(ms), cs) |
There was a problem hiding this comment.
this beaconState is basically what storage.StateFromContext does.
I should consider refactoring it out to make it clear
| return storage.NewKVStore(store) | ||
| } | ||
|
|
||
| //nolint:gochecknoglobals // unexported and use only in tests | ||
| var testStoreKey = storetypes.NewKVStoreKey("state-transition-tests") | ||
|
|
||
| func initTestStores() (*beacondb.KVStore, *depositstore.KVStore, error) { | ||
| func BuildTestStores() ( |
There was a problem hiding this comment.
pre-emptively exporting BuildTestStores which I am going to use in other packages
| return storage.NewKVStore(store) | ||
| } | ||
|
|
||
| //nolint:gochecknoglobals // unexported and use only in tests | ||
| //nolint:gochecknoglobals // unexported and used only in tests | ||
| var testStoreKey = storetypes.NewKVStoreKey("state-transition-tests") |
There was a problem hiding this comment.
What is the purpose of this special key? iirc the key by default in prod is beacond
There was a problem hiding this comment.
@rezbera I could have reused the prod one, but when I first built these tests I wanted to make sure I fully controlled them and I could use a different one.
One interesting point here is that Comet requires this very key to work: if you use this key in MountStoreWithDB and copy it in OpenKVStore, it won't be able to access the state with the copy.
This is something I would not have found out if I had not tried to use a test key
I agree. I made a single PR out of this just to make tests follow prod in the way state is handled (although not showing it by other tests, one has to take my word or dig into prod). I hope to come up soon with tests where multiple, independent states must be created on top of a finalized state, in which case I really need this PR and the caches it introduces, to make the tests independent. |
Made state handling closer to what we have in production.
I need this to keep building unit tests in both state-transition (see ongoing #2516) or other packages (validators APIs, not yet pushed)