From 9240222fbefd91e6bf6021fdee3c178c92cb65b0 Mon Sep 17 00:00:00 2001 From: Patryk Osmaczko Date: Tue, 29 Oct 2024 13:30:14 +0100 Subject: [PATCH] fix_: create request logger ad-hoc in tests Fixes `TestCall` failing when run concurrently. --- logutils/requestlog/request_log.go | 24 +++++----- mobile/{ => callog}/status_request_log.go | 17 ++++--- .../{ => callog}/status_request_log_test.go | 47 ++++--------------- mobile/init_logging_test.go | 2 +- mobile/status.go | 10 ++++ mobile/status_test.go | 40 ++++++++++++++++ 6 files changed, 79 insertions(+), 61 deletions(-) rename mobile/{ => callog}/status_request_log.go (87%) rename mobile/{ => callog}/status_request_log_test.go (72%) create mode 100644 mobile/status_test.go diff --git a/logutils/requestlog/request_log.go b/logutils/requestlog/request_log.go index ea7cfcdf2..cf050943e 100644 --- a/logutils/requestlog/request_log.go +++ b/logutils/requestlog/request_log.go @@ -14,23 +14,14 @@ var ( requestLogger *zap.Logger ) -// IsRequestLoggingEnabled returns whether RPC logging is enabled -func IsRequestLoggingEnabled() bool { - return requestLogger != nil -} - // GetRequestLogger returns the RPC logger object func GetRequestLogger() *zap.Logger { return requestLogger } -func ConfigureAndEnableRequestLogging(file string) error { +func CreateRequestLogger(file string) (*zap.Logger, error) { if len(file) == 0 { - return errors.New("file is required") - } - - if IsRequestLoggingEnabled() { - return errors.New("request logging is already enabled") + return nil, errors.New("file is required") } fileOpts := logutils.FileOptions{ @@ -44,7 +35,16 @@ func ConfigureAndEnableRequestLogging(file string) error { zap.DebugLevel, ) - requestLogger = zap.New(core).Named("RequestLogger") + return zap.New(core).Named("RequestLogger"), nil +} + +func ConfigureAndEnableRequestLogging(file string) error { + logger, err := CreateRequestLogger(file) + if err != nil { + return err + } + + requestLogger = logger return nil } diff --git a/mobile/status_request_log.go b/mobile/callog/status_request_log.go similarity index 87% rename from mobile/status_request_log.go rename to mobile/callog/status_request_log.go index b9b7e096d..ae2d5f789 100644 --- a/mobile/status_request_log.go +++ b/mobile/callog/status_request_log.go @@ -1,4 +1,4 @@ -package statusgo +package callog import ( "fmt" @@ -11,7 +11,6 @@ import ( "go.uber.org/zap" "github.com/status-im/status-go/logutils" - "github.com/status-im/status-go/logutils/requestlog" ) var sensitiveKeys = []string{ @@ -47,7 +46,7 @@ func getShortFunctionName(fn any) string { return parts[len(parts)-1] } -// call executes the given function and logs request details if logging is enabled +// Call executes the given function and logs request details if logging is enabled // // Parameters: // - fn: The function to be executed @@ -59,10 +58,10 @@ func getShortFunctionName(fn any) string { // Functionality: // 1. Sets up panic recovery to log and re-panic // 2. Records start time if request logging is enabled -// 3. Uses reflection to call the given function +// 3. Uses reflection to Call the given function // 4. If request logging is enabled, logs method name, parameters, response, and execution duration // 5. Removes sensitive information before logging -func call(fn any, params ...any) any { +func Call(logger *zap.Logger, fn any, params ...any) any { defer func() { if r := recover(); r != nil { logutils.ZapLogger().Error("panic found in call", zap.Any("error", r), zap.Stack("stacktrace")) @@ -72,7 +71,7 @@ func call(fn any, params ...any) any { var startTime time.Time - requestLoggingEnabled := requestlog.IsRequestLoggingEnabled() + requestLoggingEnabled := logger != nil if requestLoggingEnabled { startTime = time.Now() } @@ -102,7 +101,7 @@ func call(fn any, params ...any) any { paramsString := removeSensitiveInfo(fmt.Sprintf("%+v", params)) respString := removeSensitiveInfo(fmt.Sprintf("%+v", resp)) - requestlog.GetRequestLogger().Debug("call", + logger.Debug("call", zap.String("method", methodName), zap.String("params", paramsString), zap.String("resp", respString), @@ -113,8 +112,8 @@ func call(fn any, params ...any) any { return resp } -func callWithResponse(fn any, params ...any) string { - resp := call(fn, params...) +func CallWithResponse(logger *zap.Logger, fn any, params ...any) string { + resp := Call(logger, fn, params...) if resp == nil { return "" } diff --git a/mobile/status_request_log_test.go b/mobile/callog/status_request_log_test.go similarity index 72% rename from mobile/status_request_log_test.go rename to mobile/callog/status_request_log_test.go index 2096f2d55..658ab82f9 100644 --- a/mobile/status_request_log_test.go +++ b/mobile/callog/status_request_log_test.go @@ -1,7 +1,6 @@ -package statusgo +package callog import ( - "encoding/json" "fmt" "os" "strings" @@ -12,9 +11,6 @@ import ( "github.com/stretchr/testify/require" "github.com/status-im/status-go/logutils/requestlog" - "github.com/status-im/status-go/multiaccounts" - "github.com/status-im/status-go/multiaccounts/settings" - "github.com/status-im/status-go/signal" ) func TestRemoveSensitiveInfo(t *testing.T) { @@ -66,11 +62,8 @@ func TestCall(t *testing.T) { require.NoError(t, err) // Enable request logging - err = requestlog.ConfigureAndEnableRequestLogging(tempLogFile.Name()) + logger, err := requestlog.CreateRequestLogger(tempLogFile.Name()) require.NoError(t, err) - - // Logger must not be nil after enabling - logger := requestlog.GetRequestLogger() require.NotNil(t, logger) // Test case 1: Normal execution @@ -80,7 +73,7 @@ func TestCall(t *testing.T) { testParam := "test input" expectedResult := "test result: test input" - result := callWithResponse(testFunc, testParam) + result := CallWithResponse(logger, testFunc, testParam) // Check the result if result != expectedResult { @@ -120,7 +113,7 @@ func TestCall(t *testing.T) { } require.PanicsWithValue(t, e, func() { - call(panicFunc) + Call(logger, panicFunc) }) // Check if the panic was logged @@ -135,35 +128,11 @@ func TestCall(t *testing.T) { } } +func initializeApplication(requestJSON string) string { + return "" +} + func TestGetFunctionName(t *testing.T) { fn := getShortFunctionName(initializeApplication) require.Equal(t, "initializeApplication", fn) } - -type testSignalHandler struct { - receivedSignal string -} - -func (t *testSignalHandler) HandleSignal(data string) { - t.receivedSignal = data -} - -func TestSetMobileSignalHandler(t *testing.T) { - // Setup - handler := &testSignalHandler{} - SetMobileSignalHandler(handler) - t.Cleanup(signal.ResetMobileSignalHandler) - - // Test data - testAccount := &multiaccounts.Account{Name: "test"} - testSettings := &settings.Settings{KeyUID: "0x1"} - testEnsUsernames := json.RawMessage(`{"test": "test"}`) - - // Action - signal.SendLoggedIn(testAccount, testSettings, testEnsUsernames, nil) - - // Assertions - require.Contains(t, handler.receivedSignal, `"key-uid":"0x1"`, "Signal should contain the correct KeyUID") - require.Contains(t, handler.receivedSignal, `"name":"test"`, "Signal should contain the correct account name") - require.Contains(t, handler.receivedSignal, `"ensUsernames":{"test":"test"}`, "Signal should contain the correct ENS usernames") -} diff --git a/mobile/init_logging_test.go b/mobile/init_logging_test.go index 95b37bfe3..4f2159b71 100644 --- a/mobile/init_logging_test.go +++ b/mobile/init_logging_test.go @@ -22,7 +22,7 @@ func TestInitLogging(t *testing.T) { require.Equal(t, `{"error":""}`, response) _, err := os.Stat(gethLogFile) require.NoError(t, err) - require.True(t, requestlog.IsRequestLoggingEnabled()) + require.NotNil(t, requestlog.GetRequestLogger()) // requests log file should not be created yet _, err = os.Stat(requestsLogFile) diff --git a/mobile/status.go b/mobile/status.go index c1bfd2766..9eb942d7a 100644 --- a/mobile/status.go +++ b/mobile/status.go @@ -47,8 +47,18 @@ import ( "github.com/status-im/status-go/services/typeddata" "github.com/status-im/status-go/signal" "github.com/status-im/status-go/transactions" + + "github.com/status-im/status-go/mobile/callog" ) +func call(fn any, params ...any) any { + return callog.Call(requestlog.GetRequestLogger(), fn, params...) +} + +func callWithResponse(fn any, params ...any) string { + return callog.CallWithResponse(requestlog.GetRequestLogger(), fn, params...) +} + type InitializeApplicationResponse struct { Accounts []multiaccounts.Account `json:"accounts"` CentralizedMetricsInfo *centralizedmetrics.MetricsInfo `json:"centralizedMetricsInfo"` diff --git a/mobile/status_test.go b/mobile/status_test.go new file mode 100644 index 000000000..eef81854d --- /dev/null +++ b/mobile/status_test.go @@ -0,0 +1,40 @@ +package statusgo + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/status-im/status-go/multiaccounts" + "github.com/status-im/status-go/multiaccounts/settings" + "github.com/status-im/status-go/signal" +) + +type testSignalHandler struct { + receivedSignal string +} + +func (t *testSignalHandler) HandleSignal(data string) { + t.receivedSignal = data +} + +func TestSetMobileSignalHandler(t *testing.T) { + // Setup + handler := &testSignalHandler{} + SetMobileSignalHandler(handler) + t.Cleanup(signal.ResetMobileSignalHandler) + + // Test data + testAccount := &multiaccounts.Account{Name: "test"} + testSettings := &settings.Settings{KeyUID: "0x1"} + testEnsUsernames := json.RawMessage(`{"test": "test"}`) + + // Action + signal.SendLoggedIn(testAccount, testSettings, testEnsUsernames, nil) + + // Assertions + require.Contains(t, handler.receivedSignal, `"key-uid":"0x1"`, "Signal should contain the correct KeyUID") + require.Contains(t, handler.receivedSignal, `"name":"test"`, "Signal should contain the correct account name") + require.Contains(t, handler.receivedSignal, `"ensUsernames":{"test":"test"}`, "Signal should contain the correct ENS usernames") +}