refactor(dashboard): added unit tests for dashboard helpers #1309
Conversation
sasha-bitfly
changed the title
sasha-bitfly
force-pushed
the
BEDS-1006/unit-tests-for-dashboard-helpers
branch
from
59a3c87 to
3e13c77
Compare
| var result T | ||
| resultType := reflect.TypeOf(result) | ||
|
|
||
| if resultType != nil && resultType.Kind() == reflect.Slice { | ||
| err = db.SelectContext(ctx, &result, query, args...) | ||
| } else { | ||
| err = db.GetContext(ctx, &result, query, args...) | ||
| } |
There was a problem hiding this comment.
Issue: I'd do 2 separate function for select and get rather than doing a runtime type check for every query, since reflecting does have a performance impact.
There was a problem hiding this comment.
Also we're gonna need something similar for ExecContext, right? executeQuery as a func name might be a bit misleading then.
| return 0, err | ||
| } | ||
|
|
||
| query := fmt.Sprintf(`SELECT max(%s) FROM %s`, dateColumn, view) |
There was a problem hiding this comment.
Question: shouldn't we use goqu for all queries?
| validatorCountMap := processPastSyncCommitteesResults(validatorIndices) | ||
| return validatorCountMap, nil |
There was a problem hiding this comment.
Nitpick (non-blocking):
| validatorCountMap := processPastSyncCommitteesResults(validatorIndices) | |
| return validatorCountMap, nil | |
| return processPastSyncCommitteesResults(validatorIndices) | |
| , nil |
| pastSyncPeriodCutoff := utils.SyncPeriodOfEpoch(epochStart) | ||
| currentSyncPeriod := utils.SyncPeriodOfEpoch(latestEpoch) |
There was a problem hiding this comment.
Question: You have a bit of logic in your build query func? Do we want this? Or should we maybe pass this to the builder func?
| }, | ||
| }, | ||
| } | ||
| utils.Config = &cfg |
There was a problem hiding this comment.
Issue: We should not rely on global states in our tests. The data access service struct already has a specific field for its config and should ONLY read from that. We should get rid of global state reliance ASAP
| mockDB, mock, err := sqlmock.New() | ||
| assert.NoError(t, err) | ||
| defer mockDB.Close() | ||
|
|
||
| cfg := types.Config{ | ||
| Chain: types.Chain{ | ||
| ClConfig: types.ClChainConfig{ | ||
| EpochsPerSyncCommitteePeriod: 32, | ||
| AltairForkEpoch: 0, | ||
| }, | ||
| }, | ||
| } | ||
| utils.Config = &cfg | ||
|
|
||
| sqlxDB := sqlx.NewDb(mockDB, "sqlmock") | ||
| dataService := &DataAccessService{ | ||
| alloyReader: sqlxDB, | ||
| } |
There was a problem hiding this comment.
Issue: All this test setup should be in it's own function, otherwise we'll end up with a lot of repeated code across our tests. Keep in mind that the DataAccessService struct already has a Close() function implemented, which closes all of it's databases in a nil safe manner. I imagine that, ideally, we'd end up with a test setup like
func dataAccesesTestSetup(/* whatever params you need */) (service *DataAccessService, closeFunc func()) {
// setup everything
return dataAccessService, dataAccessService.Close
}// inside the actual test func
dataAccessService, close := dataAccesesTestSetup(/* whatever params you need */)
defer close() // calls close of the data access service itself| tests := []struct { | ||
| name string | ||
| period enums.TimePeriod | ||
| mockRows *sqlmock.Rows | ||
| mockError error | ||
| expectedVal uint64 | ||
| expectedError string | ||
| }{ |
There was a problem hiding this comment.
Thought (non-blocking): Since all of these mockTests use alot of similar fields, I'm wondering if we could maybe generalize some of this. E.g. a lot of the lines for a single test case iteration are repeated multiple times (setting the mock error, checking for expectedError, asserting exectedVal etc.)
sasha-bitfly
force-pushed
the
BEDS-1006/unit-tests-for-dashboard-helpers
branch
from
cf5f0d8 to
3e13c77
Compare
Deploying beaconchain with
|
| Latest commit: |
3e13c77
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bb8b150d.beaconchain.pages.dev |
| Branch Preview URL: | https://beds-1006-unit-tests-for-das.beaconchain.pages.dev |
Before we cover functions like
GetValidatorDashboardSummaryorGetValidatorDashboardGroupSummarywith unit tests we need to cleanup and refactor a codebase.Helper functions to be moved to
vdb_helpers.goto avoid mess invdb_dashboard.goEvery function is split into build query, execute query, process results
Generic
executeQueryfunction is added