test: chain: unit tests for the syncer & sync manager #8072
Conversation
Added a few tests for some uncovered methods of the syncer and sync manager. Deleted some dead code (sync.go:getLatestBeaconEntry).
Add annotations for the test crawler for the lotus.systemtestmatrix.com dashboard.
|
@ZenGround0 @magik6k Hey guy I have two test suites that are completely unrelated (or at least I think so) to my changes that are failing: test-itest-ccupgrade and test-itest-paych_api. |
|
@TheDivic thanks for the heads up. Looks like they are flaky. itest-ccupgrade should be salvageable, I'll look into it. paych_api I have less confidence in quick turn around but I'll also look. --edit-- |
| @@ -1244,25 +1244,3 @@ func (syncer *Syncer) CheckBadBlockCache(blk cid.Cid) (string, bool) { | |||
| bbr, ok := syncer.bad.Has(blk) | |||
| return bbr.String(), ok | |||
| } | |||
|
|
|||
| func (syncer *Syncer) getLatestBeaconEntry(ctx context.Context, ts *types.TipSet) (*types.BeaconEntry, error) { | |||
There was a problem hiding this comment.
@arajasek you seem to be the last person who touched this (+ our resident DRAND expert). Can you confirm that we don't want this anymore?
There was a problem hiding this comment.
Sorry about the delay here -- Yes, this can definitely be removed.
chain/sync_manager_test.go
Outdated
| ts2 := mock.TipSet(mock.MkBlock(ts1, 1, 0)) | ||
| bucket1 := newSyncTargetBucket(ts1, ts2) | ||
| bucketSet := syncBucketSet{buckets: []*syncTargetBucket{bucket1}} | ||
| fmt.Println("bucketSet: ", bucketSet.String()) |
There was a problem hiding this comment.
nit: these print statements can go
| require.NoError(tu.t, err) | ||
| require.Equal(tu.t, "", reason, "block is still bad after manually unmarking") | ||
|
|
||
| err = tu.nds[client].SyncUnmarkAllBad(tu.ctx) |
There was a problem hiding this comment.
I'm new to the lotus syncer so I'm curious: why do we need the explicit SyncUnmarkBad on the head if we are unmarking all here?
There was a problem hiding this comment.
I believe it's done intentionally, so as to check that both methods of unmarking a bad block are working as intended. In this way, if a change is commited to either SyncUnmarkBad or SyncUnmarkAllBad, the test will cover them.
While this test strays into the "integration" category, I think it's a valid choice.
Codecov Report
@@ Coverage Diff @@
## master #8072 +/- ##
==========================================
+ Coverage 39.91% 40.02% +0.10%
==========================================
Files 666 666
Lines 72554 72541 -13
==========================================
+ Hits 28963 29035 +72
+ Misses 38553 38461 -92
- Partials 5038 5045 +7
Continue to review full report at Codecov.
|
Added a few tests for some uncovered methods of the syncer and sync manager. Deleted some dead code.
Proposed Changes
sync.go:getLatestBeaconEntryto be dead code. I also checked for any references (didn't have to look far since it's a private method) and there aren't any, so I deleted it.sync.go) but I covered the last few methods that didn't have any unit tests.Additional Info
sync.go:getLatestBeaconEntryleft there on purpose?Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps