Refactor JSONValue to return an error

Instead of panicking which can cause unexpected side affects, return an error.
grafana · Jan 24, 2024 · ebefa9d · ebefa9d
1 parent 957e23b
commit ebefa9d
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 15 deletions.
diff --git a/common/frame.go b/common/frame.go
@@ -1460,7 +1460,11 @@ func (f *Frame) selectOption(selector string, values goja.Value, opts *FrameSele
 	}
 	vals := make([]string, 0, len(optHandles))
 	for _, oh := range optHandles {
-		vals = append(vals, oh.JSONValue())
+		val, err := oh.JSONValue()
+		if err != nil {
+			return nil, fmt.Errorf("reading value: %w", err)
+		}
+		vals = append(vals, val)
 		if err := oh.dispose(); err != nil {
 			return nil, fmt.Errorf("optionHandle.dispose: %w", err)
 		}

diff --git a/common/js_handle.go b/common/js_handle.go
@@ -25,7 +25,7 @@ type JSHandleAPI interface {
 	EvaluateHandle(pageFunc goja.Value, args ...goja.Value) (JSHandleAPI, error)
 	GetProperties() (map[string]JSHandleAPI, error)
 	GetProperty(propertyName string) JSHandleAPI
-	JSONValue() string
+	JSONValue() (string, error)
 	ObjectID() cdpruntime.RemoteObjectID
 }
 
@@ -168,7 +168,7 @@ func (h *BaseJSHandle) GetProperty(_ string) JSHandleAPI {
 }
 
 // JSONValue returns a JSON version of this JS handle.
-func (h *BaseJSHandle) JSONValue() string {
+func (h *BaseJSHandle) JSONValue() (string, error) {
 	if h.remoteObject.ObjectID != "" {
 		var result *runtime.RemoteObject
 		var err error
@@ -181,15 +181,17 @@ func (h *BaseJSHandle) JSONValue() string {
 		}
 		res, err := parseConsoleRemoteObject(h.logger, result)
 		if err != nil {
-			k6ext.Panic(h.ctx, "extracting value from remote object: %w", err)
+			return "", fmt.Errorf("extracting value from result: %w", err)
 		}
-		return res
+
+		return res, nil
 	}
 	res, err := parseConsoleRemoteObject(h.logger, h.remoteObject)
 	if err != nil {
-		k6ext.Panic(h.ctx, "extracting value from remote object: %w", err)
+		return "", fmt.Errorf("extracting value from remote object: %w", err)
 	}
-	return res
+
+	return res, nil
 }
 
 // ObjectID returns the remote object ID.

diff --git a/tests/js_handle_get_properties_test.go b/tests/js_handle_get_properties_test.go
@@ -28,6 +28,7 @@ func TestJSHandleGetProperties(t *testing.T) {
 	props, err := handle.GetProperties()
 	require.NoError(t, err, "expected no error when getting properties")
 
-	value := props["prop1"].JSONValue()
+	value, err := props["prop1"].JSONValue()
+	assert.NoError(t, err, "expected no error when getting JSONValue")
 	assert.Equal(t, value, "one", `expected property value of "one", got %q`, value)
 }
diff --git a/tests/page_test.go b/tests/page_test.go
@@ -823,7 +823,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "log", cm.Type)
 				assert.Equal(t, "this is a log message", cm.Text)
-				assert.Equal(t, "this is a log message", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "this is a log message", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -834,7 +836,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "debug", cm.Type)
 				assert.Equal(t, "this is a debug message", cm.Text)
-				assert.Equal(t, "this is a debug message", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "this is a debug message", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -845,7 +849,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "info", cm.Type)
 				assert.Equal(t, "this is an info message", cm.Text)
-				assert.Equal(t, "this is an info message", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "this is an info message", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -856,7 +862,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "error", cm.Type)
 				assert.Equal(t, "this is an error message", cm.Text)
-				assert.Equal(t, "this is an error message", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "this is an error message", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -867,7 +875,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "warning", cm.Type)
 				assert.Equal(t, "this is a warning message", cm.Text)
-				assert.Equal(t, "this is a warning message", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "this is a warning message", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -898,7 +908,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "table", cm.Type)
 				assert.Equal(t, "Array(2)", cm.Text)
-				assert.Equal(t, `[["Grafana","k6"],["Grafana","Mimir"]]`, cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, `[["Grafana","k6"],["Grafana","Mimir"]]`, val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},
@@ -909,7 +921,9 @@ func TestPageOn(t *testing.T) { //nolint:gocognit
 				t.Helper()
 				assert.Equal(t, "trace", cm.Type)
 				assert.Equal(t, "trace example", cm.Text)
-				assert.Equal(t, "trace example", cm.Args[0].JSONValue())
+				val, err := cm.Args[0].JSONValue()
+				assert.NoError(t, err)
+				assert.Equal(t, "trace example", val)
 				assert.True(t, cm.Page.URL() == blankPage, "url is not %s", blankPage)
 			},
 		},