[test-improver] Improve tests for logger common package (#3114)

# Test Improvements: `internal/logger/common_test.go`

## File Analyzed

- **Test File**: `internal/logger/common_test.go`
- **Package**: `internal/logger`
- **Lines of Code**: ~696 → ~770 (74 lines added)

## Improvements Made

### 1. Better Testify Patterns — Replace `t.Errorf`/`t.Fatal` with
Idiomatic Assertions

**Problem**: Several tests used `t.Errorf("should not be called")`
inside callback closures and `t.Fatal` inside `if err == nil` guards
instead of proper testify patterns.

- ✅ **`TestInitLogFile_InvalidDirectory`** — replaced `if err == nil {
file.Close(); t.Fatal(...) }` with `require.Error(err, ...)` after
closing any file handle
- ✅ **`TestInitLogFile_EmptyFileName`** — same pattern; added `require
:= require.New(t)` so the test uses testify `require.Error` consistently
- ✅ **`TestInitLogger_FileLogger`** — replaced `t.Errorf("Error handler
should not be called")` inside callback with a `errorHandlerCalled`
boolean flag + `assert.False(errorHandlerCalled, ...)`
- ✅ **`TestInitLogger_FileLoggerFallback`** — replaced `t.Errorf` in
setup callback with `setupHandlerCalled` flag + `assert.False`
- ✅ **`TestInitLogger_JSONLLogger`** — same improvement as FileLogger
- ✅ **`TestInitLogger_JSONLLoggerError`** — same improvement as
FileLoggerFallback
- ✅ **`TestInitLogger_MarkdownLogger`** — same improvement as FileLogger
- ✅ **`TestInitLogger_MarkdownLoggerFallback`** — same improvement as
FileLoggerFallback
- ✅ **`TestInitLogger_SetupError`** — replaced `t.Errorf` in
error-handler callback with flag + `assert.False`

The boolean flag pattern makes the intent explicit alongside the other
post-call assertions and avoids mixing raw `t.Errorf` with testify
assertions in the same test.

### 2. Increased Coverage — Direct Tests for `formatLogLine`

**Problem**: The `formatLogLine` function in `common.go` was exercised
only indirectly through `FileLogger.Log()` and `ServerFileLogger`, but
had **no direct unit tests**. The function is responsible for the
canonical log line format used across all file-based loggers.

- ✅ Added `TestFormatLogLine` with subtests covering:
- All four log levels (`INFO`, `WARN`, `ERROR`, `DEBUG`) appear in
brackets
  - Category appears in its own bracket
  - Format-string interpolation with args (`count=%d name=%s`)
- Full bracket structure regex:
`^\[\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z\] \[INFO\] \[auth\] event
occurred$`
  - Timestamp is RFC3339 UTC and falls within the test's time window
  - Format string used as-is when no args provided
  - Empty category produces `[]` bracket pair

### 3. Cleaner & More Stable Tests

- ✅ Added `"strings"` and `"time"` imports for the new
`TestFormatLogLine` test
- ✅ Consistent testify usage — no more mixing of `t.Errorf` with
`assert`/`require` in the same tests
- ✅ Better resource cleanup — files are closed before `require.Error`
stops the test

## Why These Changes?

`internal/logger/common_test.go` tests the core initialization
primitives (`closeLogFile`, `initLogFile`, `initLogger`) used by all
logger types in the package. The `formatLogLine` function — which
defines the log line format for all operational logs — was entirely
untested directly. The `t.Errorf` callback pattern, while valid Go, was
inconsistent with the testify style used throughout the rest of the
file.

---
*Generated by Test Improver Workflow*
*Focuses on better patterns, increased coverage, and more stable tests*




> Generated by [Test
Improver](https://github.com/github/gh-aw-mcpg/actions/runs/23944702778/agentic_workflow)
·
[◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&type=pullrequests)

<!-- gh-aw-agentic-workflow: Test Improver, engine: copilot, model:
auto, id: 23944702778, workflow_id: test-improver, run:
https://github.com/github/gh-aw-mcpg/actions/runs/23944702778 -->

<!-- gh-aw-workflow-id: test-improver -->

Author: lpcox

internal/logger/common_test.go

@@ -4,8 +4,10 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -289,11 +291,10 @@ func TestInitLogFile_InvalidDirectory(t *testing.T) {
 	fileName := "test.log"
 
 	file, err := initLogFile(logDir, fileName, os.O_APPEND)
-	if err == nil {
+	if file != nil {
 		file.Close()
-		t.Fatal("initLogFile should fail when directory can't be created")
 	}
-
+	require.Error(err, "initLogFile should fail when directory can't be created")
 	assert.Contains(err.Error(), "failed to create log directory", "Error should mention directory creation failure")
 }
 
@@ -319,16 +320,16 @@ func TestInitLogFile_UnwritableDirectory(t *testing.T) {
 
 func TestInitLogFile_EmptyFileName(t *testing.T) {
 	assert := assert.New(t)
+	require := require.New(t)
 	tmpDir := t.TempDir()
 	logDir := filepath.Join(tmpDir, "logs")
 	fileName := ""
 
 	file, err := initLogFile(logDir, fileName, os.O_APPEND)
-	if err == nil {
+	if file != nil {
 		file.Close()
-		t.Fatal("initLogFile should fail with empty fileName")
 	}
-
+	require.Error(err, "initLogFile should fail with empty fileName")
 	assert.Contains(err.Error(), "failed to open log file", "Error should mention file opening failure")
 }
 
@@ -388,6 +389,7 @@ func TestInitLogger_FileLogger(t *testing.T) {
 	fileName := "test.log"
 
 	// Test successful initialization
+	errorHandlerCalled := false
 	logger, err := initLogger(
 		logDir, fileName, os.O_APPEND,
 		func(file *os.File, logDir, fileName string) (*FileLogger, error) {
@@ -399,12 +401,13 @@ func TestInitLogger_FileLogger(t *testing.T) {
 			return fl, nil
 		},
 		func(err error, logDir, fileName string) (*FileLogger, error) {
-			// Should not be called on success
-			t.Errorf("Error handler should not be called on successful initialization")
+			errorHandlerCalled = true
 			return nil, err
 		},
 	)
 
+	assert.False(errorHandlerCalled, "Error handler should not be called on successful initialization")
+
 	require.NoError(err, "initLogger should not return error")
 	require.NotNil(logger, "logger should not be nil")
 	assert.Equal(logDir, logger.logDir, "logDir should match")
@@ -429,12 +432,12 @@ func TestInitLogger_FileLoggerFallback(t *testing.T) {
 	fileName := "test.log"
 
 	errorHandlerCalled := false
+	setupHandlerCalled := false
 
 	logger, err := initLogger(
 		logDir, fileName, os.O_APPEND,
 		func(file *os.File, logDir, fileName string) (*FileLogger, error) {
-			// Should not be called on error
-			t.Errorf("Setup handler should not be called on error")
+			setupHandlerCalled = true
 			return nil, nil
 		},
 		func(err error, logDir, fileName string) (*FileLogger, error) {
@@ -450,6 +453,8 @@ func TestInitLogger_FileLoggerFallback(t *testing.T) {
 		},
 	)
 
+	assert.False(setupHandlerCalled, "Setup handler should not be called on error")
+
 	assert.True(errorHandlerCalled, "Error handler should be called")
 	require.NoError(err, "initLogger should not return error for fallback")
 	require.NotNil(logger, "logger should not be nil")
@@ -465,6 +470,7 @@ func TestInitLogger_JSONLLogger(t *testing.T) {
 	logDir := filepath.Join(tmpDir, "logs")
 	fileName := "test.jsonl"
 
+	errorHandlerCalled := false
 	logger, err := initLogger(
 		logDir, fileName, os.O_APPEND,
 		func(file *os.File, logDir, fileName string) (*JSONLLogger, error) {
@@ -476,12 +482,13 @@ func TestInitLogger_JSONLLogger(t *testing.T) {
 			return jl, nil
 		},
 		func(err error, logDir, fileName string) (*JSONLLogger, error) {
-			// Should not be called on success
-			t.Errorf("Error handler should not be called on successful initialization")
+			errorHandlerCalled = true
 			return nil, err
 		},
 	)
 
+	assert.False(errorHandlerCalled, "Error handler should not be called on successful initialization")
+
 	require.NoError(err, "initLogger should not return error")
 	require.NotNil(logger, "logger should not be nil")
 	assert.Equal(logDir, logger.logDir, "logDir should match")
@@ -505,12 +512,12 @@ func TestInitLogger_JSONLLoggerError(t *testing.T) {
 	fileName := "test.jsonl"
 
 	errorHandlerCalled := false
+	setupHandlerCalled := false
 
 	logger, err := initLogger(
 		logDir, fileName, os.O_APPEND,
 		func(file *os.File, logDir, fileName string) (*JSONLLogger, error) {
-			// Should not be called on error
-			t.Errorf("Setup handler should not be called on error")
+			setupHandlerCalled = true
 			return nil, nil
 		},
 		func(err error, logDir, fileName string) (*JSONLLogger, error) {
@@ -521,6 +528,8 @@ func TestInitLogger_JSONLLoggerError(t *testing.T) {
 		},
 	)
 
+	assert.False(setupHandlerCalled, "Setup handler should not be called on error")
+
 	assert.True(errorHandlerCalled, "Error handler should be called")
 	assert.Error(err, "initLogger should return error")
 	assert.Nil(logger, "logger should be nil on error")
@@ -534,6 +543,7 @@ func TestInitLogger_MarkdownLogger(t *testing.T) {
 	logDir := filepath.Join(tmpDir, "logs")
 	fileName := "test.md"
 
+	errorHandlerCalled := false
 	logger, err := initLogger(
 		logDir, fileName, os.O_TRUNC,
 		func(file *os.File, logDir, fileName string) (*MarkdownLogger, error) {
@@ -546,12 +556,13 @@ func TestInitLogger_MarkdownLogger(t *testing.T) {
 			return ml, nil
 		},
 		func(err error, logDir, fileName string) (*MarkdownLogger, error) {
-			// Should not be called on success
-			t.Errorf("Error handler should not be called on successful initialization")
+			errorHandlerCalled = true
 			return nil, err
 		},
 	)
 
+	assert.False(errorHandlerCalled, "Error handler should not be called on successful initialization")
+
 	require.NoError(err, "initLogger should not return error")
 	require.NotNil(logger, "logger should not be nil")
 	assert.Equal(logDir, logger.logDir, "logDir should match")
@@ -577,12 +588,12 @@ func TestInitLogger_MarkdownLoggerFallback(t *testing.T) {
 	fileName := "test.md"
 
 	errorHandlerCalled := false
+	setupHandlerCalled := false
 
 	logger, err := initLogger(
 		logDir, fileName, os.O_TRUNC,
 		func(file *os.File, logDir, fileName string) (*MarkdownLogger, error) {
-			// Should not be called on error
-			t.Errorf("Setup handler should not be called on error")
+			setupHandlerCalled = true
 			return nil, nil
 		},
 		func(err error, logDir, fileName string) (*MarkdownLogger, error) {
@@ -598,6 +609,8 @@ func TestInitLogger_MarkdownLoggerFallback(t *testing.T) {
 		},
 	)
 
+	assert.False(setupHandlerCalled, "Setup handler should not be called on error")
+
 	assert.True(errorHandlerCalled, "Error handler should be called")
 	require.NoError(err, "initLogger should not return error for fallback")
 	require.NotNil(logger, "logger should not be nil")
@@ -611,19 +624,21 @@ func TestInitLogger_SetupError(t *testing.T) {
 	logDir := filepath.Join(tmpDir, "logs")
 	fileName := "test.log"
 
+	errorHandlerCalled := false
 	logger, err := initLogger(
 		logDir, fileName, os.O_APPEND,
 		func(file *os.File, logDir, fileName string) (*FileLogger, error) {
 			// Simulate setup error
 			return nil, assert.AnError
 		},
 		func(err error, logDir, fileName string) (*FileLogger, error) {
-			// Should not be called for setup errors
-			t.Errorf("Error handler should not be called for setup errors")
+			errorHandlerCalled = true
 			return nil, err
 		},
 	)
 
+	a.False(errorHandlerCalled, "Error handler should not be called for setup errors")
+
 	a.Error(err, "initLogger should return error on setup failure")
 	a.Equal(assert.AnError, err, "Error should match setup error")
 	a.Nil(logger, "logger should be nil on setup error")
@@ -694,3 +709,62 @@ func TestInitLogger_FileFlags(t *testing.T) {
 	assert.NotContains(string(content), "appended content", "File should not contain appended content")
 	assert.Contains(string(content), "new content", "File should contain new content")
 }
+
+// TestFormatLogLine tests the formatLogLine helper that builds standard log lines.
+func TestFormatLogLine(t *testing.T) {
+	t.Run("output contains level bracket", func(t *testing.T) {
+		levels := []LogLevel{LogLevelInfo, LogLevelWarn, LogLevelError, LogLevelDebug}
+		for _, level := range levels {
+			t.Run(string(level), func(t *testing.T) {
+				result := formatLogLine(level, "test", "msg")
+				assert.Contains(t, result, "["+string(level)+"]",
+					"Log line should contain level in brackets")
+			})
+		}
+	})
+
+	t.Run("output contains category in brackets", func(t *testing.T) {
+		result := formatLogLine(LogLevelInfo, "startup", "message")
+		assert.Contains(t, result, "[startup]", "Log line should contain the category")
+	})
+
+	t.Run("output contains formatted message", func(t *testing.T) {
+		result := formatLogLine(LogLevelInfo, "test", "count=%d name=%s", 42, "alice")
+		assert.Contains(t, result, "count=42 name=alice", "Log line should contain formatted message")
+	})
+
+	t.Run("output follows bracket structure", func(t *testing.T) {
+		result := formatLogLine(LogLevelInfo, "auth", "event occurred")
+		// Expected format: [timestamp] [INFO] [auth] event occurred
+		assert.Regexp(t, `^\[\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z\] \[INFO\] \[auth\] event occurred$`, result)
+	})
+
+	t.Run("timestamp is RFC3339 UTC within test window", func(t *testing.T) {
+		before := time.Now().UTC().Truncate(time.Second)
+		result := formatLogLine(LogLevelDebug, "test", "msg")
+		after := time.Now().UTC().Add(time.Second)
+
+		// Extract the timestamp between the first pair of brackets
+		assert.Regexp(t, `^\[\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z\]`, result,
+			"Should start with RFC3339 UTC timestamp in brackets")
+
+		parts := strings.SplitN(result, "]", 2)
+		require.Len(t, parts, 2, "Output should contain at least one closing bracket")
+		tsStr := strings.TrimPrefix(parts[0], "[")
+
+		ts, err := time.Parse(time.RFC3339, tsStr)
+		require.NoError(t, err, "Extracted timestamp should parse as RFC3339")
+		assert.False(t, ts.Before(before), "Timestamp should not be before test start")
+		assert.False(t, ts.After(after), "Timestamp should not be after test end")
+	})
+
+	t.Run("format string used as-is when no args provided", func(t *testing.T) {
+		result := formatLogLine(LogLevelWarn, "net", "plain message")
+		assert.Contains(t, result, "plain message", "Should include the literal format string")
+	})
+
+	t.Run("empty category produces empty bracket", func(t *testing.T) {
+		result := formatLogLine(LogLevelInfo, "", "message")
+		assert.Contains(t, result, "[]", "Empty category should produce empty bracket pair")
+	})
+}