From 9fbe4b3ed28cd68be6d25b6824bd638ab14b292e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 14 Apr 2026 01:27:26 +0000 Subject: [PATCH] all: fix six tests that failed with -count=2 Avery found a bunch of tests that fail with -count=2. Updates tailscale/corp#40176 (tracks making our CI detect them) Change-Id: Ie3e4398070dd92e4fe0146badddf1254749cca20 Signed-off-by: Brad Fitzpatrick Co-authored-by: Avery Pennarun --- appc/appconnector_test.go | 1 + cmd/tailscale/cli/cli.go | 51 +++++++++++++++++++++++------- cmd/tailscale/cli/cli_test.go | 2 +- ipn/ipnlocal/cert_test.go | 3 ++ net/dns/direct_linux_test.go | 2 +- tsweb/varz/varz_test.go | 2 +- util/clientmetric/clientmetric.go | 19 +++++++++++ util/clientmetric/omit.go | 2 ++ wgengine/netstack/netstack_test.go | 3 ++ 9 files changed, 70 insertions(+), 15 deletions(-) diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index d14ef68fc..c58aa8041 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -736,6 +736,7 @@ func TestRateLogger(t *testing.T) { } func TestRouteStoreMetrics(t *testing.T) { + clientmetric.ResetForTest(t) metricStoreRoutes(1, 1) metricStoreRoutes(1, 1) // the 1 buckets value should be 2 metricStoreRoutes(5, 5) // the 5 buckets value should be 1 diff --git a/cmd/tailscale/cli/cli.go b/cmd/tailscale/cli/cli.go index 8a2c2b9ef..a9d7364c2 100644 --- a/cmd/tailscale/cli/cli.go +++ b/cmd/tailscale/cli/cli.go @@ -28,6 +28,7 @@ import ( "tailscale.com/feature" "tailscale.com/paths" "tailscale.com/util/slicesx" + "tailscale.com/util/testenv" "tailscale.com/version/distro" ) @@ -194,17 +195,39 @@ func (v *onceFlagValue) IsBoolFlag() bool { return ok && bf.IsBoolFlag() } -// noDupFlagify modifies c recursively to make all the -// flag values be wrappers that permit setting the value -// at most once. -func noDupFlagify(c *ffcli.Command) { - if c.FlagSet != nil { - c.FlagSet.VisitAll(func(f *flag.Flag) { - f.Value = &onceFlagValue{Value: f.Value} - }) +// noDupFlagify modifies c recursively to make all the flag values be +// wrappers that permit setting the value at most once. If tb is +// non-nil, the original values are restored when the test completes. +func noDupFlagify(c *ffcli.Command, tb testenv.TB) { + if tb == nil && testenv.InTest() { + return } - for _, sub := range c.Subcommands { - noDupFlagify(sub) + type restore struct { + f *flag.Flag + v flag.Value + } + var restores []restore + var walk func(*ffcli.Command) + walk = func(c *ffcli.Command) { + if c.FlagSet != nil { + c.FlagSet.VisitAll(func(f *flag.Flag) { + if tb != nil { + restores = append(restores, restore{f, f.Value}) + } + f.Value = &onceFlagValue{Value: f.Value} + }) + } + for _, sub := range c.Subcommands { + walk(sub) + } + } + walk(c) + if tb != nil { + tb.Cleanup(func() { + for _, r := range restores { + r.f.Value = r.v + } + }) } } @@ -221,7 +244,7 @@ var ( _ func() *ffcli.Command ) -func newRootCmd() *ffcli.Command { +func newRootCmd(tb ...testenv.TB) *ffcli.Command { rootfs := newFlagSet("tailscale") rootfs.Func("socket", "path to tailscaled socket", func(s string) error { localClient.Socket = s @@ -303,7 +326,11 @@ change in the future. }) ffcomplete.Inject(rootCmd, func(c *ffcli.Command) { c.LongHelp = hidden + c.LongHelp }, usageFunc) - noDupFlagify(rootCmd) + var t testenv.TB + if len(tb) > 0 { + t = tb[0] + } + noDupFlagify(rootCmd, t) return rootCmd } diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index f95d84695..2974d037f 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -1618,7 +1618,7 @@ func TestNoDups(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd := newRootCmd() + cmd := newRootCmd(t) makeQuietContinueOnError(cmd) err := cmd.Parse(tt.args) if got := fmt.Sprint(err); got != tt.want { diff --git a/ipn/ipnlocal/cert_test.go b/ipn/ipnlocal/cert_test.go index e2d69da52..56d6df77f 100644 --- a/ipn/ipnlocal/cert_test.go +++ b/ipn/ipnlocal/cert_test.go @@ -519,8 +519,11 @@ func TestGetCertPEMWithValidity(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + tstest.AssertNotParallel(t) if tt.readOnlyMode { envknob.Setenv("TS_CERT_SHARE_MODE", "ro") + } else { + envknob.Setenv("TS_CERT_SHARE_MODE", "") } os.RemoveAll(certDir) diff --git a/net/dns/direct_linux_test.go b/net/dns/direct_linux_test.go index 8199b41f3..9955a863f 100644 --- a/net/dns/direct_linux_test.go +++ b/net/dns/direct_linux_test.go @@ -21,7 +21,7 @@ import ( ) func TestDNSTrampleRecovery(t *testing.T) { - HookWatchFile.Set(watchFile) + t.Cleanup(HookWatchFile.SetForTest(watchFile)) synctest.Test(t, func(t *testing.T) { tmp := t.TempDir() if err := os.MkdirAll(filepath.Join(tmp, "etc"), 0700); err != nil { diff --git a/tsweb/varz/varz_test.go b/tsweb/varz/varz_test.go index d041edb4b..27094e77b 100644 --- a/tsweb/varz/varz_test.go +++ b/tsweb/varz/varz_test.go @@ -205,7 +205,7 @@ func TestVarzHandler(t *testing.T) { "string_map", func() *expvar.Map { m := new(expvar.Map) - m.Set("a", expvar.NewString("foo")) + m.Set("a", new(expvar.String)) return m }(), "# skipping \"string_map\" expvar map key \"a\" with unknown value type *expvar.String\n", diff --git a/util/clientmetric/clientmetric.go b/util/clientmetric/clientmetric.go index b67cbbd39..98068b9fa 100644 --- a/util/clientmetric/clientmetric.go +++ b/util/clientmetric/clientmetric.go @@ -22,6 +22,7 @@ import ( "tailscale.com/feature/buildfeatures" "tailscale.com/util/set" + "tailscale.com/util/testenv" ) var ( @@ -452,6 +453,24 @@ func (b *deltaEncBuf) writeHexVarint(v int64) { b.buf.Write(hexBuf) } +// ResetForTest resets all client metric values to zero. +// It panics if not in a test or if called from a parallel test. +func ResetForTest(t testenv.TB) { + if !testenv.InTest() { + panic("clientmetric.ResetForTest called outside a test") + } + if testenv.InParallelTest(t) { + panic("clientmetric.ResetForTest called from a parallel test") + } + mu.Lock() + defer mu.Unlock() + for _, m := range metrics { + if m.v != nil { + atomic.StoreInt64(m.v, 0) + } + } +} + var TestHooks testHooks type testHooks struct{} diff --git a/util/clientmetric/omit.go b/util/clientmetric/omit.go index 380205eeb..74018f12a 100644 --- a/util/clientmetric/omit.go +++ b/util/clientmetric/omit.go @@ -30,3 +30,5 @@ func NewCounter(string) *Metric { return &zeroMetric } func NewGauge(string) *Metric { return &zeroMetric } func NewAggregateCounter(string) *Metric { return &zeroMetric } func NewCounterFunc(string, func() int64) *Metric { return &zeroMetric } + +func ResetForTest(any) {} diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index fd44741bc..32289d13b 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -33,6 +33,7 @@ import ( "tailscale.com/types/ipproto" "tailscale.com/types/logid" "tailscale.com/types/netmap" + "tailscale.com/util/clientmetric" "tailscale.com/wgengine" "tailscale.com/wgengine/filter" ) @@ -811,6 +812,8 @@ func TestTCPForwardLimits(t *testing.T) { // TestTCPForwardLimits_PerClient verifies that the per-client limit for TCP // forwarding works. func TestTCPForwardLimits_PerClient(t *testing.T) { + clientmetric.ResetForTest(t) + tstest.AssertNotParallel(t) // calls envknob.Setenv envknob.Setenv("TS_DEBUG_NETSTACK", "true") // Set our test override limits during this test.