From e210141b8f6f19f9a22b894c6360b494e6fc4419 Mon Sep 17 00:00:00 2001 From: Sky Johnson Date: Sat, 2 Aug 2025 12:18:01 -0500 Subject: [PATCH] simplify and improve faction master list validation --- internal/factions/concurrency_test.go | 221 +++++++++++------------ internal/factions/manager.go | 2 +- internal/factions/master_faction_list.go | 55 +++--- 3 files changed, 135 insertions(+), 143 deletions(-) diff --git a/internal/factions/concurrency_test.go b/internal/factions/concurrency_test.go index fb7210b..b548a0e 100644 --- a/internal/factions/concurrency_test.go +++ b/internal/factions/concurrency_test.go @@ -9,34 +9,34 @@ import ( // Stress test MasterFactionList with concurrent operations func TestMasterFactionListConcurrency(t *testing.T) { mfl := NewMasterFactionList() - + // Pre-populate with some factions for i := int32(1); i <= 10; i++ { faction := NewFaction(i, "Test Faction", "Test", "Test faction") mfl.AddFaction(faction) } - + const numGoroutines = 100 const operationsPerGoroutine = 100 - + var wg sync.WaitGroup - + t.Run("ConcurrentFactionAccess", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Mix of read operations faction := mfl.GetFaction(factionID) if faction == nil { t.Errorf("Goroutine %d: Expected faction %d to exist", goroutineID, factionID) } - + _ = mfl.GetFactionByName("Test Faction") _ = mfl.HasFaction(factionID) _ = mfl.GetFactionCount() @@ -46,21 +46,21 @@ func TestMasterFactionListConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentRelationshipManagement", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - targetID := int32(((goroutineID+1)%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + targetID := int32(((goroutineID + 1) % 10) + 1) + // Concurrent relationship operations if goroutineID%2 == 0 { mfl.AddHostileFaction(factionID, targetID) @@ -72,26 +72,26 @@ func TestMasterFactionListConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentFactionModification", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { newFactionID := int32(1000 + goroutineID*operationsPerGoroutine + j) faction := NewFaction(newFactionID, "Concurrent Faction", "Test", "Concurrent test") - + err := mfl.AddFaction(faction) if err != nil { t.Errorf("Goroutine %d: Failed to add faction %d: %v", goroutineID, newFactionID, err) } - + // Verify it was added retrieved := mfl.GetFaction(newFactionID) if retrieved == nil { @@ -100,7 +100,7 @@ func TestMasterFactionListConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) } @@ -108,7 +108,7 @@ func TestMasterFactionListConcurrency(t *testing.T) { // Stress test PlayerFaction with concurrent operations func TestPlayerFactionConcurrency(t *testing.T) { mfl := NewMasterFactionList() - + // Add test factions with proper values for i := int32(1); i <= 10; i++ { faction := NewFaction(i, "Test Faction", "Test", "Test faction") @@ -116,24 +116,24 @@ func TestPlayerFactionConcurrency(t *testing.T) { faction.NegativeChange = 50 mfl.AddFaction(faction) } - + pf := NewPlayerFaction(mfl) - + const numGoroutines = 100 const operationsPerGoroutine = 100 - + var wg sync.WaitGroup - + t.Run("ConcurrentFactionValueOperations", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Mix of faction value operations switch j % 4 { case 0: @@ -150,27 +150,27 @@ func TestPlayerFactionConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentUpdateManagement", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Operations that trigger updates pf.IncreaseFaction(factionID, 1) - + // Check for pending updates _ = pf.HasPendingUpdates() _ = pf.GetPendingUpdates() - + // Occasionally clear updates if j%10 == 0 { pf.ClearPendingUpdates() @@ -178,22 +178,22 @@ func TestPlayerFactionConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentPacketGeneration", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { // Trigger faction updates - factionID := int32((goroutineID%10) + 1) + factionID := int32((goroutineID % 10) + 1) pf.IncreaseFaction(factionID, 1) - + // Generate packets concurrently _, err := pf.FactionUpdate(int16(goroutineID % 10)) if err != nil { @@ -202,7 +202,7 @@ func TestPlayerFactionConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) } @@ -210,22 +210,22 @@ func TestPlayerFactionConcurrency(t *testing.T) { // Stress test Manager with concurrent operations func TestManagerConcurrency(t *testing.T) { manager := NewManager(nil, nil) // No database or logger for testing - + const numGoroutines = 100 const operationsPerGoroutine = 100 - + var wg sync.WaitGroup - + t.Run("ConcurrentStatisticsOperations", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Mix of statistics operations switch j % 4 { case 0: @@ -240,56 +240,56 @@ func TestManagerConcurrency(t *testing.T) { } }(i) } - + wg.Wait() - + // Verify statistics consistency stats := manager.GetStatistics() totalChanges := stats["total_faction_changes"].(int64) increases := stats["faction_increases"].(int64) decreases := stats["faction_decreases"].(int64) - + if totalChanges != increases+decreases { - t.Errorf("Statistics inconsistency: total %d != increases %d + decreases %d", + t.Errorf("Statistics inconsistency: total %d != increases %d + decreases %d", totalChanges, increases, decreases) } }) - + t.Run("ConcurrentFactionManagement", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { factionID := int32(2000 + goroutineID*operationsPerGoroutine + j) faction := NewFaction(factionID, "Manager Test", "Test", "Manager test") - + // Add faction err := manager.AddFaction(faction) if err != nil { t.Errorf("Goroutine %d: Failed to add faction %d: %v", goroutineID, factionID, err) continue } - + // Read operations _ = manager.GetFaction(factionID) _ = manager.GetFactionByName("Manager Test") } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentPlayerFactionCreation", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { // Create player factions concurrently pf := manager.CreatePlayerFaction() @@ -299,14 +299,14 @@ func TestManagerConcurrency(t *testing.T) { } }(i) } - + wg.Wait() - + // Check statistics stats := manager.GetStatistics() expectedPlayers := int64(numGoroutines * operationsPerGoroutine) actualPlayers := stats["players_with_factions"].(int64) - + if actualPlayers != expectedPlayers { t.Errorf("Expected %d players with factions, got %d", expectedPlayers, actualPlayers) } @@ -316,26 +316,26 @@ func TestManagerConcurrency(t *testing.T) { // Stress test EntityFactionAdapter with concurrent operations func TestEntityFactionAdapterConcurrency(t *testing.T) { manager := NewManager(nil, nil) - + // Mock entity entity := &mockEntity{id: 123, name: "Test Entity", dbID: 456} adapter := NewEntityFactionAdapter(entity, manager, nil) - + const numGoroutines = 100 const operationsPerGoroutine = 100 - + var wg sync.WaitGroup - + t.Run("ConcurrentFactionIDOperations", func(t *testing.T) { wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Mix of faction ID operations switch j % 3 { case 0: @@ -349,43 +349,43 @@ func TestEntityFactionAdapterConcurrency(t *testing.T) { } }(i) } - + wg.Wait() }) - + t.Run("ConcurrentRelationshipChecks", func(t *testing.T) { // Set up some factions first for i := int32(1); i <= 10; i++ { faction := NewFaction(i, "Test Faction", "Test", "Test faction") manager.AddFaction(faction) } - + // Set up relationships mfl := manager.GetMasterFactionList() mfl.AddHostileFaction(1, 2) mfl.AddFriendlyFaction(1, 3) - + adapter.SetFactionID(1) - + wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - otherFactionID := int32((goroutineID%10) + 1) - + otherFactionID := int32((goroutineID % 10) + 1) + // Concurrent relationship checks _ = adapter.IsHostileToFaction(otherFactionID) _ = adapter.IsFriendlyToFaction(otherFactionID) - + // Validation _ = adapter.ValidateFaction() } }(i) } - + wg.Wait() }) } @@ -413,20 +413,20 @@ func (e *mockEntity) GetDatabaseID() int32 { func TestDeadlockPrevention(t *testing.T) { mfl := NewMasterFactionList() manager := NewManager(nil, nil) - + // Add test factions for i := int32(1); i <= 10; i++ { faction := NewFaction(i, "Test Faction", "Test", "Test faction") manager.AddFaction(faction) } - + const numGoroutines = 50 var wg sync.WaitGroup - + // Test potential deadlock scenarios t.Run("MixedOperations", func(t *testing.T) { done := make(chan bool, 1) - + // Set a timeout to detect deadlocks go func() { time.Sleep(10 * time.Second) @@ -435,19 +435,18 @@ func TestDeadlockPrevention(t *testing.T) { return default: t.Error("Potential deadlock detected - test timed out") - t.FailNow() } }() - + wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < 100; j++ { - factionID := int32((goroutineID%10) + 1) - + factionID := int32((goroutineID % 10) + 1) + // Mix operations that could potentially deadlock switch j % 6 { case 0: @@ -473,7 +472,7 @@ func TestDeadlockPrevention(t *testing.T) { } }(i) } - + wg.Wait() done <- true }) @@ -484,42 +483,42 @@ func TestRaceConditions(t *testing.T) { if testing.Short() { t.Skip("Skipping race condition test in short mode") } - + // This test is designed to be run with: go test -race mfl := NewMasterFactionList() manager := NewManager(nil, nil) - + // Rapid concurrent operations to trigger race conditions const numGoroutines = 200 const operationsPerGoroutine = 50 - + var wg sync.WaitGroup wg.Add(numGoroutines) - + for i := 0; i < numGoroutines; i++ { go func(goroutineID int) { defer wg.Done() - + for j := 0; j < operationsPerGoroutine; j++ { - factionID := int32((goroutineID%20) + 1) - + factionID := int32((goroutineID % 20) + 1) + // Rapid-fire operations faction := NewFaction(factionID, "Race Test", "Test", "Race test") mfl.AddFaction(faction) _ = mfl.GetFaction(factionID) mfl.AddHostileFaction(factionID, factionID+1) _ = mfl.GetHostileFactions(factionID) - + manager.AddFaction(faction) manager.RecordFactionIncrease(factionID) _ = manager.GetStatistics() - + pf := manager.CreatePlayerFaction() pf.IncreaseFaction(factionID, 1) _ = pf.GetFactionValue(factionID) } }(i) } - + wg.Wait() -} \ No newline at end of file +} diff --git a/internal/factions/manager.go b/internal/factions/manager.go index e6e57d7..6f9627e 100644 --- a/internal/factions/manager.go +++ b/internal/factions/manager.go @@ -393,7 +393,7 @@ func (m *Manager) handleInfoCommand(args []string) (string, error) { return fmt.Sprintf("Faction '%s' not found.", args[0]), nil } - result := fmt.Sprintf("Faction Information:\n") + result := "Faction Information:\n" result += fmt.Sprintf("ID: %d\n", faction.ID) result += fmt.Sprintf("Name: %s\n", faction.Name) result += fmt.Sprintf("Type: %s\n", faction.Type) diff --git a/internal/factions/master_faction_list.go b/internal/factions/master_faction_list.go index ba5daa8..69def65 100644 --- a/internal/factions/master_faction_list.go +++ b/internal/factions/master_faction_list.go @@ -317,30 +317,30 @@ func (mfl *MasterFactionList) ValidateFactions() []string { mfl.mutex.RLock() defer mfl.mutex.RUnlock() - // Pre-allocate slice with reasonable capacity to avoid repeated allocations issues := make([]string, 0, 10) - // Single pass through globalFactionList - check faction validity and build seen set - seenInGlobal := make(map[int32]bool, len(mfl.globalFactionList)) - for id, faction := range mfl.globalFactionList { - seenInGlobal[id] = true + seenIDs := make(map[int32]*Faction, len(mfl.globalFactionList)) + seenNames := make(map[string]*Faction, len(mfl.factionNameList)) + // Pass 1: Validate globalFactionList and build seenID map + for id, faction := range mfl.globalFactionList { if faction == nil { issues = append(issues, fmt.Sprintf("Faction ID %d is nil", id)) continue } - // Combine multiple faction checks in one pass (inlined IsValid()) - if faction.ID <= 0 || len(faction.Name) == 0 { - issues = append(issues, fmt.Sprintf("Faction ID %d is invalid", id)) + if faction.ID <= 0 || faction.Name == "" { + issues = append(issues, fmt.Sprintf("Faction ID %d is invalid or unnamed", id)) } if faction.ID != id { issues = append(issues, fmt.Sprintf("Faction ID mismatch: map key %d != faction ID %d", id, faction.ID)) } + + seenIDs[id] = faction } - // Single pass through factionNameList + // Pass 2: Validate factionNameList and build seenName map for name, faction := range mfl.factionNameList { if faction == nil { issues = append(issues, fmt.Sprintf("Faction name '%s' maps to nil", name)) @@ -351,36 +351,29 @@ func (mfl *MasterFactionList) ValidateFactions() []string { issues = append(issues, fmt.Sprintf("Faction name mismatch: map key '%s' != faction name '%s'", name, faction.Name)) } - // Use the pre-built set instead of map lookup - if !seenInGlobal[faction.ID] { + if _, ok := seenIDs[faction.ID]; !ok { issues = append(issues, fmt.Sprintf("Faction '%s' (ID %d) exists in name map but not in ID map", name, faction.ID)) } + + seenNames[name] = faction } - // Check relationship consistency - use the pre-built set for faster lookups - for factionID, hostiles := range mfl.hostileFactions { - if !seenInGlobal[factionID] { - issues = append(issues, fmt.Sprintf("Hostile relationship defined for non-existent faction %d", factionID)) - } - - for _, hostileID := range hostiles { - if !seenInGlobal[hostileID] { - issues = append(issues, fmt.Sprintf("Faction %d has hostile relationship with non-existent faction %d", factionID, hostileID)) + // Pass 3: Validate relationships using prebuilt seenIDs + validateRelations := func(relations map[int32][]int32, relType string) { + for sourceID, targets := range relations { + if _, ok := seenIDs[sourceID]; !ok { + issues = append(issues, fmt.Sprintf("%s relationship defined for non-existent faction %d", relType, sourceID)) + } + for _, targetID := range targets { + if _, ok := seenIDs[targetID]; !ok { + issues = append(issues, fmt.Sprintf("Faction %d has %s relationship with non-existent faction %d", sourceID, relType, targetID)) + } } } } - for factionID, friendlies := range mfl.friendlyFactions { - if !seenInGlobal[factionID] { - issues = append(issues, fmt.Sprintf("Friendly relationship defined for non-existent faction %d", factionID)) - } - - for _, friendlyID := range friendlies { - if !seenInGlobal[friendlyID] { - issues = append(issues, fmt.Sprintf("Faction %d has friendly relationship with non-existent faction %d", factionID, friendlyID)) - } - } - } + validateRelations(mfl.hostileFactions, "Hostile") + validateRelations(mfl.friendlyFactions, "Friendly") return issues }