From 53e44a29aaae06ab31355319924f444e8dd1ef54 Mon Sep 17 00:00:00 2001 From: Josh Jennings Date: Wed, 24 Apr 2024 13:17:32 +0100 Subject: [PATCH 1/3] Add test for CreateFileSet:bug: Make sure that loggers added in NewMultipleLoggers don't fail with 'No Logger Source' --- changes/20240424131658.bugfix | 1 + utils/logs/multiple_logger.go | 22 +++-- utils/logs/multiple_logger_test.go | 136 ++++++++++++++++++----------- 3 files changed, 97 insertions(+), 62 deletions(-) create mode 100644 changes/20240424131658.bugfix diff --git a/changes/20240424131658.bugfix b/changes/20240424131658.bugfix new file mode 100644 index 0000000000..79f0a650fe --- /dev/null +++ b/changes/20240424131658.bugfix @@ -0,0 +1 @@ +:bug: Make sure that loggers added in NewMultipleLoggers don't fail with 'No Logger Source' diff --git a/utils/logs/multiple_logger.go b/utils/logs/multiple_logger.go index 1c9785a10e..e63ab54f66 100644 --- a/utils/logs/multiple_logger.go +++ b/utils/logs/multiple_logger.go @@ -1,6 +1,7 @@ package logs import ( + "fmt" "sync" "github.com/go-logr/logr" @@ -55,11 +56,13 @@ func (c *MultipleLogger) setLoggerSource(source string) error { var err error for i := range c.loggers { err = c.loggers[i].SetLoggerSource(source) + if err != nil { + return err + } } - if err == nil { - c.loggerSource = source - } - return err + + c.loggerSource = source + return nil } func (c *MultipleLogger) Log(output ...interface{}) { @@ -123,6 +126,12 @@ func NewMultipleLoggers(loggerSource string, loggersList ...Loggers) (l IMultipl err = commonerrors.ErrNoLoggerSource return } + l = &MultipleLoggerWithLoggerSource{} + err = l.SetLoggerSource(loggerSource) + if err != nil { + return + } + list := loggersList if len(list) == 0 { std, err := NewStdLogger(loggerSource) @@ -131,12 +140,7 @@ func NewMultipleLoggers(loggerSource string, loggersList ...Loggers) (l IMultipl } list = []Loggers{std} } - l = &MultipleLoggerWithLoggerSource{} err = l.Append(list...) - if err != nil { - return - } - err = l.SetLoggerSource(loggerSource) return } diff --git a/utils/logs/multiple_logger_test.go b/utils/logs/multiple_logger_test.go index 83244c69c0..04c0056049 100644 --- a/utils/logs/multiple_logger_test.go +++ b/utils/logs/multiple_logger_test.go @@ -37,74 +37,104 @@ func TestCombinedLogger(t *testing.T) { } func TestMultipleLoggers(t *testing.T) { - // With default logger - loggers, err := NewMultipleLoggers("Test Multiple") - require.NoError(t, err) - testLog(t, loggers) + t.Run("Manually add loggers", func(t *testing.T) { + // With default logger + loggers, err := NewMultipleLoggers("Test Multiple") + require.NoError(t, err) + testLog(t, loggers) - // Adding a file logger to the mix. - file, err := filesystem.TempFileInTempDir("test-multiplelog-filelog-*.log") - require.NoError(t, err) + // Adding a file logger to the mix. + file, err := filesystem.TempFileInTempDir("test-multiplelog-filelog-*.log") + require.NoError(t, err) - err = file.Close() - require.NoError(t, err) + err = file.Close() + require.NoError(t, err) - defer func() { _ = filesystem.Rm(file.Name()) }() + defer func() { _ = filesystem.Rm(file.Name()) }() - empty, err := filesystem.IsEmpty(file.Name()) - require.NoError(t, err) - assert.True(t, empty) + empty, err := filesystem.IsEmpty(file.Name()) + require.NoError(t, err) + assert.True(t, empty) - fl, err := NewFileLogger(file.Name(), "Test") - require.NoError(t, err) + fl, err := NewFileLogger(file.Name(), "Test") + require.NoError(t, err) - require.NoError(t, loggers.Append(fl)) + require.NoError(t, loggers.Append(fl)) - nl, err := NewNoopLogger("Test2") - require.NoError(t, err) + nl, err := NewNoopLogger("Test2") + require.NoError(t, err) - // Adding various loggers - require.NoError(t, loggers.Append(fl, nl)) + // Adding various loggers + require.NoError(t, loggers.Append(fl, nl)) - testLog(t, loggers) + testLog(t, loggers) - empty, err = filesystem.IsEmpty(file.Name()) - require.NoError(t, err) - assert.False(t, empty) + empty, err = filesystem.IsEmpty(file.Name()) + require.NoError(t, err) + assert.False(t, empty) - err = filesystem.Rm(file.Name()) - require.NoError(t, err) + err = filesystem.Rm(file.Name()) + require.NoError(t, err) - // Concurrency test multiple loggers + // Concurrency test multiple loggers - stdLogger, err := NewStdLogger("Test std logger") - require.NoError(t, err) + stdLogger, err := NewStdLogger("Test std logger") + require.NoError(t, err) - stringLogger, err := NewPlainStringLogger() - if err != nil { - return - } - mLoggers, err := NewCombinedLoggers(stdLogger, stringLogger) - if err != nil { - return - } - - wg := sync.WaitGroup{} - wg.Add(2) - go func() { - defer wg.Done() - for i := 0; i < 100; i++ { - mLoggers.Log(fmt.Sprintf("Test output %v", i)) + stringLogger, err := NewPlainStringLogger() + if err != nil { + return } - }() - go func() { - defer wg.Done() - for i := 0; i < 100; i++ { - mLoggers.LogError(fmt.Sprintf("Test output %v", i)) + mLoggers, err := NewCombinedLoggers(stdLogger, stringLogger) + if err != nil { + return } - }() - wg.Wait() - err = loggers.Close() - require.NoError(t, err) + wg := sync.WaitGroup{} + wg.Add(2) + go func() { + defer wg.Done() + for i := 0; i < 100; i++ { + mLoggers.Log(fmt.Sprintf("Test output %v", i)) + } + }() + go func() { + defer wg.Done() + for i := 0; i < 100; i++ { + mLoggers.LogError(fmt.Sprintf("Test output %v", i)) + } + }() + + wg.Wait() + err = loggers.Close() + require.NoError(t, err) + }) + + t.Run("Add loggers at start", func(t *testing.T) { + + // Adding a file logger to the mix. + file, err := filesystem.TempFileInTempDir("test-multiplelog-filelog-*.log") + require.NoError(t, err) + + err = file.Close() + require.NoError(t, err) + + defer func() { _ = filesystem.Rm(file.Name()) }() + + empty, err := filesystem.IsEmpty(file.Name()) + require.NoError(t, err) + assert.True(t, empty) + + fl, err := NewFileLogger(file.Name(), "Test") + require.NoError(t, err) + + nl, err := NewNoopLogger("Test2") + require.NoError(t, err) + + // With default logger + loggers, err := NewMultipleLoggers("Test Multiple", fl, nl) + require.NoError(t, err) + testLog(t, loggers) + + }) } From b675a889a84ac77e25509759a332e48a22b173bb Mon Sep 17 00:00:00 2001 From: Josh Jennings Date: Wed, 24 Apr 2024 13:17:47 +0100 Subject: [PATCH 2/3] lint --- utils/logs/multiple_logger.go | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/logs/multiple_logger.go b/utils/logs/multiple_logger.go index e63ab54f66..0f131de0a0 100644 --- a/utils/logs/multiple_logger.go +++ b/utils/logs/multiple_logger.go @@ -1,7 +1,6 @@ package logs import ( - "fmt" "sync" "github.com/go-logr/logr" From b0c59d5fc882bba5d9f34f6eba36ea7992651615 Mon Sep 17 00:00:00 2001 From: Josh Jennings Date: Wed, 24 Apr 2024 13:21:05 +0100 Subject: [PATCH 3/3] comments --- utils/logs/multiple_logger_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/utils/logs/multiple_logger_test.go b/utils/logs/multiple_logger_test.go index 04c0056049..8efa71da9a 100644 --- a/utils/logs/multiple_logger_test.go +++ b/utils/logs/multiple_logger_test.go @@ -111,8 +111,6 @@ func TestMultipleLoggers(t *testing.T) { }) t.Run("Add loggers at start", func(t *testing.T) { - - // Adding a file logger to the mix. file, err := filesystem.TempFileInTempDir("test-multiplelog-filelog-*.log") require.NoError(t, err) @@ -131,10 +129,8 @@ func TestMultipleLoggers(t *testing.T) { nl, err := NewNoopLogger("Test2") require.NoError(t, err) - // With default logger loggers, err := NewMultipleLoggers("Test Multiple", fl, nl) require.NoError(t, err) testLog(t, loggers) - }) }