Skip to content

Commit 3db6aca

Browse files
julienrbrtjulienrbrt
committed
updates
1 parent e5dcf6b commit 3db6aca

5 files changed

Lines changed: 35 additions & 56 deletions

File tree

‎block/aggregation_test.go‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,10 @@ func TestAggregationLoop_Normal_BasicInterval(t *testing.T) {
118118
// TestAggregationLoop_Normal_PublishBlockError verifies that the aggregation loop handles errors from publishBlock gracefully.
119119
func TestAggregationLoop_Normal_PublishBlockError(t *testing.T) {
120120
t.Parallel()
121-
assert := assert.New(t)
122121
require := require.New(t)
123122

124123
blockTime := 50 * time.Millisecond
125124
waitTime := blockTime*4 + blockTime/2
126-
tolerance := blockTime / 2
127125

128126
mockStore := mocks.NewStore(t)
129127
mockStore.On("Height", mock.Anything).Return(uint64(1), nil).Maybe()
@@ -185,10 +183,12 @@ func TestAggregationLoop_Normal_PublishBlockError(t *testing.T) {
185183

186184
ctx, cancel := context.WithCancel(context.Background())
187185
var wg sync.WaitGroup
186+
errCh := make(chan error, 1)
187+
188188
wg.Add(1)
189189
go func() {
190190
defer wg.Done()
191-
m.AggregationLoop(ctx, make(chan<- error))
191+
m.AggregationLoop(ctx, errCh)
192192
m.logger.Info("AggregationLoop exited")
193193
}()
194194

@@ -201,12 +201,7 @@ func TestAggregationLoop_Normal_PublishBlockError(t *testing.T) {
201201
defer publishLock.Unlock()
202202

203203
calls := publishCalls.Load()
204-
assert.GreaterOrEqualf(calls, int64(4), "publishBlock should have been called multiple times (around 4), but was called %d times", calls)
205-
assert.LessOrEqualf(calls, int64(5), "publishBlock should have been called multiple times (around 4-5), but was called %d times", calls)
206-
207-
require.GreaterOrEqual(len(publishTimes), 3, "Need at least 3 timestamps to check intervals after error")
208-
for i := 2; i < len(publishTimes); i++ {
209-
interval := publishTimes[i].Sub(publishTimes[i-1])
210-
WithinDuration(t, blockTime, interval, tolerance)
211-
}
204+
require.Equal(calls, int64(1))
205+
require.ErrorContains(<-errCh, expectedErr.Error())
206+
require.Equal(len(publishTimes), 1, "Expected only one publish time after error")
212207
}

‎block/lazy_aggregation_test.go‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ func TestLazyAggregationLoop_LazyTimerTrigger(t *testing.T) {
163163
wg.Wait()
164164
}
165165

166-
// TestLazyAggregationLoop_PublishError tests that the loop continues after a publish error.
166+
// TestLazyAggregationLoop_PublishError tests that the loop exits.
167167
func TestLazyAggregationLoop_PublishError(t *testing.T) {
168168
t.Parallel()
169169
require := require.New(t)
@@ -176,7 +176,7 @@ func TestLazyAggregationLoop_PublishError(t *testing.T) {
176176
pubMock.err = errors.New("publish failed")
177177
pubMock.mu.Unlock()
178178

179-
ctx, cancel := context.WithCancel(context.Background())
179+
ctx, cancel := context.WithCancel(t.Context())
180180
defer cancel()
181181

182182
var wg sync.WaitGroup
@@ -196,20 +196,8 @@ func TestLazyAggregationLoop_PublishError(t *testing.T) {
196196
require.Fail("timed out waiting for first block publication attempt")
197197
}
198198

199-
// Remove the error for subsequent calls
200-
pubMock.mu.Lock()
201-
pubMock.err = nil
202-
pubMock.mu.Unlock()
203-
204-
// Wait for the second publish attempt (should succeed)
205-
// Use a longer timeout since we need to wait for either the lazy timer or block timer to fire
206-
select {
207-
case <-pubMock.calls:
208-
case <-time.After(2 * lazyTime): // Use the longer of the two timers with some buffer
209-
require.Fail("timed out waiting for second block publication attempt after error")
210-
}
199+
// loop exited, nothing to do.
211200

212-
cancel()
213201
wg.Wait()
214202
}
215203

‎block/manager.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,7 @@ func (m *Manager) isUsingExpectedSingleSequencer(header *types.SignedHeader) boo
499499

500500
// publishBlockInternal is the internal implementation for publishing a block.
501501
// It's assigned to the publishBlock field by default.
502+
// Any error will be returned, unless the error is due to a publishing error.
502503
func (m *Manager) publishBlockInternal(ctx context.Context) error {
503504
select {
504505
case <-ctx.Done():
@@ -507,7 +508,7 @@ func (m *Manager) publishBlockInternal(ctx context.Context) error {
507508
}
508509

509510
if m.config.Node.MaxPendingHeaders != 0 && m.pendingHeaders.numPendingHeaders() >= m.config.Node.MaxPendingHeaders {
510-
m.logger.Warn("refusing to create block: pending blocks [%d] reached limit [%d]", m.pendingHeaders.numPendingHeaders(), m.config.Node.MaxPendingHeaders)
511+
m.logger.Warn(fmt.Sprintf("refusing to create block: pending blocks [%d] reached limit [%d]", m.pendingHeaders.numPendingHeaders(), m.config.Node.MaxPendingHeaders))
511512
return nil
512513
}
513514

‎block/publish_block_test.go‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package block
22

33
import (
4+
"bytes"
45
"context"
56
"encoding/binary"
67
"errors"
@@ -27,7 +28,12 @@ import (
2728
)
2829

2930
// setupManagerForPublishBlockTest creates a Manager instance with mocks for testing publishBlockInternal.
30-
func setupManagerForPublishBlockTest(t *testing.T, isProposer bool, initialHeight uint64, lastSubmittedHeight uint64) (
31+
func setupManagerForPublishBlockTest(
32+
t *testing.T,
33+
initialHeight uint64,
34+
lastSubmittedHeight uint64,
35+
logBuffer *bytes.Buffer,
36+
) (
3137
*Manager,
3238
*mocks.Store,
3339
*mocks.Executor,
@@ -57,7 +63,10 @@ func setupManagerForPublishBlockTest(t *testing.T, isProposer bool, initialHeigh
5763
headerCh := make(chan *types.SignedHeader, 1)
5864
dataCh := make(chan *types.Data, 1)
5965
_, cancel := context.WithCancel(context.Background())
60-
logger := log.NewTestLogger(t)
66+
logger := log.NewLogger(
67+
logBuffer,
68+
log.ColorOption(false),
69+
)
6170

6271
lastSubmittedBytes := make([]byte, 8)
6372
binary.LittleEndian.PutUint64(lastSubmittedBytes, lastSubmittedHeight)
@@ -109,14 +118,14 @@ func setupManagerForPublishBlockTest(t *testing.T, isProposer bool, initialHeigh
109118
// TestPublishBlockInternal_MaxPendingHeadersReached verifies that publishBlockInternal returns an error if the maximum number of pending headers is reached.
110119
func TestPublishBlockInternal_MaxPendingHeadersReached(t *testing.T) {
111120
t.Parallel()
112-
assert := assert.New(t)
113121
require := require.New(t)
114122

115123
currentHeight := uint64(10)
116124
lastSubmitted := uint64(5)
117125
maxPending := uint64(5)
126+
logBuffer := new(bytes.Buffer)
118127

119-
manager, mockStore, mockExec, mockSeq, _, _, _, cancel := setupManagerForPublishBlockTest(t, true, currentHeight+1, lastSubmitted)
128+
manager, mockStore, mockExec, mockSeq, _, _, _, cancel := setupManagerForPublishBlockTest(t, currentHeight+1, lastSubmitted, logBuffer)
120129
defer cancel()
121130

122131
manager.config.Node.MaxPendingHeaders = maxPending
@@ -126,8 +135,8 @@ func TestPublishBlockInternal_MaxPendingHeadersReached(t *testing.T) {
126135

127136
err := manager.publishBlock(ctx)
128137

129-
require.Error(err, "publishBlockInternal should return an error")
130-
assert.Contains(err.Error(), "pending blocks [5] reached limit [5]", "error message mismatch")
138+
require.Nil(err, "publishBlockInternal should not return an error (otherwise the chain would halt)")
139+
require.Contains(logBuffer.String(), "pending blocks [5] reached limit [5]", "log message mismatch")
131140

132141
mockStore.AssertExpectations(t)
133142
mockExec.AssertNotCalled(t, "GetTxs", mock.Anything)
@@ -336,7 +345,7 @@ func Test_publishBlock_Success(t *testing.T) {
336345
newHeight := initialHeight + 1
337346
chainID := "testchain"
338347

339-
manager, mockStore, mockExec, mockSeq, _, headerCh, dataCh, _ := setupManagerForPublishBlockTest(t, true, initialHeight, 0)
348+
manager, mockStore, mockExec, mockSeq, _, headerCh, dataCh, _ := setupManagerForPublishBlockTest(t, initialHeight, 0, new(bytes.Buffer))
340349
manager.lastState.LastBlockHeight = initialHeight
341350

342351
mockStore.On("Height", t.Context()).Return(initialHeight, nil).Once()

‎block/sync_test.go‎

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package block
33
import (
44
"context"
55
"errors"
6-
"fmt"
76
"sync"
87
"sync/atomic"
98
"testing"
@@ -647,8 +646,9 @@ func TestSyncLoop_IgnoreDuplicateEvents(t *testing.T) {
647646
assert.Nil(m.dataCache.GetItem(heightH1), "Data cache should be cleared for H+1")
648647
}
649648

650-
// TestSyncLoop_PanicOnApplyError verifies that the SyncLoop panics if ApplyBlock fails.
651-
func TestSyncLoop_PanicOnApplyError(t *testing.T) {
649+
// TestSyncLoop_ErrorOnApplyError verifies that the SyncLoop halts if ApplyBlock fails.
650+
// Halting after sync loop error is handled in full.go and is not tested here.
651+
func TestSyncLoop_ErrorOnApplyError(t *testing.T) {
652652
assert := assert.New(t)
653653
require := require.New(t)
654654

@@ -677,7 +677,6 @@ func TestSyncLoop_PanicOnApplyError(t *testing.T) {
677677
}
678678

679679
applyErrorSignal := make(chan struct{})
680-
panicCaughtSignal := make(chan struct{})
681680
applyError := errors.New("apply failed")
682681

683682
// --- Mock Expectations ---
@@ -692,19 +691,12 @@ func TestSyncLoop_PanicOnApplyError(t *testing.T) {
692691

693692
var wg sync.WaitGroup
694693
wg.Add(1)
694+
695+
errCh := make(chan error, 1)
696+
695697
go func() {
696698
defer wg.Done()
697-
defer func() {
698-
if r := recover(); r != nil {
699-
t.Logf("Caught expected panic: %v", r)
700-
assert.Contains(fmt.Sprintf("%v", r), applyError.Error(), "Panic message should contain the original error")
701-
close(panicCaughtSignal)
702-
} else {
703-
t.Error("SyncLoop did not panic as expected")
704-
}
705-
}()
706-
m.SyncLoop(ctx, make(chan<- error))
707-
t.Log("SyncLoop exited.")
699+
m.SyncLoop(ctx, errCh)
708700
}()
709701

710702
// --- Send Events ---
@@ -722,13 +714,7 @@ func TestSyncLoop_PanicOnApplyError(t *testing.T) {
722714
t.Fatal("Timeout waiting for ApplyBlock error signal")
723715
}
724716

725-
t.Log("Waiting for panic to be caught...")
726-
select {
727-
case <-panicCaughtSignal:
728-
t.Log("Panic caught.")
729-
case <-time.After(2 * time.Second):
730-
t.Fatal("Timeout waiting for panic signal")
731-
}
717+
require.Error(<-errCh, "SyncLoop should return an error when ApplyBlock errors")
732718

733719
wg.Wait()
734720

0 commit comments

Comments
 (0)