Merge pull request #7314 from yyforyongyu/fix-unit-test-mailbox

htlcswitch: fix test flake in `TestMailBoxAddExpiry`

.golangci.yml

@@ -238,3 +238,10 @@ issues:
         - govet
         # itest case can be very long so we disable long function check.
         - funlen
+
+    - path: lnmock/*
+      linters:
+        # forcetypeassert is skipped for the mock because the test would fail
+        # if the returned value doesn't match the type, so there's no need to
+        # check the convert.
+        - forcetypeassert

docs/release-notes/release-notes-0.16.0.md

@@ -205,6 +205,9 @@ certain large transactions](https://github.com/lightningnetwork/lnd/pull/7100).
 * [Code style cleanup](https://github.com/lightningnetwork/lnd/pull/7308) in the
   funding package.
 
+* [Fixed a flake in the TestMailBoxAddExpiry unit
+  test](https://github.com/lightningnetwork/lnd/pull/7314).
+
 ## `lncli`
 
 * [Add an `insecure` flag to skip tls auth as well as a `metadata` string slice

htlcswitch/circuit_map.go

@@ -935,7 +935,7 @@ type Keystone struct {
 }
 
 // String returns a human readable description of the Keystone.
-func (k *Keystone) String() string {
+func (k Keystone) String() string {
 	return fmt.Sprintf("%s --> %s", k.InKey, k.OutKey)
 }
 

htlcswitch/mailbox.go

@@ -548,6 +548,8 @@ func (m *memoryMailBox) mailCourier(cType courierType) {
 				m.pktCond.L.Unlock()
 
 			case <-deadline:
+				log.Debugf("Expiring add htlc with "+
+					"keystone=%v", add.keystone())
 				m.FailAdd(add)
 
 			case pktDone := <-m.pktReset:

htlcswitch/mailbox_test.go

@@ -10,8 +10,10 @@ import (
 	"github.com/davecgh/go-spew/spew"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/clock"
+	"github.com/lightningnetwork/lnd/lnmock"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -192,6 +194,35 @@ type mailboxContext struct {
 	forwards chan *htlcPacket
 }
 
+// newMailboxContextWithClock creates a new mailbox context with the given
+// mocked clock.
+//
+// TODO(yy): replace all usage of `newMailboxContext` with this method.
+func newMailboxContextWithClock(t *testing.T,
+	clock clock.Clock) *mailboxContext {
+
+	ctx := &mailboxContext{
+		t:        t,
+		forwards: make(chan *htlcPacket, 1),
+	}
+
+	failMailboxUpdate := func(outScid,
+		mboxScid lnwire.ShortChannelID) lnwire.FailureMessage {
+
+		return &lnwire.FailTemporaryNodeFailure{}
+	}
+
+	ctx.mailbox = newMemoryMailBox(&mailBoxConfig{
+		failMailboxUpdate: failMailboxUpdate,
+		forwardPackets:    ctx.forward,
+		clock:             clock,
+	})
+	ctx.mailbox.Start()
+	t.Cleanup(ctx.mailbox.Stop)
+
+	return ctx
+}
+
 func newMailboxContext(t *testing.T, startTime time.Time,
 	expiry time.Duration) *mailboxContext {
 
@@ -294,7 +325,7 @@ func (c *mailboxContext) checkFails(adds []*htlcPacket) {
 
 	select {
 	case pkt := <-c.forwards:
-		c.t.Fatalf("unexpected forward: %v", pkt)
+		c.t.Fatalf("unexpected forward: %v", pkt.keystone())
 	case <-time.After(50 * time.Millisecond):
 	}
 }
@@ -461,34 +492,44 @@ func TestMailBoxPacketPrioritization(t *testing.T) {
 	}
 }
 
-// TestMailBoxAddExpiry asserts that the mailbox will cancel back Adds that have
+// TestMailBoxAddExpiry asserts that the mailbox will cancel back Adds that
-// reached their expiry time.
+// have reached their expiry time.
 func TestMailBoxAddExpiry(t *testing.T) {
-	var (
-		expiry            = time.Minute
-		batchDelay        = time.Second
-		firstBatchStart   = time.Now()
-		firstBatchExpiry  = firstBatchStart.Add(expiry)
-		secondBatchStart  = firstBatchStart.Add(batchDelay)
-		secondBatchExpiry = secondBatchStart.Add(expiry)
-	)
-
-	ctx := newMailboxContext(t, firstBatchStart, expiry)
-
 	// Each batch will consist of 10 messages.
 	const numBatchPackets = 10
 
-	firstBatch := ctx.sendAdds(0, numBatchPackets)
+	// deadline is the returned value from the `pktWithExpiry.deadline`.
+	deadline := make(chan time.Time, numBatchPackets*2)
 
-	ctx.clock.SetTime(secondBatchStart)
+	// Create a mock clock and mock the methods.
+	mockClock := &lnmock.MockClock{}
+	mockClock.On("Now").Return(time.Now())
+
+	// Mock TickAfter, which mounts the above `deadline` channel to the
+	// returned value from `pktWithExpiry.deadline`.
+	mockClock.On("TickAfter", mock.Anything).Return(deadline)
+
+	// Create a test mailbox context.
+	ctx := newMailboxContextWithClock(t, mockClock)
+
+	// Send 10 packets and assert no failures are sent back.
+	firstBatch := ctx.sendAdds(0, numBatchPackets)
 	ctx.checkFails(nil)
 
+	// Send another 10 packets and assert no failures are sent back.
 	secondBatch := ctx.sendAdds(numBatchPackets, numBatchPackets)
+	ctx.checkFails(nil)
 
-	ctx.clock.SetTime(firstBatchExpiry)
+	// Tick 10 times and we should see the first batch expired.
+	for i := 0; i < numBatchPackets; i++ {
+		deadline <- time.Now()
+	}
 	ctx.checkFails(firstBatch)
 
-	ctx.clock.SetTime(secondBatchExpiry)
+	// Tick another 10 times and we should see the second batch expired.
+	for i := 0; i < numBatchPackets; i++ {
+		deadline <- time.Now()
+	}
 	ctx.checkFails(secondBatch)
 }
 

lnmock/clock.go

@@ -0,0 +1,30 @@
+// NOTE: forcetypeassert is skipped for the mock because the test would fail if
+// the returned value doesn't match the type.
+package lnmock
+
+import (
+	"time"
+
+	"github.com/lightningnetwork/lnd/clock"
+	"github.com/stretchr/testify/mock"
+)
+
+// MockClock implements the `clock.Clock` interface.
+type MockClock struct {
+	mock.Mock
+}
+
+// Compile time assertion that MockClock implements clock.Clock.
+var _ clock.Clock = (*MockClock)(nil)
+
+func (m *MockClock) Now() time.Time {
+	args := m.Called()
+
+	return args.Get(0).(time.Time)
+}
+
+func (m *MockClock) TickAfter(d time.Duration) <-chan time.Time {
+	args := m.Called(d)
+
+	return args.Get(0).(chan time.Time)
+}