Address some of the review comments in #130

Author: Ivan Mirić

common/frame_session.go

@@ -79,7 +79,7 @@ type FrameSession struct {
 
 	logger *Logger
 	// logger that will properly serialize RemoteObject instances
-	loggerObj *logrus.Logger
+	serializer *logrus.Logger
 }
 
 func NewFrameSession(
@@ -88,11 +88,6 @@ func NewFrameSession(
 ) (*FrameSession, error) {
 	logger.Debugf("NewFrameSession", "sid:%v tid:%v", session.id, targetID)
 	state := k6lib.GetState(ctx)
-	loggerObj := &logrus.Logger{
-		Out:       state.Logger.Out,
-		Level:     state.Logger.Level,
-		Formatter: &mapAsObjectFormatter{state.Logger.Formatter},
-	}
 	fs := FrameSession{
 		ctx:                  ctx, // TODO: create cancelable context that can be used to cancel and close all child sessions
 		session:              session,
@@ -108,7 +103,11 @@ func NewFrameSession(
 		eventCh:              make(chan Event),
 		childSessions:        make(map[cdp.FrameID]*FrameSession),
 		logger:               logger,
-		loggerObj:            loggerObj,
+		serializer: &logrus.Logger{
+			Out:       state.Logger.Out,
+			Level:     state.Logger.Level,
+			Formatter: &consoleLogFormatter{state.Logger.Formatter},
+		},
 	}
 	var err error
 	if fs.parent != nil {
@@ -467,14 +466,13 @@ func (fs *FrameSession) navigateFrame(frame *Frame, url, referrer string) (strin
 }
 
 func (fs *FrameSession) onConsoleAPICalled(event *runtime.EventConsoleAPICalled) {
-	state := k6lib.GetState(fs.ctx)
 	// TODO: switch to using browser logger instead of directly outputting to k6 logging system
-	l := fs.loggerObj.
+	l := fs.serializer.
 		WithTime(event.Timestamp.Time()).
 		WithField("source", "browser-console-api")
 
-	if state.Group.Path != "" {
-		l = l.WithField("group", state.Group.Path)
+	if s := k6lib.GetState(fs.ctx); s.Group.Path != "" {
+		l = l.WithField("group", s.Group.Path)
 	}
 
 	var parsedObjects []interface{}
@@ -490,9 +488,7 @@ func (fs *FrameSession) onConsoleAPICalled(event *runtime.EventConsoleAPICalled)
 	l = l.WithField("objects", parsedObjects)
 
 	switch event.Type {
-	case "log":
-		l.Info()
-	case "info":
+	case "log", "info":
 		l.Info()
 	case "warning":
 		l.Warn()

common/helper_test.go

@@ -38,20 +38,19 @@ import (
 	k6common "go.k6.io/k6/js/common"
 )
 
-func newExecCtx() (context.Context, *ExecutionContext, *goja.Runtime) {
+func newExecCtx() (*ExecutionContext, context.Context, *goja.Runtime) {
 	ctx := context.Background()
 	logger := NewLogger(ctx, logrus.New(), false, nil)
 	execCtx := NewExecutionContext(ctx, nil, nil, runtime.ExecutionContextID(123456789), logger)
-	rt := goja.New()
 
-	return ctx, execCtx, rt
+	return execCtx, ctx, goja.New()
 }
 
 func TestHelpersConvertArgument(t *testing.T) {
 	t.Parallel()
 
 	t.Run("int64", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		var value int64 = 777
 		arg, _ := convertArgument(ctx, execCtx, rt.ToValue(value))
 
@@ -63,7 +62,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("int64 maxint", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 
 		var value int64 = math.MaxInt32 + 1
 		arg, _ := convertArgument(ctx, execCtx, rt.ToValue(value))
@@ -75,7 +74,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("float64", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 
 		var value float64 = 777.0
 		arg, _ := convertArgument(ctx, execCtx, rt.ToValue(value))
@@ -88,7 +87,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("float64 unserializable values", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 
 		unserializableValues := []struct {
 			value    float64
@@ -122,7 +121,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("bool", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 
 		var value bool = true
 		arg, _ := convertArgument(ctx, execCtx, rt.ToValue(value))
@@ -135,7 +134,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("string", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		var value string = "hello world"
 		arg, _ := convertArgument(ctx, execCtx, rt.ToValue(value))
 
@@ -147,7 +146,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("*BaseJSHandle", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		timeoutSetings := NewTimeoutSettings(nil)
 		frameManager := NewFrameManager(ctx, nil, nil, timeoutSetings, NewLogger(ctx, NullLogger(), false, nil))
 		frame := NewFrame(ctx, frameManager, nil, cdp.FrameID("frame_id_0123456789"))
@@ -168,7 +167,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("*BaseJSHandle wrong context", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		timeoutSetings := NewTimeoutSettings(nil)
 		frameManager := NewFrameManager(ctx, nil, nil, timeoutSetings, NewLogger(ctx, NullLogger(), false, nil))
 		frame := NewFrame(ctx, frameManager, nil, cdp.FrameID("frame_id_0123456789"))
@@ -188,7 +187,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("*BaseJSHandle is disposed", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		timeoutSetings := NewTimeoutSettings(nil)
 		frameManager := NewFrameManager(ctx, nil, nil, timeoutSetings, NewLogger(ctx, NullLogger(), false, nil))
 		frame := NewFrame(ctx, frameManager, nil, cdp.FrameID("frame_id_0123456789"))
@@ -208,7 +207,7 @@ func TestHelpersConvertArgument(t *testing.T) {
 	})
 
 	t.Run("*BaseJSHandle as *ElementHandle", func(t *testing.T) {
-		ctx, execCtx, rt := newExecCtx()
+		execCtx, ctx, rt := newExecCtx()
 		timeoutSetings := NewTimeoutSettings(nil)
 		frameManager := NewFrameManager(ctx, nil, nil, timeoutSetings, NewLogger(ctx, NullLogger(), false, nil))
 		frame := NewFrame(ctx, frameManager, nil, cdp.FrameID("frame_id_0123456789"))

common/js_handle.go

@@ -49,27 +49,23 @@ func NewJSHandle(
 	ctx context.Context, session *Session, execCtx *ExecutionContext,
 	frame *Frame, remoteObject *runtime.RemoteObject, logger *Logger,
 ) api.JSHandle {
-	if remoteObject.Subtype == "node" && execCtx.Frame() != nil {
-		return &ElementHandle{
-			BaseJSHandle: BaseJSHandle{
-				ctx:          ctx,
-				session:      session,
-				execCtx:      execCtx,
-				remoteObject: remoteObject,
-				disposed:     false,
-				logger:       logger,
-			},
-			frame: frame,
-		}
-	}
-	return &BaseJSHandle{
+	eh := &BaseJSHandle{
 		ctx:          ctx,
 		session:      session,
 		execCtx:      execCtx,
 		remoteObject: remoteObject,
 		disposed:     false,
 		logger:       logger,
 	}
+
+	if remoteObject.Subtype == "node" && execCtx.Frame() != nil {
+		return &ElementHandle{
+			BaseJSHandle: *eh,
+			frame:        frame,
+		}
+	}
+
+	return eh
 }
 
 // AsElement returns an element handle if this JSHandle is a reference to a JS HTML element
@@ -132,10 +128,11 @@ func (h *BaseJSHandle) GetProperties() map[string]api.JSHandle {
 
 	props := make(map[string]api.JSHandle, len(result))
 	for i := 0; i < len(result); i++ {
-		if result[i].Enumerable {
-			props[result[i].Name] = NewJSHandle(
-				h.ctx, h.session, h.execCtx, h.execCtx.Frame(), result[i].Value, h.logger)
+		if !result[i].Enumerable {
+			continue
 		}
+		props[result[i].Name] = NewJSHandle(
+			h.ctx, h.session, h.execCtx, h.execCtx.Frame(), result[i].Value, h.logger)
 	}
 	return props
 }

common/logger.go

@@ -160,11 +160,13 @@ func goRoutineID() int {
 	return id
 }
 
-type mapAsObjectFormatter struct {
+type consoleLogFormatter struct {
 	logrus.Formatter
 }
 
-func (f *mapAsObjectFormatter) Format(entry *logrus.Entry) ([]byte, error) {
+// Format assembles a message from marshalling elements in the "objects" field
+// to JSON separated by space, and deletes the field when done.
+func (f *consoleLogFormatter) Format(entry *logrus.Entry) ([]byte, error) {
 	if objects, ok := entry.Data["objects"].([]interface{}); ok {
 		var msg []string
 		for _, obj := range objects {

common/logger_test.go

@@ -34,31 +34,33 @@ func (f *testLogFormatter) Format(entry *logrus.Entry) ([]byte, error) {
 	return []byte(entry.Message), nil
 }
 
-func TestMapAsObjectFormatter(t *testing.T) {
+func TestConsoleLogFormatter(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
 		objects  []interface{}
 		expected string
 	}{
 		{objects: nil, expected: ""},
-		{objects: []interface{}{
-			map[string]interface{}{"one": 1, "two": "two"},
-			map[string]interface{}{"nested": map[string]interface{}{
-				"sub": float64(7.777),
-			}},
-		},
+		{
+			objects: []interface{}{
+				map[string]interface{}{"one": 1, "two": "two"},
+				map[string]interface{}{"nested": map[string]interface{}{
+					"sub": float64(7.777),
+				}},
+			},
 			expected: `{"one":1,"two":"two"} {"nested":{"sub":7.777}}`,
 		},
-		{objects: []interface{}{
-			map[string]interface{}{"one": 1, "fail": make(chan int)},
-			map[string]interface{}{"two": 2},
-		},
+		{
+			objects: []interface{}{
+				map[string]interface{}{"one": 1, "fail": make(chan int)},
+				map[string]interface{}{"two": 2},
+			},
 			expected: `{"two":2}`,
 		},
 	}
 
-	fmtr := &mapAsObjectFormatter{&testLogFormatter{}}
+	fmtr := &consoleLogFormatter{&testLogFormatter{}}
 
 	for _, tc := range testCases {
 		var data logrus.Fields