Add UT for Transaction PreExecute()
#1852
Conversation
| @@ -252,3 +334,146 @@ func TestSignRawActionBytesTx(t *testing.T) { | |||
| require.NoError(err) | |||
| require.Equal(signedTx.Bytes(), rawSignedTxBytes) | |||
| } | |||
|
|
|||
| func TestPreExecute(t *testing.T) { | |||
There was a problem hiding this comment.
(could be split to separate PR) - we should write unit tests for the pre executor type itself as well, since that is how PreExecute will be called from outside this package
chain/transaction_test.go
Outdated
| func (*auth1) ValidRange(_ chain.Rules) (int64, int64) { | ||
| return 1 * consts.MillisecondsPerSecond, 1 * consts.MillisecondsPerSecond | ||
| } |
There was a problem hiding this comment.
Can we set the valid range start/end as fields on auth1 and same for action3 ? This will make it easier to look at each individual test case and see immediately the start/end values rather than needing to know the start/end on the implementation above
chain/transaction_test.go
Outdated
| panic("unimplemented") | ||
| } | ||
|
|
||
| type mockAuth struct{} |
There was a problem hiding this comment.
could we rename this abstractMockAuth to match the implementation of the base mockAction above?
chain/transaction_test.go
Outdated
| tx: &chain.Transaction{ | ||
| TransactionData: chain.TransactionData{ | ||
| Base: &chain.Base{ | ||
| Timestamp: 62 * consts.MillisecondsPerSecond, |
There was a problem hiding this comment.
Can we set testRules above declaring the test cases and use (testRules.ValidityWindow + 1) * consts.MillisecondsPerSecond ?
This should make the test case more clear in that it's setting the timestamp to greater than the validity window rather than the reader needing to know the default value of 60
chain/transaction_test.go
Outdated
| TransactionData: chain.TransactionData{ | ||
| Base: &chain.Base{}, | ||
| Actions: func() []chain.Action { | ||
| actions := make([]chain.Action, 17) |
There was a problem hiding this comment.
Could we use a variable for the maximum number of actions here rather than requiring the reader to know that 16 is the maximum?
chain/transaction_test.go
Outdated
| panic("unimplemented") | ||
| } | ||
|
|
||
| type auth1 struct { |
There was a problem hiding this comment.
(optional for this PR) - the pattern of extending an abstract type that panics for every function is a bit annoying especially when we implement a new authX type for each new test.
Could we switch to use a single mock or test action/auth type that allows tests to set arbitrary values?
There was a problem hiding this comment.
Added to the following issue: #1860
There was a problem hiding this comment.
Added a single mock/auth type; kept mockTransferAction and action2 instead of solely using mockAction since these types have additional field required for codec-related tests
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: rodrigo <[email protected]>
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: rodrigo <[email protected]>
No description provided.