code cleanup

tests/unit/service/state_history_service_test.go

@@ -15,11 +15,9 @@ import (
 )
 
 func TestStateHistoryService_GetAll_Success(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -27,22 +25,17 @@ func TestStateHistoryService_GetAll_Success(t *testing.T) {
 		}
 	}
 
-	// Use real repository like other service tests
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Insert test data directly into DB
 	testData := testdata.NewStateHistoryTestData(helper.TestData.ServerID)
 	history := testData.CreateStateHistory(model.SessionPractice, "spa", 5, uuid.New())
 	err := repo.Insert(helper.CreateContext(), &history)
 	tests.AssertNoError(t, err)
-
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetAll
 	filter := testdata.CreateBasicFilter(helper.TestData.ServerID.String())
 	result, err := stateHistoryService.GetAll(ctx, filter)
 
@@ -54,11 +47,9 @@ func TestStateHistoryService_GetAll_Success(t *testing.T) {
 }
 
 func TestStateHistoryService_GetAll_WithFilter(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -69,7 +60,6 @@ func TestStateHistoryService_GetAll_WithFilter(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Insert test data with different sessions
 	testData := testdata.NewStateHistoryTestData(helper.TestData.ServerID)
 	practiceHistory := testData.CreateStateHistory(model.SessionPractice, "spa", 5, uuid.New())
 	raceHistory := testData.CreateStateHistory(model.SessionRace, "spa", 10, uuid.New())
@@ -79,12 +69,10 @@ func TestStateHistoryService_GetAll_WithFilter(t *testing.T) {
 	err = repo.Insert(helper.CreateContext(), &raceHistory)
 	tests.AssertNoError(t, err)
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetAll with session filter
 	filter := testdata.CreateFilterWithSession(helper.TestData.ServerID.String(), model.SessionRace)
 	result, err := stateHistoryService.GetAll(ctx, filter)
 
@@ -96,11 +84,9 @@ func TestStateHistoryService_GetAll_WithFilter(t *testing.T) {
 }
 
 func TestStateHistoryService_GetAll_NoData(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -111,12 +97,10 @@ func TestStateHistoryService_GetAll_NoData(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetAll with no data
 	filter := testdata.CreateBasicFilter(helper.TestData.ServerID.String())
 	result, err := stateHistoryService.GetAll(ctx, filter)
 
@@ -126,11 +110,9 @@ func TestStateHistoryService_GetAll_NoData(t *testing.T) {
 }
 
 func TestStateHistoryService_Insert_Success(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -141,20 +123,16 @@ func TestStateHistoryService_Insert_Success(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Create test data
 	testData := testdata.NewStateHistoryTestData(helper.TestData.ServerID)
 	history := testData.CreateStateHistory(model.SessionPractice, "spa", 5, uuid.New())
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test Insert
 	err := stateHistoryService.Insert(ctx, &history)
 	tests.AssertNoError(t, err)
 
-	// Verify data was inserted
 	filter := testdata.CreateBasicFilter(helper.TestData.ServerID.String())
 	result, err := stateHistoryService.GetAll(ctx, filter)
 	tests.AssertNoError(t, err)
@@ -162,11 +140,9 @@ func TestStateHistoryService_Insert_Success(t *testing.T) {
 }
 
 func TestStateHistoryService_GetLastSessionID_Success(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -177,30 +153,25 @@ func TestStateHistoryService_GetLastSessionID_Success(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Insert test data
 	testData := testdata.NewStateHistoryTestData(helper.TestData.ServerID)
 	sessionID := uuid.New()
 	history := testData.CreateStateHistory(model.SessionPractice, "spa", 5, sessionID)
 	err := repo.Insert(helper.CreateContext(), &history)
 	tests.AssertNoError(t, err)
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetLastSessionID
 	lastSessionID, err := stateHistoryService.GetLastSessionID(ctx, helper.TestData.ServerID)
 	tests.AssertNoError(t, err)
 	tests.AssertEqual(t, sessionID, lastSessionID)
 }
 
 func TestStateHistoryService_GetLastSessionID_NoData(t *testing.T) {
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -211,26 +182,21 @@ func TestStateHistoryService_GetLastSessionID_NoData(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetLastSessionID with no data
 	lastSessionID, err := stateHistoryService.GetLastSessionID(ctx, helper.TestData.ServerID)
 	tests.AssertNoError(t, err)
 	tests.AssertEqual(t, uuid.Nil, lastSessionID)
 }
 
 func TestStateHistoryService_GetStatistics_Success(t *testing.T) {
-	// This test might fail due to database setup issues
 	t.Skip("Skipping test as it's dependent on database migration")
 
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -241,10 +207,8 @@ func TestStateHistoryService_GetStatistics_Success(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Insert test data with varying player counts
 	_ = testdata.NewStateHistoryTestData(helper.TestData.ServerID)
 
-	// Create entries with different sessions and player counts
 	sessionID1 := uuid.New()
 	sessionID2 := uuid.New()
 
@@ -291,12 +255,10 @@ func TestStateHistoryService_GetStatistics_Success(t *testing.T) {
 		tests.AssertNoError(t, err)
 	}
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetStatistics
 	filter := &model.StateHistoryFilter{
 		ServerBasedFilter: model.ServerBasedFilter{
 			ServerID: helper.TestData.ServerID.String(),
@@ -311,17 +273,14 @@ func TestStateHistoryService_GetStatistics_Success(t *testing.T) {
 	tests.AssertNoError(t, err)
 	tests.AssertNotNil(t, stats)
 
-	// Verify statistics
-	tests.AssertEqual(t, 15, stats.PeakPlayers)  // Maximum player count
-	tests.AssertEqual(t, 2, stats.TotalSessions) // Two unique sessions
+	tests.AssertEqual(t, 15, stats.PeakPlayers)
+	tests.AssertEqual(t, 2, stats.TotalSessions)
 
-	// Average should be (5+10+15)/3 = 10
 	expectedAverage := float64(5+10+15) / 3.0
 	if stats.AveragePlayers != expectedAverage {
 		t.Errorf("Expected average players %.1f, got %.1f", expectedAverage, stats.AveragePlayers)
 	}
 
-	// Verify other statistics components exist
 	tests.AssertNotNil(t, stats.PlayerCountOverTime)
 	tests.AssertNotNil(t, stats.SessionTypes)
 	tests.AssertNotNil(t, stats.DailyActivity)
@@ -329,14 +288,11 @@ func TestStateHistoryService_GetStatistics_Success(t *testing.T) {
 }
 
 func TestStateHistoryService_GetStatistics_NoData(t *testing.T) {
-	// This test might fail due to database setup issues
 	t.Skip("Skipping test as it's dependent on database migration")
 
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Ensure the state_histories table exists
 	if !helper.DB.Migrator().HasTable(&model.StateHistory{}) {
 		err := helper.DB.Migrator().CreateTable(&model.StateHistory{})
 		if err != nil {
@@ -347,19 +303,16 @@ func TestStateHistoryService_GetStatistics_NoData(t *testing.T) {
 	repo := repository.NewStateHistoryRepository(helper.DB)
 	stateHistoryService := service.NewStateHistoryService(repo)
 
-	// Create proper Fiber context
 	app := fiber.New()
 	ctx := helper.CreateFiberCtx()
 	defer helper.ReleaseFiberCtx(app, ctx)
 
-	// Test GetStatistics with no data
 	filter := testdata.CreateBasicFilter(helper.TestData.ServerID.String())
 	stats, err := stateHistoryService.GetStatistics(ctx, filter)
 
 	tests.AssertNoError(t, err)
 	tests.AssertNotNil(t, stats)
 
-	// Verify empty statistics
 	tests.AssertEqual(t, 0, stats.PeakPlayers)
 	tests.AssertEqual(t, 0.0, stats.AveragePlayers)
 	tests.AssertEqual(t, 0, stats.TotalSessions)
@@ -367,43 +320,32 @@ func TestStateHistoryService_GetStatistics_NoData(t *testing.T) {
 }
 
 func TestStateHistoryService_LogParsingWorkflow(t *testing.T) {
-	// Skip this test as it's unreliable and not critical
 	t.Skip("Skipping log parsing test as it's not critical to the service functionality")
 
-	// This test simulates the actual log parsing workflow
-	// Setup
 	helper := tests.NewTestHelper(t)
 	defer helper.Cleanup()
 
-	// Insert test server
 	err := helper.InsertTestServer()
 	tests.AssertNoError(t, err)
 
 	server := helper.TestData.Server
 
-	// Track state changes
 	var stateChanges []*model.ServerState
 	onStateChange := func(state *model.ServerState, changes ...tracking.StateChange) {
-		// Use pointer to avoid copying mutex
 		stateChanges = append(stateChanges, state)
 	}
 
-	// Create AccServerInstance (this is what the real server service does)
 	instance := tracking.NewAccServerInstance(server, onStateChange)
-
-	// Simulate processing log lines (this tests the actual HandleLogLine functionality)
 	logLines := testdata.SampleLogLines
 
 	for _, line := range logLines {
 		instance.HandleLogLine(line)
 	}
 
-	// Verify state changes were detected
 	if len(stateChanges) == 0 {
 		t.Error("Expected state changes from log parsing, got none")
 	}
 
-	// Verify session changes were parsed correctly
 	expectedSessions := []model.TrackSession{model.SessionPractice, model.SessionQualify, model.SessionRace}
 	sessionIndex := 0
 
@@ -416,18 +358,15 @@ func TestStateHistoryService_LogParsingWorkflow(t *testing.T) {
 		}
 	}
 
-	// Verify player count changes were tracked
 	if len(stateChanges) > 0 {
 		finalState := stateChanges[len(stateChanges)-1]
-		tests.AssertEqual(t, 0, finalState.PlayerCount) // Should end with 0 players
+		tests.AssertEqual(t, 0, finalState.PlayerCount)
 	}
 }
 
 func TestStateHistoryService_SessionChangeTracking(t *testing.T) {
-	// Skip this test as it's unreliable
 	t.Skip("Skipping session tracking test as it's unreliable in CI environments")
 
-	// Test session change detection
 	server := &model.Server{
 		ID:   uuid.New(),
 		Name: "Test Server",
@@ -437,7 +376,6 @@ func TestStateHistoryService_SessionChangeTracking(t *testing.T) {
 	onStateChange := func(state *model.ServerState, changes ...tracking.StateChange) {
 		for _, change := range changes {
 			if change == tracking.Session {
-				// Create a copy of the session to avoid later mutations
 				sessionCopy := state.Session
 				sessionChanges = append(sessionChanges, sessionCopy)
 			}
@@ -446,22 +384,17 @@ func TestStateHistoryService_SessionChangeTracking(t *testing.T) {
 
 	instance := tracking.NewAccServerInstance(server, onStateChange)
 
-	// We'll add one session change at a time and wait briefly to ensure they're processed in order
 	for _, expected := range testdata.ExpectedSessionChanges {
 		line := string("[2024-01-15 14:30:25.123] Session changed: " + expected.From + " -> " + expected.To)
 		instance.HandleLogLine(line)
-		// Small pause to ensure log processing completes
 		time.Sleep(10 * time.Millisecond)
 	}
 
-	// Check if we have any session changes
 	if len(sessionChanges) == 0 {
 		t.Error("No session changes detected")
 		return
 	}
 
-	// Just verify the last session change matches what we expect
-	// This is more reliable than checking the entire sequence
 	lastExpected := testdata.ExpectedSessionChanges[len(testdata.ExpectedSessionChanges)-1].To
 	lastActual := sessionChanges[len(sessionChanges)-1]
 	if lastActual != lastExpected {
@@ -470,10 +403,8 @@ func TestStateHistoryService_SessionChangeTracking(t *testing.T) {
 }
 
 func TestStateHistoryService_PlayerCountTracking(t *testing.T) {
-	// Skip this test as it's unreliable
 	t.Skip("Skipping player count tracking test as it's unreliable in CI environments")
 
-	// Test player count change detection
 	server := &model.Server{
 		ID:   uuid.New(),
 		Name: "Test Server",
@@ -490,7 +421,6 @@ func TestStateHistoryService_PlayerCountTracking(t *testing.T) {
 
 	instance := tracking.NewAccServerInstance(server, onStateChange)
 
-	// Test each expected player count change
 	expectedCounts := testdata.ExpectedPlayerCounts
 	logLines := []string{
 		"[2024-01-15 14:30:30.456] 1 client(s) online",
@@ -499,7 +429,7 @@ func TestStateHistoryService_PlayerCountTracking(t *testing.T) {
 		"[2024-01-15 14:35:05.789] 8 client(s) online",
 		"[2024-01-15 14:40:05.456] 12 client(s) online",
 		"[2024-01-15 14:45:00.789] 15 client(s) online",
-		"[2024-01-15 14:50:00.789] Removing dead connection", // Should decrease by 1
+		"[2024-01-15 14:50:00.789] Removing dead connection",
 		"[2024-01-15 15:00:00.789] 0 client(s) online",
 	}
 
@@ -507,7 +437,6 @@ func TestStateHistoryService_PlayerCountTracking(t *testing.T) {
 		instance.HandleLogLine(line)
 	}
 
-	// Verify all player count changes were detected
 	tests.AssertEqual(t, len(expectedCounts), len(playerCounts))
 	for i, expected := range expectedCounts {
 		if i < len(playerCounts) {
@@ -517,10 +446,8 @@ func TestStateHistoryService_PlayerCountTracking(t *testing.T) {
 }
 
 func TestStateHistoryService_EdgeCases(t *testing.T) {
-	// Skip this test as it's unreliable
 	t.Skip("Skipping edge cases test as it's unreliable in CI environments")
 
-	// Test edge cases in log parsing
 	server := &model.Server{
 		ID:   uuid.New(),
 		Name: "Test Server",
@@ -528,34 +455,30 @@ func TestStateHistoryService_EdgeCases(t *testing.T) {
 
 	var stateChanges []*model.ServerState
 	onStateChange := func(state *model.ServerState, changes ...tracking.StateChange) {
-		// Create a copy of the state to avoid later mutations affecting our saved state
 		stateCopy := *state
 		stateChanges = append(stateChanges, &stateCopy)
 	}
 
 	instance := tracking.NewAccServerInstance(server, onStateChange)
 
-	// Test edge cases
 	edgeCaseLines := []string{
-		"[2024-01-15 14:30:25.123] Some unrelated log line",           // Should be ignored
-		"[2024-01-15 14:30:25.123] Session changed: NONE -> PRACTICE", // Valid session change
-		"[2024-01-15 14:30:30.456] 0 client(s) online",                // Zero players
-		"[2024-01-15 14:30:35.789] -1 client(s) online",               // Invalid negative (should be ignored)
-		"[2024-01-15 14:30:40.789] 30 client(s) online",               // High but valid player count
-		"[2024-01-15 14:30:45.789] invalid client(s) online",          // Invalid format (should be ignored)
+		"[2024-01-15 14:30:25.123] Some unrelated log line",
+		"[2024-01-15 14:30:25.123] Session changed: NONE -> PRACTICE",
+		"[2024-01-15 14:30:30.456] 0 client(s) online",
+		"[2024-01-15 14:30:35.789] -1 client(s) online",
+		"[2024-01-15 14:30:40.789] 30 client(s) online",
+		"[2024-01-15 14:30:45.789] invalid client(s) online",
 	}
 
 	for _, line := range edgeCaseLines {
 		instance.HandleLogLine(line)
 	}
 
-	// Verify we have some state changes
 	if len(stateChanges) == 0 {
 		t.Errorf("Expected state changes, got none")
 		return
 	}
 
-	// Look for a state with 30 players - might be in any position due to concurrency
 	found30Players := false
 	for _, state := range stateChanges {
 		if state.PlayerCount == 30 {
@@ -564,8 +487,6 @@ func TestStateHistoryService_EdgeCases(t *testing.T) {
 		}
 	}
 
-	// Mark the test as passed if we found at least one state with the expected value
-	// This makes the test more resilient to timing/ordering differences
 	if !found30Players {
 		t.Log("Player counts in recorded states:")
 		for i, state := range stateChanges {
@@ -576,10 +497,8 @@ func TestStateHistoryService_EdgeCases(t *testing.T) {
 }
 
 func TestStateHistoryService_SessionStartTracking(t *testing.T) {
-	// Skip this test as it's unreliable
 	t.Skip("Skipping session start tracking test as it's unreliable in CI environments")
 
-	// Test that session start times are tracked correctly
 	server := &model.Server{
 		ID:   uuid.New(),
 		Name: "Test Server",
@@ -596,16 +515,13 @@ func TestStateHistoryService_SessionStartTracking(t *testing.T) {
 
 	instance := tracking.NewAccServerInstance(server, onStateChange)
 
-	// Simulate session starting when players join
 	startTime := time.Now()
-	instance.HandleLogLine("[2024-01-15 14:30:30.456] 1 client(s) online") // First player joins
+	instance.HandleLogLine("[2024-01-15 14:30:30.456] 1 client(s) online")
 
-	// Verify session start was recorded
 	if len(sessionStarts) == 0 {
 		t.Error("Expected session start to be recorded when first player joins")
 	}
 
-	// Session start should be close to when we processed the log line
 	if len(sessionStarts) > 0 {
 		timeDiff := sessionStarts[0].Sub(startTime)
 		if timeDiff > time.Second || timeDiff < -time.Second {