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 <bradfitz@tailscale.com>
Co-authored-by: Avery Pennarun <apenwarr@tailscale.com>

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

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
 }
 

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 {

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)

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 {

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",

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{}

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) {}

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.