P-chain UTs consolidation 3 - Fork Handling #2169
Conversation
abi87
force-pushed
the
pchain_UTs_forks_consolidation
branch
from
44e4105 to
269be20
Compare
| @@ -164,8 +173,8 @@ func addSubnet(t *testing.T, env *environment) { | |||
| ts.Keys[1].PublicKey().Address(), | |||
| ts.Keys[2].PublicKey().Address(), | |||
| }, | |||
| []*secp256k1.PrivateKey{ts.Keys[0]}, | |||
| ts.Keys[0].PublicKey().Address(), | |||
| []*secp256k1.PrivateKey{ts.Keys[4]}, | |||
There was a problem hiding this comment.
we used to use key 0 to fund a subnet creation. This messes up the key 0 balance (it reduces its initial balanche ts.Balance of the subnet creation fee) which can be annoying in some UTs where we do balance comparison.
In such tests we rarely use key 4 instead, so it's easier to use key 4 here to fund the subnet creation and keep balance checks simpler
| @@ -216,13 +226,6 @@ func defaultState( | |||
| return state | |||
| } | |||
|
|
|||
| func defaultClock() *mockable.Clock { | |||
There was a problem hiding this comment.
moved to test setup package
| config: ts.Config(fork, forkTime), | ||
| clk: ts.Clock(forkTime), |
There was a problem hiding this comment.
so finally local clock and chain time are aligned and selected fork is active.
| @@ -183,6 +189,7 @@ func addSubnet(t *testing.T, env *environment) { | |||
|
|
|||
| stateDiff.AddTx(testSubnet1, status.Committed) | |||
| require.NoError(stateDiff.Apply(env.state)) | |||
| require.NoError(env.state.Commit()) | |||
There was a problem hiding this comment.
this commit is needed to properly consume utxos used to fund subnet creation. Without this, a tx creation may end up using consumed utxos (not properly stored on state) and fail.
The reason that didn't happen before is that subnet creation fee used to be set to zero, which masked the issue.
| @@ -87,7 +88,8 @@ func TestCreateChainTxWrongControlSig(t *testing.T) { | |||
| // Replace a valid signature with one from another key | |||
| sig, err := key.SignHash(hashing.ComputeHash256(tx.Unsigned.Bytes())) | |||
| require.NoError(err) | |||
| copy(tx.Creds[0].(*secp256k1fx.Credential).Sigs[0][:], sig) | |||
| sigPos := len(tx.Creds) - 1 | |||
There was a problem hiding this comment.
subnet creation fees are non zero, which implies that credentials changed position. Tx sign credential are always append last, so this change should make test less brittle
vms/platformvm/vm_regression_test.go
Outdated
| @@ -1123,7 +1129,14 @@ func TestValidatorSetAtCacheOverwriteRegression(t *testing.T) { | |||
| func TestAddDelegatorTxAddBeforeRemove(t *testing.T) { | |||
| require := require.New(t) | |||
|
|
|||
| validatorStartTime := banffForkTime.Add(executor.SyncBound).Add(1 * time.Second) | |||
There was a problem hiding this comment.
we don't want package-scoped fork times anymore.
Default fork times must be in test setup package; if a test needs a different fork time, it should scope it locally.
abi87
changed the title
abi87
changed the title
| @@ -63,30 +63,23 @@ import ( | |||
| txexecutor "github.com/ava-labs/avalanchego/vms/platformvm/txs/executor" | |||
| ) | |||
|
|
|||
| var ( | |||
| latestForkTime = genesistest.ValidateEndTime.Add(-5 * configtest.MinStakingDuration) | |||
There was a problem hiding this comment.
no global latestForkTime anymore. So we don't have to worry to reset it across UTs
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Why this should be merged
P-chain unit tests needs to be easier to maintain.
Specifically I currently find non-trivial to understand what specific fork a unit test is testing and possibly to update the tests to the latest fork.
Also fork times do not make sense and vm clock is not set to fullfil them.
This PR, centralizes and simplifies fork setting in UTs. It also fixes a few subtly broken UTs.
Thanks @joshua-kim and @marun for the discussions that brought me to these changes.
This PR replaces #1530, a very similar, previous attempt which lacked centralization of testing utilities in test setup package
How this works
Just refactor tests so that the latest active fork is explictly set upon vm creation and vm.clock is set to make the fork active.
How this was tested
Testing test is tricky. I guess I tested by manual inspection