From 4abd2fd984b3392b7410e4d3d48efc2f06a400e7 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:41:00 +1000 Subject: [PATCH] fix: delete crap tests --- spinifex/admin/admin_test.go | 10 - .../handlers/eks/cluster_reconciler_test.go | 2 +- spinifex/network/external/dhcp/client_test.go | 32 -- spinifex/network/host/host_test.go | 8 - spinifex/network/reconcile/liveness_test.go | 8 - .../services/viperblockd/viperblockd_test.go | 339 ------------------ 6 files changed, 1 insertion(+), 398 deletions(-) diff --git a/spinifex/admin/admin_test.go b/spinifex/admin/admin_test.go index b74bbab3c..c3857110a 100644 --- a/spinifex/admin/admin_test.go +++ b/spinifex/admin/admin_test.go @@ -1080,16 +1080,6 @@ func TestChownServicePaths_SkipsMissingPath(t *testing.T) { assert.NoError(t, err) } -func TestSetServiceOwnership_RequiresRoot(t *testing.T) { - // SetServiceOwnership operates on hardcoded /etc/spinifex and - // /var/lib/spinifex paths and chowns to the spinifex group, both of - // which require root. The per-service failure-aggregation logic itself - // is covered without root via TestChownServicePaths_MissingUser above. - if os.Geteuid() != 0 { - t.Skip("SetServiceOwnership requires root") - } -} - // --- SetGPUPassthrough --- func writeToml(t *testing.T, content string) string { diff --git a/spinifex/handlers/eks/cluster_reconciler_test.go b/spinifex/handlers/eks/cluster_reconciler_test.go index 3da6987b6..bff3a6346 100644 --- a/spinifex/handlers/eks/cluster_reconciler_test.go +++ b/spinifex/handlers/eks/cluster_reconciler_test.go @@ -97,7 +97,7 @@ func TestClusterReconciler_AcquireLeaseFirstHolderWins(t *testing.T) { require.NotNil(t, release) defer release() - // Second AcquireLease from same holder must fail (Create returns KeyExists). + // Re-acquiring from the same holder succeeds: the lease is already held. release2, ok2 := r.AcquireLease(t.Context()) assert.True(t, ok2) assert.NotNil(t, release2) diff --git a/spinifex/network/external/dhcp/client_test.go b/spinifex/network/external/dhcp/client_test.go index 8f248d28f..6c67ead6e 100644 --- a/spinifex/network/external/dhcp/client_test.go +++ b/spinifex/network/external/dhcp/client_test.go @@ -2,7 +2,6 @@ package dhcp_test import ( "context" - "errors" "net" "strings" "testing" @@ -52,17 +51,6 @@ func TestFakeAcquireReturnsLeaseAndTracksCount(t *testing.T) { } } -func TestFakeAcquireHookOverridesDefault(t *testing.T) { - f := dhcp.NewFake() - f.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) { - return nil, errors.New("injected") - } - _, err := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "x"}) - if err == nil || err.Error() != "injected" { - t.Fatalf("expected injected error, got %v", err) - } -} - func TestFakeRenewRefreshesAcquiredAt(t *testing.T) { f := dhcp.NewFake() lease, err := f.Acquire(context.Background(), dhcp.AcquireRequest{ @@ -102,26 +90,6 @@ func TestFakeReleaseClearsTrackedLease(t *testing.T) { } } -func TestFakeRenewHookSurfacesError(t *testing.T) { - f := dhcp.NewFake() - f.RenewHook = func(*dhcp.Lease) (*dhcp.Lease, error) { - return nil, errors.New("server NAK") - } - lease, _ := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "eni-4"}) - if _, err := f.Renew(context.Background(), lease); err == nil { - t.Fatal("expected hook error") - } -} - -func TestFakeReleaseHookSurfacesError(t *testing.T) { - f := dhcp.NewFake() - f.ReleaseHook = func(*dhcp.Lease) error { return errors.New("server unreachable") } - lease, _ := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "eni-rel"}) - if err := f.Release(context.Background(), lease); err == nil { - t.Fatal("expected hook error") - } -} - func TestFakeRenewNilLease(t *testing.T) { f := dhcp.NewFake() if _, err := f.Renew(context.Background(), nil); err == nil { diff --git a/spinifex/network/host/host_test.go b/spinifex/network/host/host_test.go index 26f6a389b..8f530de75 100644 --- a/spinifex/network/host/host_test.go +++ b/spinifex/network/host/host_test.go @@ -6,7 +6,6 @@ import ( "fmt" "net" "net/netip" - "slices" "strings" "sync" "testing" @@ -281,10 +280,3 @@ var ( _ Runner = execRunner{} _ InterfaceReader = kernelReader{} ) - -func TestUplinkModes_ConstantSet(t *testing.T) { - modes := []UplinkMode{UplinkModeUnknown, UplinkModePhysical, UplinkModeVeth} - if !slices.Contains(modes, UplinkModePhysical) || !slices.Contains(modes, UplinkModeVeth) { - t.Fatal("expected physical and veth in supported mode set") - } -} diff --git a/spinifex/network/reconcile/liveness_test.go b/spinifex/network/reconcile/liveness_test.go index 088df9f39..ffe2cb623 100644 --- a/spinifex/network/reconcile/liveness_test.go +++ b/spinifex/network/reconcile/liveness_test.go @@ -64,14 +64,6 @@ func TestL2_DriftIntervalBounded(t *testing.T) { // Blocked on a clock-injectable DriftLoop signature. } -// TestL3_FederationReEnablesOnLinkRecovery is a placeholder for ADR-0006 L3 -// (OVN-IC tunnel re-establishment on inter-AZ link recovery). -func TestL3_FederationReEnablesOnLinkRecovery(t *testing.T) { - t.Skip(`ADR-0006 L3 deferred: network/federation/ has not yet been built. ` + - `Once it lands, replace this stub with a test that drives a fake ` + - `LinkObserver from Down → Degraded and asserts BringUpLink fires.`) -} - // scaledIntent produces an IntentState with n VPCs, each with one subnet, port, and SG. func scaledIntent(t *testing.T, n int) IntentState { t.Helper() diff --git a/spinifex/services/viperblockd/viperblockd_test.go b/spinifex/services/viperblockd/viperblockd_test.go index af62a7d6c..554919914 100644 --- a/spinifex/services/viperblockd/viperblockd_test.go +++ b/spinifex/services/viperblockd/viperblockd_test.go @@ -6,9 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" - "os" "path/filepath" - "sync" "sync/atomic" "testing" "time" @@ -55,106 +53,6 @@ func TestNewWithNilConfig(t *testing.T) { assert.NotNil(t, svc.Config) } -// TestConfigDefaults tests Config struct default values. -func TestConfigDefaults(t *testing.T) { - cfg := &Config{} - - assert.Empty(t, cfg.NatsHost) - assert.Empty(t, cfg.S3Host) - assert.Empty(t, cfg.Bucket) - assert.Empty(t, cfg.Region) - assert.False(t, cfg.Debug) - assert.Empty(t, cfg.MountedVolumes) -} - -// TestConfigWithDebug tests Config with debug enabled. -func TestConfigWithDebug(t *testing.T) { - cfg := &Config{ - Debug: true, - NatsHost: "nats://localhost:4222", - BaseDir: "/tmp/test", - } - - assert.True(t, cfg.Debug) - assert.Equal(t, "nats://localhost:4222", cfg.NatsHost) - assert.Equal(t, "/tmp/test", cfg.BaseDir) -} - -// TestMountedVolumeStruct tests the MountedVolume struct. -func TestMountedVolumeStruct(t *testing.T) { - vol := MountedVolume{ - Name: "vol-123", - Port: 10809, - Socket: "/tmp/vol-123.sock", - PID: 12345, - } - - assert.Equal(t, "vol-123", vol.Name) - assert.Equal(t, 10809, vol.Port) - assert.Equal(t, "/tmp/vol-123.sock", vol.Socket) - assert.Equal(t, 12345, vol.PID) -} - -// TestConfigMountedVolumesAppend tests adding volumes to the config. -func TestConfigMountedVolumesAppend(t *testing.T) { - cfg := &Config{ - MountedVolumes: []MountedVolume{}, - } - - assert.Empty(t, cfg.MountedVolumes) - - // Add first volume - cfg.MountedVolumes = append(cfg.MountedVolumes, MountedVolume{ - Name: "vol-1", - Port: 10809, - PID: 100, - }) - - assert.Len(t, cfg.MountedVolumes, 1) - assert.Equal(t, "vol-1", cfg.MountedVolumes[0].Name) - - // Add second volume - cfg.MountedVolumes = append(cfg.MountedVolumes, MountedVolume{ - Name: "vol-2", - Port: 10810, - PID: 101, - }) - - assert.Len(t, cfg.MountedVolumes, 2) - assert.Equal(t, "vol-2", cfg.MountedVolumes[1].Name) -} - -// TestConfigMutexThreadSafety tests that the mutex protects MountedVolumes. -func TestConfigMutexThreadSafety(t *testing.T) { - cfg := &Config{ - MountedVolumes: []MountedVolume{}, - } - - var wg sync.WaitGroup - iterations := 100 - - // Spawn multiple goroutines to append volumes concurrently - for i := range iterations { - wg.Add(1) - go func(id int) { - defer wg.Done() - cfg.mu.Lock() - cfg.MountedVolumes = append(cfg.MountedVolumes, MountedVolume{ - Name: "vol-" + string(rune(id)), - Port: 10809 + id, - PID: 1000 + id, - }) - cfg.mu.Unlock() - }(i) - } - - wg.Wait() - - cfg.mu.Lock() - assert.Len(t, cfg.MountedVolumes, iterations) - cfg.mu.Unlock() -} - // TestServiceMethods tests the service interface methods. func TestServiceMethods(t *testing.T) { cfg := &Config{ @@ -209,248 +107,11 @@ func TestServiceStop(t *testing.T) { assert.Error(t, err) } -// TestConfigPluginPath tests PluginPath configuration. -func TestConfigPluginPath(t *testing.T) { - cfg := &Config{ - PluginPath: "/usr/lib/nbdkit/plugins", - BaseDir: "/var/lib/viperblock", - } - - assert.Equal(t, "/usr/lib/nbdkit/plugins", cfg.PluginPath) - assert.Equal(t, "/var/lib/viperblock", cfg.BaseDir) -} - -// TestConfigFullyPopulated tests a fully populated config. -func TestConfigFullyPopulated(t *testing.T) { - cfg := &Config{ - ConfigPath: "/etc/viperblock/config.yml", - PluginPath: "/usr/lib/nbdkit/plugins", - Debug: true, - NatsHost: "nats://10.0.0.1:4222", - S3Host: "https://s3.us-west-2.amazonaws.com", - Bucket: "my-volumes", - Region: "us-west-2", - AccessKey: "AKIAIOSFODNN7EXAMPLE", - SecretKey: "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", - BaseDir: "/var/lib/viperblock", - MountedVolumes: []MountedVolume{ - { - Name: "vol-001", - Port: 10809, - Socket: "/tmp/vol-001.sock", - PID: 5000, - }, - }, - } - - assert.Equal(t, "/etc/viperblock/config.yml", cfg.ConfigPath) - assert.Equal(t, "/usr/lib/nbdkit/plugins", cfg.PluginPath) - assert.True(t, cfg.Debug) - assert.Equal(t, "nats://10.0.0.1:4222", cfg.NatsHost) - assert.Equal(t, "https://s3.us-west-2.amazonaws.com", cfg.S3Host) - assert.Equal(t, "my-volumes", cfg.Bucket) - assert.Equal(t, "us-west-2", cfg.Region) - assert.Equal(t, "AKIAIOSFODNN7EXAMPLE", cfg.AccessKey) - assert.Equal(t, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", cfg.SecretKey) - assert.Equal(t, "/var/lib/viperblock", cfg.BaseDir) - assert.Len(t, cfg.MountedVolumes, 1) - assert.Equal(t, "vol-001", cfg.MountedVolumes[0].Name) -} - -// TestMountedVolumeFindByName tests finding a mounted volume by name. -func TestMountedVolumeFindByName(t *testing.T) { - cfg := &Config{ - MountedVolumes: []MountedVolume{ - {Name: "vol-1", Port: 10809, PID: 100}, - {Name: "vol-2", Port: 10810, PID: 101}, - {Name: "vol-3", Port: 10811, PID: 102}, - }, - } - - // Find existing volume - var found *MountedVolume - for _, vol := range cfg.MountedVolumes { - if vol.Name == "vol-2" { - found = &vol - break - } - } - - assert.NotNil(t, found) - assert.Equal(t, "vol-2", found.Name) - assert.Equal(t, 10810, found.Port) - assert.Equal(t, 101, found.PID) - - // Try to find non-existent volume - found = nil - for _, vol := range cfg.MountedVolumes { - if vol.Name == "vol-999" { - found = &vol - break - } - } - - assert.Nil(t, found) -} - -// TestMountedVolumeRemoval tests removing a mounted volume. -func TestMountedVolumeRemoval(t *testing.T) { - cfg := &Config{ - MountedVolumes: []MountedVolume{ - {Name: "vol-1", Port: 10809, PID: 100}, - {Name: "vol-2", Port: 10810, PID: 101}, - {Name: "vol-3", Port: 10811, PID: 102}, - }, - } - - assert.Len(t, cfg.MountedVolumes, 3) - - // Remove vol-2 - cfg.mu.Lock() - for i, vol := range cfg.MountedVolumes { - if vol.Name == "vol-2" { - cfg.MountedVolumes = append(cfg.MountedVolumes[:i], cfg.MountedVolumes[i+1:]...) - break - } - } - cfg.mu.Unlock() - - assert.Len(t, cfg.MountedVolumes, 2) - assert.Equal(t, "vol-1", cfg.MountedVolumes[0].Name) - assert.Equal(t, "vol-3", cfg.MountedVolumes[1].Name) -} - // TestServiceNameConstant tests the serviceName constant. func TestServiceNameConstant(t *testing.T) { assert.Equal(t, "viperblock", serviceName) } -// TestConfigS3Credentials tests S3 credential fields. -func TestConfigS3Credentials(t *testing.T) { - cfg := &Config{ - AccessKey: "test-key", - SecretKey: "test-secret", - } - - assert.NotEmpty(t, cfg.AccessKey) - assert.NotEmpty(t, cfg.SecretKey) - assert.Equal(t, "test-key", cfg.AccessKey) - assert.Equal(t, "test-secret", cfg.SecretKey) -} - -// TestMountedVolumeWithSocket tests MountedVolume with socket path. -func TestMountedVolumeWithSocket(t *testing.T) { - vol := MountedVolume{ - Name: "vol-ebs-123", - Port: 10809, - Socket: "/var/run/viperblock/vol-ebs-123.sock", - PID: 9876, - } - - assert.Contains(t, vol.Socket, "vol-ebs-123") -} - -// TestConfigConcurrentRead tests concurrent reads are safe. -func TestConfigConcurrentRead(t *testing.T) { - cfg := &Config{ - MountedVolumes: []MountedVolume{ - {Name: "vol-1", Port: 10809, PID: 100}, - }, - } - - var wg sync.WaitGroup - iterations := 50 - - // Spawn multiple goroutines to read volumes concurrently - for range iterations { - wg.Go(func() { - cfg.mu.Lock() - _ = len(cfg.MountedVolumes) - if len(cfg.MountedVolumes) > 0 { - _ = cfg.MountedVolumes[0].Name - } - cfg.mu.Unlock() - }) - } - - wg.Wait() - - // Should still have original volume - cfg.mu.Lock() - assert.Len(t, cfg.MountedVolumes, 1) - cfg.mu.Unlock() -} - -// TestServiceStartWithoutNATS tests Start method behavior -// Note: This will fail without a running NATS server, which is expected. -func TestServiceStartWithoutNATS(t *testing.T) { - // Skip this test in CI environments or when NATS is not available - if os.Getenv("SKIP_INTEGRATION_TESTS") == "true" { - t.Skip("Skipping integration test") - } - - // Port 1 is never listening, so the dial is refused immediately rather than - // racing a NATS server that may be running on the default port. - cfg := &Config{ - NatsHost: "nats://127.0.0.1:1", - BaseDir: "/tmp/viperblock-test", - } - - svc, err := New(cfg) - assert.NoError(t, err) - - // Start in a goroutine since it blocks - errChan := make(chan error, 1) - go func() { - _, err := svc.Start() - errChan <- err - }() - - // Wait a short time for connection attempt - select { - case err := <-errChan: - // Expected to fail without NATS server - assert.Error(t, err) - case <-time.After(200 * time.Millisecond): - // Timeout is also acceptable - t.Log("Start() timed out as expected without NATS server") - } -} - -// TestConfigValidation tests validation of required fields. -func TestConfigValidation(t *testing.T) { - // Test empty config - fields should be empty - cfg := &Config{} - assert.Empty(t, cfg.NatsHost) - assert.Empty(t, cfg.Bucket) - assert.Empty(t, cfg.Region) - - // Test minimal valid config - cfg2 := &Config{ - NatsHost: "nats://localhost:4222", - Bucket: "test-bucket", - Region: "us-east-1", - } - assert.NotEmpty(t, cfg2.NatsHost) - assert.NotEmpty(t, cfg2.Bucket) - assert.NotEmpty(t, cfg2.Region) -} - -// TestMountedVolumePortRange tests port assignment. -func TestMountedVolumePortRange(t *testing.T) { - volumes := []MountedVolume{ - {Name: "vol-1", Port: 10809}, - {Name: "vol-2", Port: 10810}, - {Name: "vol-3", Port: 10811}, - } - - for i, vol := range volumes { - assert.Equal(t, 10809+i, vol.Port) - assert.GreaterOrEqual(t, vol.Port, 10809) - assert.LessOrEqual(t, vol.Port, 65535) - } -} - func TestConfigLoadStateRetryPolicy(t *testing.T) { attempts, delay := (&Config{}).loadStateRetryPolicy() assert.Equal(t, defaultLoadStateRetryAttempts, attempts)