[test-improver] Improve tests for logger common package

[github-actions]

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 · ◷

[Copilot]

Pull request overview

Improves unit tests for the internal/logger common helpers by making assertions more idiomatic and adding direct coverage for the shared log-line formatting helper.

Changes:

• Replaced t.Errorf/t.Fatal patterns inside callbacks with boolean flags plus assert/require assertions for clearer intent.
• Improved error-path tests for initLogFile to assert errors after safely closing any returned file handle.
• Added direct subtests for formatLogLine output structure, message formatting, and RFC3339 UTC timestamp parsing.

Show a summary per file

File	Description
internal/logger/common_test.go	Refactors existing tests to use consistent testify assertions and adds direct unit tests for formatLogLine.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

internal/logger/common_test.go:515

• This test assumes "/root/nonexistent/directory" is unwritable to trigger the error path. If tests run with elevated privileges, initLogFile may succeed and this assertion about error-handler behavior becomes flaky. Prefer constructing a deterministic failure (e.g., temp file used as a directory component, or temp dir with permissions removed) so the error path is guaranteed.

	// Use a non-writable directory to trigger error
	logDir := "/root/nonexistent/directory"
	fileName := "test.jsonl"

	errorHandlerCalled := false
	setupHandlerCalled := false

internal/logger/common_test.go:591

• Using "/root/nonexistent/directory" to force initLogFile failure can be flaky when tests run as root (or in containers with different permission models). To keep this fallback test stable across environments, consider forcing failure via a temp dir with permissions removed, or by attempting to create a subdirectory under a regular file (as done in TestInitLogFile_InvalidDirectory).

	// Use a non-writable directory to trigger error
	logDir := "/root/nonexistent/directory"
	fileName := "test.md"

	errorHandlerCalled := false
	setupHandlerCalled := false

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Copilot]

These fallback tests rely on "/root/nonexistent/directory" being non-writable. In CI (or local) runs as root, initLogFile may succeed and the test will fail (unlike TestInitLogFile_UnwritableDirectory, which skips when writable). Consider making the directory reliably unwritable using a temp dir with chmod 000 / read-only permissions, or using the existing "file-as-directory component" approach from TestInitLogFile_InvalidDirectory to force initLogFile to fail deterministically.

This issue also appears in the following locations of the same file:

• line 510
• line 586

See below for a potential fix:

	// Use a path with a file as a directory component to trigger error deterministically.
	tmpDir := t.TempDir()
	fileAsDir := filepath.Join(tmpDir, "not-a-directory")
	err := os.WriteFile(fileAsDir, []byte("test"), 0644)
	require.NoError(err)

	logDir := filepath.Join(fileAsDir, "nonexistent")