diff --git a/.golangci.yml b/.golangci.yml index 2ab612f58..7b97fee00 100644 --- a/.golangci.yml +++ b/.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 diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index 47f369ce6..3e5b81825 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/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 diff --git a/htlcswitch/circuit_map.go b/htlcswitch/circuit_map.go index a62067727..9b26a1d07 100644 --- a/htlcswitch/circuit_map.go +++ b/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) } diff --git a/htlcswitch/mailbox.go b/htlcswitch/mailbox.go index de85f7818..cc3bd1c83 100644 --- a/htlcswitch/mailbox.go +++ b/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: diff --git a/htlcswitch/mailbox_test.go b/htlcswitch/mailbox_test.go index b4fc8a6ec..e48aacfcd 100644 --- a/htlcswitch/mailbox_test.go +++ b/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 -// reached their expiry time. +// TestMailBoxAddExpiry asserts that the mailbox will cancel back Adds that +// 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) } diff --git a/lnmock/clock.go b/lnmock/clock.go new file mode 100644 index 000000000..bd00c42c8 --- /dev/null +++ b/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) +}