From 265c76dbc5469e852277ef6f1bc691c7895e6e58 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 8 Apr 2025 07:49:28 -0700 Subject: [PATCH] all: unify some redundant testing.TB interface copies I added yet another one in 6d117d64a256234 but that new one is at the best place int he dependency graph and has the best name, so let's use that one for everything possible. types/lazy can't use it for circular dependency reasons, so unexport that copy at least. Updates #cleanup Change-Id: I25db6b6a0d81dbb8e89a0a9080c7f15cbf7aa770 Signed-off-by: Brad Fitzpatrick --- cmd/stund/depaware.txt | 3 ++- types/lazy/lazy.go | 8 +++++--- types/logger/logger.go | 9 ++------- util/syspolicy/handler.go | 8 ++------ util/syspolicy/internal/internal.go | 15 ++------------- util/syspolicy/internal/loggerx/logger.go | 4 ++-- util/syspolicy/internal/metrics/metrics.go | 2 +- util/syspolicy/internal/metrics/test_handler.go | 5 +++-- util/syspolicy/policy_keys.go | 2 +- util/syspolicy/rsop/resultant_policy.go | 4 ++-- util/syspolicy/rsop/store_registration.go | 4 ++-- util/syspolicy/setting/setting.go | 3 ++- util/syspolicy/source/test_store.go | 8 ++++---- util/syspolicy/syspolicy.go | 3 ++- util/syspolicy/syspolicy_test.go | 3 ++- util/syspolicy/syspolicy_windows.go | 2 +- 16 files changed, 35 insertions(+), 48 deletions(-) diff --git a/cmd/stund/depaware.txt b/cmd/stund/depaware.txt index 2326e3a24..6168e1582 100644 --- a/cmd/stund/depaware.txt +++ b/cmd/stund/depaware.txt @@ -80,6 +80,7 @@ tailscale.com/cmd/stund dependencies: (generated by github.com/tailscale/depawar tailscale.com/util/nocasemaps from tailscale.com/types/ipproto tailscale.com/util/rands from tailscale.com/tsweb tailscale.com/util/slicesx from tailscale.com/tailcfg + tailscale.com/util/testenv from tailscale.com/types/logger tailscale.com/util/vizerror from tailscale.com/tailcfg+ tailscale.com/version from tailscale.com/envknob+ tailscale.com/version/distro from tailscale.com/envknob @@ -186,7 +187,7 @@ tailscale.com/cmd/stund dependencies: (generated by github.com/tailscale/depawar encoding/pem from crypto/tls+ errors from bufio+ expvar from github.com/prometheus/client_golang/prometheus+ - flag from tailscale.com/cmd/stund + flag from tailscale.com/cmd/stund+ fmt from compress/flate+ go/token from google.golang.org/protobuf/internal/strs hash from crypto+ diff --git a/types/lazy/lazy.go b/types/lazy/lazy.go index c29a03db4..f5d7be494 100644 --- a/types/lazy/lazy.go +++ b/types/lazy/lazy.go @@ -120,9 +120,9 @@ func (z *SyncValue[T]) PeekErr() (v T, err error, ok bool) { return zero, nil, false } -// TB is a subset of testing.TB that we use to set up test helpers. +// testing_TB is a subset of testing.TB that we use to set up test helpers. // It's defined here to avoid pulling in the testing package. -type TB interface { +type testing_TB interface { Helper() Cleanup(func()) } @@ -132,7 +132,9 @@ type TB interface { // subtests complete. // It is not safe for concurrent use and must not be called concurrently with // any SyncValue methods, including another call to itself. -func (z *SyncValue[T]) SetForTest(tb TB, val T, err error) { +// +// The provided tb should be a [*testing.T] or [*testing.B]. +func (z *SyncValue[T]) SetForTest(tb testing_TB, val T, err error) { tb.Helper() oldErr, oldVal := z.err.Load(), z.v diff --git a/types/logger/logger.go b/types/logger/logger.go index 66b989480..aeced352e 100644 --- a/types/logger/logger.go +++ b/types/logger/logger.go @@ -24,6 +24,7 @@ import ( "go4.org/mem" "tailscale.com/envknob" "tailscale.com/util/ctxkey" + "tailscale.com/util/testenv" ) // Logf is the basic Tailscale logger type: a printf-like func. @@ -384,16 +385,10 @@ func (a asJSONResult) Format(s fmt.State, verb rune) { s.Write(v) } -// TBLogger is the testing.TB subset needed by TestLogger. -type TBLogger interface { - Helper() - Logf(format string, args ...any) -} - // TestLogger returns a logger that logs to tb.Logf // with a prefix to make it easier to distinguish spam // from explicit test failures. -func TestLogger(tb TBLogger) Logf { +func TestLogger(tb testenv.TB) Logf { return func(format string, args ...any) { tb.Helper() tb.Logf(" ... "+format, args...) diff --git a/util/syspolicy/handler.go b/util/syspolicy/handler.go index f511f0a56..c4bfd9de9 100644 --- a/util/syspolicy/handler.go +++ b/util/syspolicy/handler.go @@ -4,10 +4,10 @@ package syspolicy import ( - "tailscale.com/util/syspolicy/internal" "tailscale.com/util/syspolicy/rsop" "tailscale.com/util/syspolicy/setting" "tailscale.com/util/syspolicy/source" + "tailscale.com/util/testenv" ) // TODO(nickkhyl): delete this file once other repos are updated. @@ -38,15 +38,11 @@ func RegisterHandler(h Handler) { rsop.RegisterStore("DeviceHandler", setting.DeviceScope, WrapHandler(h)) } -// TB is a subset of testing.TB that we use to set up test helpers. -// It's defined here to avoid pulling in the testing package. -type TB = internal.TB - // SetHandlerForTest wraps and sets the specified handler as the device's policy // [source.Store] for the duration of tb. // // Deprecated: using [MustRegisterStoreForTest] should be preferred. -func SetHandlerForTest(tb TB, h Handler) { +func SetHandlerForTest(tb testenv.TB, h Handler) { RegisterWellKnownSettingsForTest(tb) MustRegisterStoreForTest(tb, "DeviceHandler-TestOnly", setting.DefaultScope(), WrapHandler(h)) } diff --git a/util/syspolicy/internal/internal.go b/util/syspolicy/internal/internal.go index 2e1737e5b..6ab147de6 100644 --- a/util/syspolicy/internal/internal.go +++ b/util/syspolicy/internal/internal.go @@ -10,6 +10,7 @@ import ( "github.com/go-json-experiment/json/jsontext" "tailscale.com/types/lazy" + "tailscale.com/util/testenv" "tailscale.com/version" ) @@ -25,22 +26,10 @@ func OS() string { return OSForTesting.Get(version.OS) } -// TB is a subset of testing.TB that we use to set up test helpers. -// It's defined here to avoid pulling in the testing package. -type TB interface { - Helper() - Cleanup(func()) - Logf(format string, args ...any) - Error(args ...any) - Errorf(format string, args ...any) - Fatal(args ...any) - Fatalf(format string, args ...any) -} - // EqualJSONForTest compares the JSON in j1 and j2 for semantic equality. // It returns "", "", true if j1 and j2 are equal. Otherwise, it returns // indented versions of j1 and j2 and false. -func EqualJSONForTest(tb TB, j1, j2 jsontext.Value) (s1, s2 string, equal bool) { +func EqualJSONForTest(tb testenv.TB, j1, j2 jsontext.Value) (s1, s2 string, equal bool) { tb.Helper() j1 = j1.Clone() j2 = j2.Clone() diff --git a/util/syspolicy/internal/loggerx/logger.go b/util/syspolicy/internal/loggerx/logger.go index c29a5f084..d1f48cbb4 100644 --- a/util/syspolicy/internal/loggerx/logger.go +++ b/util/syspolicy/internal/loggerx/logger.go @@ -10,7 +10,7 @@ import ( "tailscale.com/types/lazy" "tailscale.com/types/logger" - "tailscale.com/util/syspolicy/internal" + "tailscale.com/util/testenv" ) const ( @@ -58,7 +58,7 @@ func verbosef(format string, args ...any) { // SetForTest sets the specified printf and verbosef functions for the duration // of tb and its subtests. -func SetForTest(tb internal.TB, printf, verbosef logger.Logf) { +func SetForTest(tb testenv.TB, printf, verbosef logger.Logf) { lazyPrintf.SetForTest(tb, printf, nil) lazyVerbosef.SetForTest(tb, verbosef, nil) } diff --git a/util/syspolicy/internal/metrics/metrics.go b/util/syspolicy/internal/metrics/metrics.go index 770a34d29..43f2a285a 100644 --- a/util/syspolicy/internal/metrics/metrics.go +++ b/util/syspolicy/internal/metrics/metrics.go @@ -259,7 +259,7 @@ var addMetricTestHook, setMetricTestHook syncs.AtomicValue[metricFn] // SetHooksForTest sets the specified addMetric and setMetric functions // as the metric functions for the duration of tb and all its subtests. -func SetHooksForTest(tb internal.TB, addMetric, setMetric metricFn) { +func SetHooksForTest(tb testenv.TB, addMetric, setMetric metricFn) { oldAddMetric := addMetricTestHook.Swap(addMetric) oldSetMetric := setMetricTestHook.Swap(setMetric) tb.Cleanup(func() { diff --git a/util/syspolicy/internal/metrics/test_handler.go b/util/syspolicy/internal/metrics/test_handler.go index f9e484609..36c3f2cad 100644 --- a/util/syspolicy/internal/metrics/test_handler.go +++ b/util/syspolicy/internal/metrics/test_handler.go @@ -9,6 +9,7 @@ import ( "tailscale.com/util/clientmetric" "tailscale.com/util/set" "tailscale.com/util/syspolicy/internal" + "tailscale.com/util/testenv" ) // TestState represents a metric name and its expected value. @@ -19,13 +20,13 @@ type TestState struct { // TestHandler facilitates testing of the code that uses metrics. type TestHandler struct { - t internal.TB + t testenv.TB m map[string]int64 } // NewTestHandler returns a new TestHandler. -func NewTestHandler(t internal.TB) *TestHandler { +func NewTestHandler(t testenv.TB) *TestHandler { return &TestHandler{t, make(map[string]int64)} } diff --git a/util/syspolicy/policy_keys.go b/util/syspolicy/policy_keys.go index a81c1e5d5..8da0e0cc8 100644 --- a/util/syspolicy/policy_keys.go +++ b/util/syspolicy/policy_keys.go @@ -239,7 +239,7 @@ func WellKnownSettingDefinition(k Key) (*setting.Definition, error) { // RegisterWellKnownSettingsForTest registers all implicit setting definitions // for the duration of the test. -func RegisterWellKnownSettingsForTest(tb TB) { +func RegisterWellKnownSettingsForTest(tb testenv.TB) { tb.Helper() err := setting.SetDefinitionsForTest(tb, implicitDefinitions...) if err != nil { diff --git a/util/syspolicy/rsop/resultant_policy.go b/util/syspolicy/rsop/resultant_policy.go index b811a00ee..297d26f9f 100644 --- a/util/syspolicy/rsop/resultant_policy.go +++ b/util/syspolicy/rsop/resultant_policy.go @@ -11,9 +11,9 @@ import ( "sync/atomic" "time" - "tailscale.com/util/syspolicy/internal" "tailscale.com/util/syspolicy/internal/loggerx" "tailscale.com/util/syspolicy/setting" + "tailscale.com/util/testenv" "tailscale.com/util/syspolicy/source" ) @@ -449,7 +449,7 @@ func (p *Policy) Close() { } } -func setForTest[T any](tb internal.TB, target *T, newValue T) { +func setForTest[T any](tb testenv.TB, target *T, newValue T) { oldValue := *target tb.Cleanup(func() { *target = oldValue }) *target = newValue diff --git a/util/syspolicy/rsop/store_registration.go b/util/syspolicy/rsop/store_registration.go index f9836846e..a7c354b6d 100644 --- a/util/syspolicy/rsop/store_registration.go +++ b/util/syspolicy/rsop/store_registration.go @@ -9,9 +9,9 @@ import ( "sync/atomic" "time" - "tailscale.com/util/syspolicy/internal" "tailscale.com/util/syspolicy/setting" "tailscale.com/util/syspolicy/source" + "tailscale.com/util/testenv" ) // ErrAlreadyConsumed is the error returned when [StoreRegistration.ReplaceStore] @@ -33,7 +33,7 @@ func RegisterStore(name string, scope setting.PolicyScope, store source.Store) ( // RegisterStoreForTest is like [RegisterStore], but unregisters the store when // tb and all its subtests complete. -func RegisterStoreForTest(tb internal.TB, name string, scope setting.PolicyScope, store source.Store) (*StoreRegistration, error) { +func RegisterStoreForTest(tb testenv.TB, name string, scope setting.PolicyScope, store source.Store) (*StoreRegistration, error) { setForTest(tb, &policyReloadMinDelay, 10*time.Millisecond) setForTest(tb, &policyReloadMaxDelay, 500*time.Millisecond) diff --git a/util/syspolicy/setting/setting.go b/util/syspolicy/setting/setting.go index 70fb0a931..13c7a2a5f 100644 --- a/util/syspolicy/setting/setting.go +++ b/util/syspolicy/setting/setting.go @@ -16,6 +16,7 @@ import ( "tailscale.com/types/lazy" "tailscale.com/util/syspolicy/internal" + "tailscale.com/util/testenv" ) // Scope indicates the broadest scope at which a policy setting may apply, @@ -277,7 +278,7 @@ func DefinitionMapOf(settings []*Definition) (DefinitionMap, error) { // for the test duration. It is not concurrency-safe, but unlike [Register], // it does not panic and can be called anytime. // It returns an error if ds contains two different settings with the same [Key]. -func SetDefinitionsForTest(tb lazy.TB, ds ...*Definition) error { +func SetDefinitionsForTest(tb testenv.TB, ds ...*Definition) error { m, err := DefinitionMapOf(ds) if err != nil { return err diff --git a/util/syspolicy/source/test_store.go b/util/syspolicy/source/test_store.go index e6c09d6b0..4b175611f 100644 --- a/util/syspolicy/source/test_store.go +++ b/util/syspolicy/source/test_store.go @@ -12,8 +12,8 @@ import ( "tailscale.com/util/mak" "tailscale.com/util/set" "tailscale.com/util/slicesx" - "tailscale.com/util/syspolicy/internal" "tailscale.com/util/syspolicy/setting" + "tailscale.com/util/testenv" ) var ( @@ -79,7 +79,7 @@ func (r TestExpectedReads) operation() testReadOperation { // TestStore is a [Store] that can be used in tests. type TestStore struct { - tb internal.TB + tb testenv.TB done chan struct{} @@ -98,7 +98,7 @@ type TestStore struct { // NewTestStore returns a new [TestStore]. // The tb will be used to report coding errors detected by the [TestStore]. -func NewTestStore(tb internal.TB) *TestStore { +func NewTestStore(tb testenv.TB) *TestStore { m := make(map[setting.Key]any) store := &TestStore{ tb: tb, @@ -112,7 +112,7 @@ func NewTestStore(tb internal.TB) *TestStore { // NewTestStoreOf is a shorthand for [NewTestStore] followed by [TestStore.SetBooleans], // [TestStore.SetUInt64s], [TestStore.SetStrings] or [TestStore.SetStringLists]. -func NewTestStoreOf[T TestValueType](tb internal.TB, settings ...TestSetting[T]) *TestStore { +func NewTestStoreOf[T TestValueType](tb testenv.TB, settings ...TestSetting[T]) *TestStore { store := NewTestStore(tb) switch settings := any(settings).(type) { case []TestSetting[bool]: diff --git a/util/syspolicy/syspolicy.go b/util/syspolicy/syspolicy.go index d925731c3..5d5a283fb 100644 --- a/util/syspolicy/syspolicy.go +++ b/util/syspolicy/syspolicy.go @@ -20,6 +20,7 @@ import ( "tailscale.com/util/syspolicy/rsop" "tailscale.com/util/syspolicy/setting" "tailscale.com/util/syspolicy/source" + "tailscale.com/util/testenv" ) var ( @@ -46,7 +47,7 @@ func RegisterStore(name string, scope setting.PolicyScope, store source.Store) ( } // MustRegisterStoreForTest is like [rsop.RegisterStoreForTest], but it fails the test if the store could not be registered. -func MustRegisterStoreForTest(tb TB, name string, scope setting.PolicyScope, store source.Store) *rsop.StoreRegistration { +func MustRegisterStoreForTest(tb testenv.TB, name string, scope setting.PolicyScope, store source.Store) *rsop.StoreRegistration { tb.Helper() reg, err := rsop.RegisterStoreForTest(tb, name, scope, store) if err != nil { diff --git a/util/syspolicy/syspolicy_test.go b/util/syspolicy/syspolicy_test.go index a70a49d39..fc01f3645 100644 --- a/util/syspolicy/syspolicy_test.go +++ b/util/syspolicy/syspolicy_test.go @@ -14,6 +14,7 @@ import ( "tailscale.com/util/syspolicy/internal/metrics" "tailscale.com/util/syspolicy/setting" "tailscale.com/util/syspolicy/source" + "tailscale.com/util/testenv" ) var someOtherError = errors.New("error other than not found") @@ -596,7 +597,7 @@ func TestGetStringArray(t *testing.T) { } } -func registerSingleSettingStoreForTest[T source.TestValueType](tb TB, s source.TestSetting[T]) { +func registerSingleSettingStoreForTest[T source.TestValueType](tb testenv.TB, s source.TestSetting[T]) { policyStore := source.NewTestStoreOf(tb, s) MustRegisterStoreForTest(tb, "TestStore", setting.DeviceScope, policyStore) } diff --git a/util/syspolicy/syspolicy_windows.go b/util/syspolicy/syspolicy_windows.go index 9d57e249e..ca0fd329a 100644 --- a/util/syspolicy/syspolicy_windows.go +++ b/util/syspolicy/syspolicy_windows.go @@ -43,7 +43,7 @@ func init() { // configureSyspolicy configures syspolicy for use on Windows, // either in test or regular builds depending on whether tb has a non-nil value. -func configureSyspolicy(tb internal.TB) error { +func configureSyspolicy(tb testenv.TB) error { const localSystemSID = "S-1-5-18" // Always create and register a machine policy store that reads // policy settings from the HKEY_LOCAL_MACHINE registry hive.