From 640134421eb922e8b85c3686f9966d7001a02879 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 7 Sep 2021 19:27:19 -0700 Subject: [PATCH] all: update tests to use tstest.MemLogger And give MemLogger a mutex, as one caller had, which does match the logf contract better. Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 8 +++----- ipn/ipnlocal/peerapi_test.go | 9 +++------ net/dns/manager_linux_test.go | 11 +++-------- net/portmapper/upnp_test.go | 10 +++------- tstest/log.go | 3 +++ wgengine/magicsock/magicsock_test.go | 24 ++---------------------- 6 files changed, 17 insertions(+), 48 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 4ab7c5e8e..24f3fb100 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -17,6 +17,7 @@ import ( "inet.af/netaddr" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" + "tailscale.com/tstest" "tailscale.com/types/persist" "tailscale.com/types/preftype" ) @@ -607,10 +608,7 @@ func TestPrefsFromUpArgs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var warnBuf bytes.Buffer - warnf := func(format string, a ...interface{}) { - fmt.Fprintf(&warnBuf, format, a...) - } + var warnBuf tstest.MemLogger goos := tt.goos if goos == "" { goos = "linux" @@ -619,7 +617,7 @@ func TestPrefsFromUpArgs(t *testing.T) { if st == nil { st = new(ipnstate.Status) } - got, err := prefsFromUpArgs(tt.args, warnf, st, goos) + got, err := prefsFromUpArgs(tt.args, warnBuf.Logf, st, goos) gotErr := fmt.Sprint(err) if tt.wantErr != "" { if tt.wantErr != gotErr { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index f804219bb..1b9c5fb72 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -20,16 +20,13 @@ import ( "testing" "tailscale.com/tailcfg" + "tailscale.com/tstest" ) type peerAPITestEnv struct { ph *peerAPIHandler rr *httptest.ResponseRecorder - logBuf bytes.Buffer -} - -func (e *peerAPITestEnv) logf(format string, a ...interface{}) { - fmt.Fprintf(&e.logBuf, format, a...) + logBuf tstest.MemLogger } type check func(*testing.T, *peerAPITestEnv) @@ -403,7 +400,7 @@ func TestHandlePeerAPI(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var e peerAPITestEnv lb := &LocalBackend{ - logf: e.logf, + logf: e.logBuf.Logf, capFileSharing: tt.capSharing, } e.ph = &peerAPIHandler{ diff --git a/net/dns/manager_linux_test.go b/net/dns/manager_linux_test.go index c1891822b..d634de9a3 100644 --- a/net/dns/manager_linux_test.go +++ b/net/dns/manager_linux_test.go @@ -5,14 +5,13 @@ package dns import ( - "bytes" "errors" - "fmt" "io/fs" "os" "strings" "testing" + "tailscale.com/tstest" "tailscale.com/util/cmpver" ) @@ -146,12 +145,8 @@ func TestLinuxDNSMode(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var logBuf bytes.Buffer - logf := func(format string, a ...interface{}) { - fmt.Fprintf(&logBuf, format, a...) - logBuf.WriteByte('\n') - } - got, err := dnsMode(logf, tt.env) + var logBuf tstest.MemLogger + got, err := dnsMode(logBuf.Logf, tt.env) if err != nil { t.Fatal(err) } diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index 347d00b6a..8071179de 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -5,7 +5,6 @@ package portmapper import ( - "bytes" "context" "fmt" "io" @@ -17,6 +16,7 @@ import ( "testing" "inet.af/netaddr" + "tailscale.com/tstest" ) // Google Wifi @@ -97,12 +97,8 @@ func TestGetUPnPClient(t *testing.T) { })) defer ts.Close() gw, _ := netaddr.FromStdIP(ts.Listener.Addr().(*net.TCPAddr).IP) - var logBuf bytes.Buffer - logf := func(format string, a ...interface{}) { - fmt.Fprintf(&logBuf, format, a...) - logBuf.WriteByte('\n') - } - c, err := getUPnPClient(context.Background(), logf, gw, uPnPDiscoResponse{ + var logBuf tstest.MemLogger + c, err := getUPnPClient(context.Background(), logBuf.Logf, gw, uPnPDiscoResponse{ Location: ts.URL + "/rootDesc.xml", }) if err != nil { diff --git a/tstest/log.go b/tstest/log.go index 867a5a1bc..c28199c63 100644 --- a/tstest/log.go +++ b/tstest/log.go @@ -127,10 +127,13 @@ func (lt *LogLineTracker) Close() { // MemLogger is a bytes.Buffer with a Logf method for tests that want // to log to a buffer. type MemLogger struct { + sync.Mutex bytes.Buffer } func (ml *MemLogger) Logf(format string, args ...interface{}) { + ml.Lock() + defer ml.Unlock() fmt.Fprintf(&ml.Buffer, format, args...) if !mem.HasSuffix(mem.B(ml.Buffer.Bytes()), mem.S("\n")) { ml.Buffer.WriteByte('\n') diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 7fb988654..6c9495f9b 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -1315,26 +1315,6 @@ func BenchmarkReceiveFrom_Native(b *testing.B) { } } -type bufLog struct { - sync.Mutex - buf bytes.Buffer -} - -func (b *bufLog) Logf(format string, args ...interface{}) { - b.Lock() - defer b.Unlock() - fmt.Fprintf(&b.buf, format, args...) - if !bytes.HasPrefix(b.buf.Bytes(), []byte("\n")) { - b.buf.WriteByte('\n') - } -} - -func (b *bufLog) String() string { - b.Lock() - defer b.Unlock() - return b.buf.String() -} - // Test that a netmap update where node changes its node key but // doesn't change its disco key doesn't result in a broken state. // @@ -1342,7 +1322,7 @@ func (b *bufLog) String() string { func TestSetNetworkMapChangingNodeKey(t *testing.T) { conn := newTestConn(t) t.Cleanup(func() { conn.Close() }) - var buf bufLog + var buf tstest.MemLogger conn.logf = buf.Logf conn.SetPrivateKey(wgkey.Private{0: 1}) @@ -1401,7 +1381,7 @@ func TestSetNetworkMapChangingNodeKey(t *testing.T) { func TestRebindStress(t *testing.T) { conn := newTestConn(t) - var buf bufLog + var buf tstest.MemLogger conn.logf = buf.Logf closed := false