From 2871e167f865d412f5c361267d7316b0927a8b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20L=C3=B3pez=20de=20la=20Franca=20Beltran?= Date: Tue, 24 Oct 2023 17:41:58 +0200 Subject: [PATCH] Apply review feedback --- cmd/root.go | 5 +-- errext/{print.go => format.go} | 12 +++---- errext/{print_test.go => format_test.go} | 42 ++++++++++-------------- js/runner.go | 14 ++++---- js/summary_test.go | 3 +- 5 files changed, 35 insertions(+), 41 deletions(-) rename errext/{print.go => format.go} (64%) rename errext/{print_test.go => format_test.go} (52%) diff --git a/cmd/root.go b/cmd/root.go index c199a80bf5d..c23605b9370 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -112,9 +112,10 @@ func (c *rootCommand) execute() { exitCode = int(ecerr.ExitCode()) } - errext.Fprint(c.globalState.Logger, err) + errText, fields := errext.Format(err) + c.globalState.Logger.WithFields(fields).Error(errText) if c.loggerIsRemote { - errext.Fprint(c.globalState.FallbackLogger, err) + c.globalState.FallbackLogger.WithFields(fields).Error(errText) } } diff --git a/errext/print.go b/errext/format.go similarity index 64% rename from errext/print.go rename to errext/format.go index c2291cd53c7..d52088c63ac 100644 --- a/errext/print.go +++ b/errext/format.go @@ -2,16 +2,14 @@ package errext import ( "errors" - - "github.com/sirupsen/logrus" ) -// Fprint formats the given error and writes it to l. +// Format formats the given error and writes it to l. // In case of Exception, it uses the stack trace as the error message. // In case of HasHint, it also adds the hint as a field. -func Fprint(l logrus.FieldLogger, err error) { +func Format(err error) (string, map[string]interface{}) { if err == nil { - return + return "", nil } errText := err.Error() @@ -20,11 +18,11 @@ func Fprint(l logrus.FieldLogger, err error) { errText = xerr.StackTrace() } - fields := logrus.Fields{} + fields := make(map[string]interface{}) var herr HasHint if errors.As(err, &herr) { fields["hint"] = herr.Hint() } - l.WithFields(fields).Error(errText) + return errText, fields } diff --git a/errext/print_test.go b/errext/format_test.go similarity index 52% rename from errext/print_test.go rename to errext/format_test.go index cae81716318..c8a94f5a8e2 100644 --- a/errext/print_test.go +++ b/errext/format_test.go @@ -1,61 +1,53 @@ package errext_test import ( - "bytes" "errors" "testing" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "go.k6.io/k6/errext" ) -func TestFprint(t *testing.T) { +func TestFormat(t *testing.T) { t.Parallel() - init := func() (*bytes.Buffer, logrus.FieldLogger) { - var buf bytes.Buffer - logger := logrus.New() - logger.Out = &buf - return &buf, logger - } - t.Run("Nil", func(t *testing.T) { t.Parallel() - buf, logger := init() - errext.Fprint(logger, nil) - assert.Equal(t, "", buf.String()) + errorText, fields := errext.Format(nil) + assert.Equal(t, "", errorText) + assert.Empty(t, fields) }) t.Run("Simple", func(t *testing.T) { t.Parallel() - buf, logger := init() - errext.Fprint(logger, errors.New("simple error")) - assert.Contains(t, buf.String(), "level=error msg=\"simple error\"") + errorText, fields := errext.Format(errors.New("simple error")) + assert.Equal(t, "simple error", errorText) + assert.Empty(t, fields) }) t.Run("Exception", func(t *testing.T) { t.Parallel() - buf, logger := init() err := fakeException{error: errors.New("simple error"), stack: "stack trace"} - errext.Fprint(logger, err) - assert.Contains(t, buf.String(), "level=error msg=\"stack trace\"") + errorText, fields := errext.Format(err) + assert.Equal(t, "stack trace", errorText) + assert.Empty(t, fields) }) t.Run("Hint", func(t *testing.T) { t.Parallel() - buf, logger := init() err := errext.WithHint(errors.New("error with hint"), "hint message") - errext.Fprint(logger, err) - assert.Contains(t, buf.String(), "level=error msg=\"error with hint\" hint=\"hint message\"") + errorText, fields := errext.Format(err) + assert.Equal(t, "error with hint", errorText) + assert.Equal(t, map[string]interface{}{"hint": "hint message"}, fields) }) t.Run("ExceptionWithHint", func(t *testing.T) { t.Parallel() - buf, logger := init() err := fakeException{error: errext.WithHint(errors.New("error with hint"), "hint message"), stack: "stack trace"} - errext.Fprint(logger, err) - assert.Contains(t, buf.String(), "level=error msg=\"stack trace\" hint=\"hint message\"") + errorText, fields := errext.Format(err) + assert.Equal(t, "stack trace", errorText) + assert.Equal(t, map[string]interface{}{"hint": "hint message"}, fields) }) } diff --git a/js/runner.go b/js/runner.go index ac3884ca7b6..2303accab38 100644 --- a/js/runner.go +++ b/js/runner.go @@ -397,14 +397,16 @@ func (r *Runner) HandleSummary(ctx context.Context, summary *lib.Summary) (map[s callbackResult := goja.Undefined() fn := vu.getExported(consts.HandleSummaryFn) if fn != nil { - if handleSummaryFn, ok := goja.AssertFunction(fn); ok { - callbackResult, _, _, err = vu.runFn(summaryCtx, false, handleSummaryFn, nil, vu.Runtime.ToValue(summaryDataForJS)) - if err != nil { - errext.Fprint(r.preInitState.Logger, err) - } - } else if fn != nil { + handleSummaryFn, ok := goja.AssertFunction(fn) + if !ok { return nil, fmt.Errorf("exported identifier %s must be a function", consts.HandleSummaryFn) } + + callbackResult, _, _, err = vu.runFn(summaryCtx, false, handleSummaryFn, nil, vu.Runtime.ToValue(summaryDataForJS)) + if err != nil { + errText, fields := errext.Format(err) + r.preInitState.Logger.WithFields(fields).Error(errText) + } } wrapper := strings.Replace(summaryWrapperLambdaCode, "/*JSLIB_SUMMARY_CODE*/", jslibSummaryCode, 1) diff --git a/js/summary_test.go b/js/summary_test.go index a43f68aa4b0..0c597077f00 100644 --- a/js/summary_test.go +++ b/js/summary_test.go @@ -698,5 +698,6 @@ func TestExceptionInHandleSummaryFallsBackToTextSummary(t *testing.T) { assert.Equal(t, 1, len(logErrors)) errMsg, err := logErrors[0].String() require.NoError(t, err) - assert.Contains(t, errMsg, "intentional error") + assert.Contains(t, errMsg, "\"Error: intentional error\\n\\tat file:///script.js:5:11(3)\\n") + assert.Equal(t, logErrors[0].Data, logrus.Fields{"hint": "script exception"}) }