Add UTs for PreExecutor.PreExecute()
#1858
Conversation
RodrigoVillar
requested review from
aaronbuchwald and
joshua-kim
as code owners
chain/pre_executor_test.go
Outdated
| name: "raw fee doesn't exist", | ||
| err: database.ErrNotFound, | ||
| }, |
There was a problem hiding this comment.
Can we include a valid transaction for a test case like this where it will error before dereferencing the transaction?
If the code were to change the order of verification, but the raw fee was still missing, ideally the test case would still pass. By writing the test this way, it makes it brittle and it will fail if we make any ordering changes to the code itself.
There was a problem hiding this comment.
Same comment for remaining test cases. To avoid duplicate code, we could construct a single valid tx above the creation of the slice of test cases.
There was a problem hiding this comment.
Can we also add a more specific error, so that we won't confuse database.ErrNotFound from another part of the code with failing to fetch the raw fee?
chain/pre_executor_test.go
Outdated
| isRepeatError error | ||
| setBits []int |
There was a problem hiding this comment.
can we replace setBits with a value of type set.Bits ? Not sure there's any advantage to handling this within the mocked IsRepeat instead of leaving to the caller.
chain/pre_executor.go
Outdated
| if err != nil && err != database.ErrNotFound { | ||
| return err | ||
| } | ||
| if err == database.ErrNotFound { | ||
| return ErrFeeNotFound | ||
| } |
There was a problem hiding this comment.
Why differentiate these two errors? In this case, we can treat both the value not being present or any other error identically (no need to special case database.ErrNotFound here. Could we instead use fmt.Errorf("%w: %w", ErrFailedToFetchFee, err) so that we include the specific error we will want to test for and whatever error actually came from the view?
chain/pre_executor_test.go
Outdated
| type mockValidityWindow struct { | ||
| isRepeatError error | ||
| setBits set.Bits | ||
| } |
There was a problem hiding this comment.
Can we use the general purpose mock validity window added in #1875 ? This way we do not re-create the same mock at each test site.
chain/pre_executor_test.go
Outdated
| db, err := merkledb.New( | ||
| ctx, | ||
| memdb.New(), | ||
| merkledb.Config{ | ||
| BranchFactor: merkledb.BranchFactor16, | ||
| Tracer: trace.Noop, | ||
| }, | ||
| ) | ||
| r.NoError(err) | ||
|
|
||
| for k, v := range tt.state { | ||
| r.NoError(db.Put([]byte(k), v)) | ||
| } | ||
| r.NoError(db.CommitToDB(ctx)) |
There was a problem hiding this comment.
Do we need to create a merkledb instance here? I think we can just the state type taken by the pre-executor to state.Immutable so that we can simply use state.ImmutableStorage instead
There was a problem hiding this comment.
Switched the PreExecutor from taking in state.View to taking in state.Immutable
chain/pre_executor_test.go
Outdated
| ruleFactory := genesis.ImmutableRuleFactory{ | ||
| Rules: testRules, | ||
| } | ||
| validTX := &chain.Transaction{ |
There was a problem hiding this comment.
Can we camel case this ie. validTx ?
This PR adds UTs for the
PreExecute()method ofPreExecutor.