Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
github-actions wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −20
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 94 additions & 20 deletions internal/logger/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"fmt"
"os"
"path/filepath"
"strings"
"sync"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -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")
}

Expand All @@ -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")
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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")
Expand All @@ -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) {
Expand All @@ -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")
Expand All @@ -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) {
Expand All @@ -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")
Expand All @@ -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) {
Expand All @@ -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")
Expand All @@ -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) {
Expand All @@ -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")
Expand All @@ -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) {
Expand All @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
})
}
Loading