From 3eff6ccff272a493b44756198389bbc6205d07d1 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 6 May 2026 15:19:51 +1000 Subject: [PATCH] test(vm): remove tautologies and asserts-nothing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit manager_deps_test.go: drop TestDeps_VolumeMounterSatisfiesInterface, TestDeps_StateStoreSatisfiesInterface, TestDeps_StateStoreErrorPropagates, TestSetDeps_OverridesDeps, TestManagerHooks_InitiallyNil — they assert a fake recorded its own input, that a fake returns an error literally constructed in the test, or Go zero-value/direct-assignment semantics. Compile-time interface checks already prove satisfaction. state_test.go: replace TestEC2StateCodes_AllStatesHaveMapping and TestEC2StateCodes_CorrectValues — both compared the production map to its own literal. Now assert against an independent AWS contract using ec2.InstanceStateName* SDK enum constants paired with the AWS-documented numeric codes; a typo in production no longer replicates into the test. watchdog_test.go: replace TestStartPendingWatchdog_CtxCancelStopsGoroutine's time.Sleep(10ms) "assertion" with goleak.VerifyNone. The previous form passed even if the goroutine never returned on cancel — the harness reaped it on test exit. --- spinifex/vm/manager_deps_test.go | 93 -------------------------------- spinifex/vm/state_test.go | 77 ++++++++++++++++---------- spinifex/vm/watchdog_test.go | 13 +++-- 3 files changed, 54 insertions(+), 129 deletions(-) diff --git a/spinifex/vm/manager_deps_test.go b/spinifex/vm/manager_deps_test.go index ebef7c6ef..58f0293bf 100644 --- a/spinifex/vm/manager_deps_test.go +++ b/spinifex/vm/manager_deps_test.go @@ -1,7 +1,6 @@ package vm import ( - "errors" "maps" "sync" "testing" @@ -105,37 +104,6 @@ func TestNewManagerWithDeps_StoresDeps(t *testing.T) { } } -func TestSetDeps_OverridesDeps(t *testing.T) { - m := NewManager() - if m.NodeID() != "" { - t.Fatalf("NodeID on default manager: got %q, want empty", m.NodeID()) - } - - deps := Deps{NodeID: "node-b"} - m.SetDeps(deps) - if m.NodeID() != "node-b" { - t.Fatalf("NodeID after SetDeps: got %q, want %q", m.NodeID(), "node-b") - } - - // Replacing deps fully overwrites prior deps, not merges. - m.SetDeps(Deps{NodeID: "node-c"}) - if m.NodeID() != "node-c" { - t.Fatalf("NodeID after second SetDeps: got %q, want %q", m.NodeID(), "node-c") - } -} - -func TestManagerHooks_InitiallyNil(t *testing.T) { - // A manager constructed without hooks must expose nil hook fields so - // call sites can no-op rather than panicking. - m := NewManager() - if m.deps.Hooks.OnInstanceUp != nil { - t.Fatal("OnInstanceUp: got non-nil on default manager") - } - if m.deps.Hooks.OnInstanceDown != nil { - t.Fatal("OnInstanceDown: got non-nil on default manager") - } -} - // fakeVolumeMounter records every call so lifecycle tests can assert ordering. // The mutex covers the recording slices so StopAll's per-instance fan-out // goroutines stay race-free. @@ -193,64 +161,3 @@ func (f *fakeVolumeMounter) UnmountOne(req types.EBSRequest) { } var _ VolumeMounter = (*fakeVolumeMounter)(nil) - -func TestDeps_VolumeMounterSatisfiesInterface(t *testing.T) { - mounter := &fakeVolumeMounter{} - deps := Deps{VolumeMounter: mounter} - m := NewManagerWithDeps(deps) - - if err := m.deps.VolumeMounter.Mount(&VM{ID: "i-1"}); err != nil { - t.Fatalf("Mount: %v", err) - } - if err := m.deps.VolumeMounter.Unmount(&VM{ID: "i-1"}); err != nil { - t.Fatalf("Unmount: %v", err) - } - if len(mounter.mounted) != 1 || mounter.mounted[0] != "i-1" { - t.Fatalf("mounted: got %v, want [i-1]", mounter.mounted) - } - if len(mounter.unmounted) != 1 || mounter.unmounted[0] != "i-1" { - t.Fatalf("unmounted: got %v, want [i-1]", mounter.unmounted) - } -} - -func TestDeps_StateStoreSatisfiesInterface(t *testing.T) { - store := newFakeStateStore() - deps := Deps{NodeID: "n", StateStore: store} - m := NewManagerWithDeps(deps) - - if err := m.deps.StateStore.WriteStoppedInstance("i-1", &VM{ID: "i-1"}); err != nil { - t.Fatalf("WriteStoppedInstance: %v", err) - } - if got := store.stopped["i-1"]; got == nil || got.ID != "i-1" { - t.Fatalf("WriteStoppedInstance: got %v, want VM{ID:i-1}", got) - } - - if err := m.deps.StateStore.SaveRunningState("n", map[string]*VM{"i-1": {ID: "i-1"}}); err != nil { - t.Fatalf("SaveRunningState: %v", err) - } - loaded, err := m.deps.StateStore.LoadRunningState("n") - if err != nil { - t.Fatalf("LoadRunningState: %v", err) - } - if len(loaded) != 1 { - t.Fatalf("LoadRunningState: got %d entries, want 1", len(loaded)) - } -} - -// failStateStore returns errors so lifecycle tests can verify propagation -// when the manager wires through StateStore. -type failStateStore struct{ *fakeStateStore } - -func (failStateStore) SaveRunningState(string, map[string]*VM) error { - return errors.New("save failed") -} - -func TestDeps_StateStoreErrorPropagates(t *testing.T) { - deps := Deps{NodeID: "n", StateStore: failStateStore{newFakeStateStore()}} - m := NewManagerWithDeps(deps) - - err := m.deps.StateStore.SaveRunningState("n", nil) - if err == nil { - t.Fatal("SaveRunningState: expected error from failStateStore, got nil") - } -} diff --git a/spinifex/vm/state_test.go b/spinifex/vm/state_test.go index 24f3e3bd1..cd6addc84 100644 --- a/spinifex/vm/state_test.go +++ b/spinifex/vm/state_test.go @@ -3,6 +3,7 @@ package vm import ( "testing" + "github.com/aws/aws-sdk-go/service/ec2" "github.com/stretchr/testify/assert" ) @@ -20,40 +21,58 @@ func TestIsValidTransition(t *testing.T) { assert.False(t, IsValidTransition(StateStopped, StateStopping)) } -func TestEC2StateCodes_AllStatesHaveMapping(t *testing.T) { - allStates := []InstanceState{ - StateProvisioning, - StatePending, - StateRunning, - StateStopping, - StateStopped, - StateShuttingDown, - StateTerminated, - StateError, - } +// awsEC2StateCodes is the AWS-documented EC2 InstanceState code/name +// contract (see API_InstanceState). Names come from the SDK's enum +// constants; codes are fixed by the AWS API and have no SDK constant. +// Verifying our production map against an independent literal protects +// against a typo replicated identically in test and production. +var awsEC2StateCodes = map[string]int64{ + ec2.InstanceStateNamePending: 0, + ec2.InstanceStateNameRunning: 16, + ec2.InstanceStateNameShuttingDown: 32, + ec2.InstanceStateNameTerminated: 48, + ec2.InstanceStateNameStopping: 64, + ec2.InstanceStateNameStopped: 80, +} - for _, s := range allStates { +func TestEC2StateCodes_AllInstanceStatesMapped(t *testing.T) { + for _, s := range []InstanceState{ + StateProvisioning, StatePending, StateRunning, StateStopping, + StateStopped, StateShuttingDown, StateTerminated, StateError, + } { info, ok := EC2StateCodes[s] - assert.True(t, ok, "State %s should have an EC2 mapping", s) - assert.NotEmpty(t, info.Name, "State %s EC2 name should not be empty", s) + assert.True(t, ok, "InstanceState %s missing from EC2StateCodes", s) + assert.NotEmpty(t, info.Name, "InstanceState %s mapped to empty EC2 name", s) } } -func TestEC2StateCodes_CorrectValues(t *testing.T) { - expected := map[InstanceState]EC2StateInfo{ - StateProvisioning: {Code: 0, Name: "pending"}, - StatePending: {Code: 0, Name: "pending"}, - StateRunning: {Code: 16, Name: "running"}, - StateStopping: {Code: 64, Name: "stopping"}, - StateStopped: {Code: 80, Name: "stopped"}, - StateShuttingDown: {Code: 32, Name: "shutting-down"}, - StateTerminated: {Code: 48, Name: "terminated"}, - StateError: {Code: 0, Name: "error"}, +func TestEC2StateCodes_MatchAWSContract(t *testing.T) { + // Production states with a direct AWS equivalent must match the AWS + // (code, name) tuple exactly. State{Provisioning,Error} are + // Spinifex-only and verified separately below. + awsBacked := map[InstanceState]string{ + StatePending: ec2.InstanceStateNamePending, + StateRunning: ec2.InstanceStateNameRunning, + StateStopping: ec2.InstanceStateNameStopping, + StateStopped: ec2.InstanceStateNameStopped, + StateShuttingDown: ec2.InstanceStateNameShuttingDown, + StateTerminated: ec2.InstanceStateNameTerminated, } - - for state, exp := range expected { - actual := EC2StateCodes[state] - assert.Equal(t, exp.Code, actual.Code, "State %s code mismatch", state) - assert.Equal(t, exp.Name, actual.Name, "State %s name mismatch", state) + for state, awsName := range awsBacked { + got := EC2StateCodes[state] + assert.Equal(t, awsName, got.Name, + "InstanceState %s name diverges from AWS contract", state) + assert.Equal(t, awsEC2StateCodes[awsName], got.Code, + "InstanceState %s code diverges from AWS contract", state) } } + +func TestEC2StateCodes_SpinifexOnlyStatesPresentAsPending(t *testing.T) { + // Provisioning is a pre-launch internal state; it surfaces to AWS + // callers as "pending" so DescribeInstances returns a code AWS clients + // understand. Error is a terminal-ish failure state with no AWS + // equivalent — code 0 is the safest fallback ("pending" code). + assert.Equal(t, "pending", EC2StateCodes[StateProvisioning].Name) + assert.Equal(t, int64(0), EC2StateCodes[StateProvisioning].Code) + assert.Equal(t, int64(0), EC2StateCodes[StateError].Code) +} diff --git a/spinifex/vm/watchdog_test.go b/spinifex/vm/watchdog_test.go index c9226144c..7a00a7935 100644 --- a/spinifex/vm/watchdog_test.go +++ b/spinifex/vm/watchdog_test.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) // pendingInstance returns a VM in the supplied state with LaunchTime set @@ -116,18 +117,16 @@ func TestScanAndMarkStuckPending_OnlyPendingStatesScanned(t *testing.T) { } func TestStartPendingWatchdog_CtxCancelStopsGoroutine(t *testing.T) { + // goleak fails the test if the watchdog goroutine outlives ctx. + // Without this, a regression that ignored ctx.Done would still pass: + // the harness reaps the leaked goroutine on test process exit. + defer goleak.VerifyNone(t) + m, _, _, _ := crashTestManager(t) ctx, cancel := context.WithCancel(t.Context()) m.StartPendingWatchdog(ctx) - - // Immediately cancel; with PendingWatchdogInterval at 60s, the only - // observable signal that the goroutine respected ctx.Done is that the - // test exits cleanly without waiting for the next tick. cancel() - // Yield briefly so the goroutine actually returns before the test - // finishes; race detector catches any leaked work. - time.Sleep(10 * time.Millisecond) } func assertStuckMarkedFailed(t *testing.T, m *Manager, rt *recordedTransitions, v *VM) {