Skip to content

Commit 7e1014b

Browse files
qdm12qdm12
authored
fix(dot/sync): Fix flaky tests Test_chainSync_logSyncSpeed and Test_chainSync_start (#2610)
1 parent c061b35 commit 7e1014b

2 files changed

Lines changed: 52 additions & 55 deletions

File tree

‎dot/sync/chain_sync.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,10 @@ type chainSync struct {
162162
maxWorkerRetries uint16
163163
slotDuration time.Duration
164164

165-
logSyncPeriod time.Duration
165+
logSyncTicker *time.Ticker
166+
logSyncTickerC <-chan time.Time // channel as field for unit testing
167+
logSyncStarted bool
168+
logSyncDone chan struct{}
166169
}
167170

168171
type chainSyncConfig struct {
@@ -178,6 +181,8 @@ func newChainSync(cfg *chainSyncConfig) *chainSync {
178181
ctx, cancel := context.WithCancel(context.Background())
179182
const syncSamplesToKeep = 30
180183
const logSyncPeriod = 5 * time.Second
184+
logSyncTicker := time.NewTicker(logSyncPeriod)
185+
181186
return &chainSync{
182187
ctx: ctx,
183188
cancel: cancel,
@@ -197,7 +202,9 @@ func newChainSync(cfg *chainSyncConfig) *chainSync {
197202
minPeers: cfg.minPeers,
198203
maxWorkerRetries: uint16(cfg.maxPeers),
199204
slotDuration: cfg.slotDuration,
200-
logSyncPeriod: logSyncPeriod,
205+
logSyncTicker: logSyncTicker,
206+
logSyncTickerC: logSyncTicker.C,
207+
logSyncDone: make(chan struct{}),
201208
}
202209
}
203210

@@ -219,6 +226,7 @@ func (cs *chainSync) start() {
219226
cs.pendingBlockDoneCh = pendingBlockDoneCh
220227
go cs.pendingBlocks.run(pendingBlockDoneCh)
221228
go cs.sync()
229+
cs.logSyncStarted = true
222230
go cs.logSyncSpeed()
223231
}
224232

@@ -227,6 +235,9 @@ func (cs *chainSync) stop() {
227235
close(cs.pendingBlockDoneCh)
228236
}
229237
cs.cancel()
238+
if cs.logSyncStarted {
239+
<-cs.logSyncDone
240+
}
230241
}
231242

232243
func (cs *chainSync) syncState() chainSyncState {
@@ -333,8 +344,8 @@ func (cs *chainSync) setPeerHead(p peer.ID, hash common.Hash, number uint) error
333344
}
334345

335346
func (cs *chainSync) logSyncSpeed() {
336-
t := time.NewTicker(cs.logSyncPeriod)
337-
defer t.Stop()
347+
defer close(cs.logSyncDone)
348+
defer cs.logSyncTicker.Stop()
338349

339350
for {
340351
before, err := cs.blockState.BestBlockHeader()
@@ -347,10 +358,7 @@ func (cs *chainSync) logSyncSpeed() {
347358
}
348359

349360
select {
350-
case <-t.C:
351-
if cs.ctx.Err() != nil {
352-
return
353-
}
361+
case <-cs.logSyncTickerC: // channel of cs.logSyncTicker
354362
case <-cs.ctx.Done():
355363
return
356364
}

‎dot/sync/chain_sync_test.go‎

Lines changed: 36 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1118,7 +1118,7 @@ func Test_chainSync_logSyncSpeed(t *testing.T) {
11181118

11191119
type fields struct {
11201120
blockStateBuilder func(ctrl *gomock.Controller) BlockState
1121-
networkBuilder func(ctrl *gomock.Controller, done chan struct{}) Network
1121+
networkBuilder func(ctrl *gomock.Controller) Network
11221122
state chainSyncState
11231123
benchmarker *syncBenchmarker
11241124
}
@@ -1131,16 +1131,13 @@ func Test_chainSync_logSyncSpeed(t *testing.T) {
11311131
fields: fields{
11321132
blockStateBuilder: func(ctrl *gomock.Controller) BlockState {
11331133
mockBlockState := NewMockBlockState(ctrl)
1134-
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).AnyTimes()
1134+
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).Times(3)
11351135
mockBlockState.EXPECT().GetHighestFinalisedHeader().Return(&types.Header{}, nil)
11361136
return mockBlockState
11371137
},
1138-
networkBuilder: func(ctrl *gomock.Controller, done chan struct{}) Network {
1138+
networkBuilder: func(ctrl *gomock.Controller) Network {
11391139
mockNetwork := NewMockNetwork(ctrl)
1140-
mockNetwork.EXPECT().Peers().DoAndReturn(func() error {
1141-
close(done)
1142-
return nil
1143-
})
1140+
mockNetwork.EXPECT().Peers().Return(nil)
11441141
return mockNetwork
11451142
},
11461143
benchmarker: newSyncBenchmarker(10),
@@ -1152,16 +1149,13 @@ func Test_chainSync_logSyncSpeed(t *testing.T) {
11521149
fields: fields{
11531150
blockStateBuilder: func(ctrl *gomock.Controller) BlockState {
11541151
mockBlockState := NewMockBlockState(ctrl)
1155-
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).AnyTimes()
1152+
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).Times(3)
11561153
mockBlockState.EXPECT().GetHighestFinalisedHeader().Return(&types.Header{}, nil)
11571154
return mockBlockState
11581155
},
1159-
networkBuilder: func(ctrl *gomock.Controller, done chan struct{}) Network {
1156+
networkBuilder: func(ctrl *gomock.Controller) Network {
11601157
mockNetwork := NewMockNetwork(ctrl)
1161-
mockNetwork.EXPECT().Peers().DoAndReturn(func() error {
1162-
close(done)
1163-
return nil
1164-
})
1158+
mockNetwork.EXPECT().Peers().Return(nil)
11651159
return mockNetwork
11661160
},
11671161
benchmarker: newSyncBenchmarker(10),
@@ -1175,19 +1169,24 @@ func Test_chainSync_logSyncSpeed(t *testing.T) {
11751169
t.Parallel()
11761170
ctrl := gomock.NewController(t)
11771171
ctx, cancel := context.WithCancel(context.Background())
1178-
done := make(chan struct{})
1172+
tickerChannel := make(chan time.Time)
11791173
cs := &chainSync{
1180-
ctx: ctx,
1181-
cancel: cancel,
1182-
blockState: tt.fields.blockStateBuilder(ctrl),
1183-
network: tt.fields.networkBuilder(ctrl, done),
1184-
state: tt.fields.state,
1185-
benchmarker: tt.fields.benchmarker,
1186-
logSyncPeriod: time.Millisecond,
1174+
ctx: ctx,
1175+
cancel: cancel,
1176+
blockState: tt.fields.blockStateBuilder(ctrl),
1177+
network: tt.fields.networkBuilder(ctrl),
1178+
state: tt.fields.state,
1179+
benchmarker: tt.fields.benchmarker,
1180+
logSyncTickerC: tickerChannel,
1181+
logSyncTicker: time.NewTicker(time.Hour), // just here to be stopped
1182+
logSyncDone: make(chan struct{}),
11871183
}
1184+
11881185
go cs.logSyncSpeed()
1189-
<-done
1190-
cancel()
1186+
1187+
tickerChannel <- time.Time{}
1188+
cs.cancel()
1189+
<-cs.logSyncDone
11911190
})
11921191
}
11931192
}
@@ -1197,10 +1196,8 @@ func Test_chainSync_start(t *testing.T) {
11971196

11981197
type fields struct {
11991198
blockStateBuilder func(ctrl *gomock.Controller) BlockState
1200-
disjointBlockSetBuilder func(ctrl *gomock.Controller) DisjointBlockSet
1201-
networkBuilder func(ctrl *gomock.Controller, done chan struct{}) Network
1199+
disjointBlockSetBuilder func(ctrl *gomock.Controller, called chan<- struct{}) DisjointBlockSet
12021200
benchmarker *syncBenchmarker
1203-
slotDuration time.Duration
12041201
}
12051202
tests := []struct {
12061203
name string
@@ -1211,26 +1208,18 @@ func Test_chainSync_start(t *testing.T) {
12111208
fields: fields{
12121209
blockStateBuilder: func(ctrl *gomock.Controller) BlockState {
12131210
mockBlockState := NewMockBlockState(ctrl)
1214-
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).AnyTimes()
1215-
mockBlockState.EXPECT().GetHighestFinalisedHeader().Return(&types.Header{}, nil)
1216-
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil).AnyTimes()
1211+
mockBlockState.EXPECT().BestBlockHeader().Return(&types.Header{}, nil)
12171212
return mockBlockState
12181213
},
1219-
disjointBlockSetBuilder: func(ctrl *gomock.Controller) DisjointBlockSet {
1214+
disjointBlockSetBuilder: func(ctrl *gomock.Controller, called chan<- struct{}) DisjointBlockSet {
12201215
mockDisjointBlockSet := NewMockDisjointBlockSet(ctrl)
1221-
mockDisjointBlockSet.EXPECT().run(gomock.Any())
1216+
mockDisjointBlockSet.EXPECT().run(gomock.AssignableToTypeOf(make(<-chan struct{}))).
1217+
DoAndReturn(func(stop <-chan struct{}) {
1218+
close(called) // test glue, ideally we would use a ready chan struct passed to run().
1219+
})
12221220
return mockDisjointBlockSet
12231221
},
1224-
networkBuilder: func(ctrl *gomock.Controller, done chan struct{}) Network {
1225-
mockNetwork := NewMockNetwork(ctrl)
1226-
mockNetwork.EXPECT().Peers().DoAndReturn(func() []common.PeerInfo {
1227-
close(done)
1228-
return nil
1229-
})
1230-
return mockNetwork
1231-
},
1232-
slotDuration: defaultSlotDuration,
1233-
benchmarker: newSyncBenchmarker(1),
1222+
benchmarker: newSyncBenchmarker(1),
12341223
},
12351224
},
12361225
}
@@ -1240,19 +1229,19 @@ func Test_chainSync_start(t *testing.T) {
12401229
t.Parallel()
12411230
ctrl := gomock.NewController(t)
12421231
ctx, cancel := context.WithCancel(context.Background())
1243-
done := make(chan struct{})
1232+
disjointBlockSetCalled := make(chan struct{})
12441233
cs := &chainSync{
12451234
ctx: ctx,
12461235
cancel: cancel,
12471236
blockState: tt.fields.blockStateBuilder(ctrl),
1248-
pendingBlocks: tt.fields.disjointBlockSetBuilder(ctrl),
1249-
network: tt.fields.networkBuilder(ctrl, done),
1237+
pendingBlocks: tt.fields.disjointBlockSetBuilder(ctrl, disjointBlockSetCalled),
12501238
benchmarker: tt.fields.benchmarker,
1251-
slotDuration: tt.fields.slotDuration,
1252-
logSyncPeriod: time.Second,
1239+
slotDuration: time.Hour,
1240+
logSyncTicker: time.NewTicker(time.Hour), // just here to be closed
1241+
logSyncDone: make(chan struct{}),
12531242
}
12541243
cs.start()
1255-
<-done
1244+
<-disjointBlockSetCalled
12561245
cs.stop()
12571246
})
12581247
}

0 commit comments

Comments
 (0)