Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Goja from Execution Context Eval and from related parts #1189

Merged
merged 12 commits into from
Jan 25, 2024
Merged

Remove Goja from Execution Context Eval and from related parts #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6a4636d
Refactor away from goja in execctx eval
inancgumus Jan 24, 2024
eb125e9
Refactor away evaluate handle and friends
inancgumus Jan 24, 2024
548337e
Fix escaping goja from evals into business logic
inancgumus Jan 24, 2024
02bbe7a
Add type detection to ElementHandle.fill
inancgumus Jan 25, 2024
8b2a59a
Refactor screenshotter error
inancgumus Jan 25, 2024
848babb
Remove test browser goja helpers
inancgumus Jan 25, 2024
e239916
Add test asString helper
inancgumus Jan 25, 2024
7518104
Refactor test helpers to TB
inancgumus Jan 25, 2024
9e5a472
Refactor test assertions to asBool and asString
inancgumus Jan 25, 2024
a05e8e3
Refactor done strings to resultDone
inancgumus Jan 25, 2024
0e26184
Add temporary assertions for catching goja values
inancgumus Jan 25, 2024
3ed072b
Mark failing tests due to goja delivery to execctx
inancgumus Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 40 additions & 10 deletions browser/mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,20 @@ func mapJSHandle(vu moduleVU, jsh common.JSHandleAPI) mapping {
m := mapElementHandle(vu, jsh.AsElement())
return rt.ToValue(m).ToObject(rt)
},
"dispose": jsh.Dispose,
"evaluate": jsh.Evaluate,
"evaluateHandle": func(pageFunc goja.Value, args ...goja.Value) (mapping, error) {
h, err := jsh.EvaluateHandle(pageFunc, args...)
"dispose": jsh.Dispose,
"evaluate": func(pageFunc goja.Value, gargs ...goja.Value) any {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
return jsh.Evaluate(pageFunc.String(), args...)
},
"evaluateHandle": func(pageFunc goja.Value, gargs ...goja.Value) (mapping, error) {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
h, err := jsh.EvaluateHandle(pageFunc.String(), args...)
if err != nil {
return nil, err //nolint:wrapcheck
}
Expand Down Expand Up @@ -348,9 +358,19 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
"content": f.Content,
"dblclick": f.Dblclick,
"dispatchEvent": f.DispatchEvent,
"evaluate": f.Evaluate,
"evaluateHandle": func(pageFunction goja.Value, args ...goja.Value) (mapping, error) {
jsh, err := f.EvaluateHandle(pageFunction, args...)
"evaluate": func(pageFunction goja.Value, gargs ...goja.Value) any {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
return f.Evaluate(pageFunction.String(), args...)
},
"evaluateHandle": func(pageFunction goja.Value, gargs ...goja.Value) (mapping, error) {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
jsh, err := f.EvaluateHandle(pageFunction.String(), args...)
if err != nil {
return nil, err //nolint:wrapcheck
}
Expand Down Expand Up @@ -540,9 +560,19 @@ func mapPage(vu moduleVU, p *common.Page) mapping {
"dragAndDrop": p.DragAndDrop,
"emulateMedia": p.EmulateMedia,
"emulateVisionDeficiency": p.EmulateVisionDeficiency,
"evaluate": p.Evaluate,
"evaluateHandle": func(pageFunc goja.Value, args ...goja.Value) (mapping, error) {
jsh, err := p.EvaluateHandle(pageFunc, args...)
"evaluate": func(pageFunction goja.Value, gargs ...goja.Value) any {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
return p.Evaluate(pageFunction.String(), args...)
},
"evaluateHandle": func(pageFunc goja.Value, gargs ...goja.Value) (mapping, error) {
args := make([]any, 0, len(gargs))
for _, a := range gargs {
args = append(args, a.Export())
}
jsh, err := p.EvaluateHandle(pageFunc.String(), args...)
if err != nil {
return nil, err //nolint:wrapcheck
}
Expand Down
95 changes: 46 additions & 49 deletions common/element_handle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,8 @@ func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position)
}

// Either we're done or an error happened (returned as "error:..." from JS)
const done = "done"
v, ok := result.(goja.Value)
if !ok {
return false, fmt.Errorf("unexpected type %T", result)
}
if v.ExportType().Kind() != reflect.String {
const done = resultDone
if v, ok := result.(string); !ok {
// We got a { hitTargetDescription: ... } result
// Meaning: Another element is preventing pointer events.
//
Expand All @@ -107,8 +103,8 @@ func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position)
// because we don't need any more functionality from this JS function
// right now.
return false, errorFromDOMError("error:intercept")
} else if v.String() != done {
return false, errorFromDOMError(v.String())
} else if v != done {
return false, errorFromDOMError(v)
}

return true, nil
Expand All @@ -128,18 +124,11 @@ func (h *ElementHandle) checkElementState(_ context.Context, state string) (*boo
if err != nil {
return nil, err
}
v, ok := result.(goja.Value)
if !ok {
return nil, fmt.Errorf("unexpected type %T", result)
}
//nolint:exhaustive
switch v.ExportType().Kind() {
case reflect.String: // An error happened (returned as "error:..." from JS)
return nil, errorFromDOMError(v.String())
case reflect.Bool:
returnVal := new(bool)
*returnVal = v.ToBoolean()
return returnVal, nil
switch v := result.(type) {
case string: // An error happened (returned as "error:..." from JS)
return nil, errorFromDOMError(v)
case bool:
return &v, nil
}

return nil, fmt.Errorf(
Expand Down Expand Up @@ -273,11 +262,11 @@ func (h *ElementHandle) fill(_ context.Context, value string) error {
if err != nil {
return err
}
v, ok := result.(goja.Value)
s, ok := result.(string)
if !ok {
return fmt.Errorf("unexpected type %T", result)
}
if s := v.String(); s != resultDone {
if ok && s != resultDone {
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
// Either we're done or an error happened (returned as "error:..." from JS)
return errorFromDOMError(s)
}
Expand All @@ -299,12 +288,15 @@ func (h *ElementHandle) focus(apiCtx context.Context, resetSelectionIfNotFocused
if err != nil {
return err
}
switch result := result.(type) {
case string: // Either we're done or an error happened (returned as "error:..." from JS)
if result != "done" {
return errorFromDOMError(result)
}
s, ok := result.(string)
if !ok {
return fmt.Errorf("unexpected type %T", result)
}
if s != resultDone {
// Either we're done or an error happened (returned as "error:..." from JS)
return errorFromDOMError(s)
}

return nil
}

Expand All @@ -321,7 +313,7 @@ func (h *ElementHandle) getAttribute(apiCtx context.Context, name string) (any,
return h.eval(apiCtx, opts, js)
}

func (h *ElementHandle) hover(apiCtx context.Context, p *Position) error {
func (h *ElementHandle) hover(_ context.Context, p *Position) error {
return h.frame.page.Mouse.move(p.X, p.Y, NewMouseMoveOptions())
}

Expand All @@ -348,6 +340,7 @@ func (h *ElementHandle) innerText(apiCtx context.Context) (any, error) {
forceCallable: true,
returnByValue: true,
}

return h.eval(apiCtx, opts, js)
}

Expand Down Expand Up @@ -572,7 +565,7 @@ func (h *ElementHandle) selectOption(apiCtx context.Context, values goja.Value)
}
switch result := result.(type) {
case string: // An error happened (returned as "error:..." from JS)
if result != "done" {
if result != resultDone {
return nil, errorFromDOMError(result)
}
}
Expand All @@ -595,7 +588,7 @@ func (h *ElementHandle) selectText(apiCtx context.Context) error {
}
switch result := result.(type) {
case string: // Either we're done or an error happened (returned as "error:..." from JS)
if result != "done" {
if result != resultDone {
return errorFromDOMError(result)
}
}
Expand Down Expand Up @@ -667,19 +660,14 @@ func (h *ElementHandle) waitForElementState(
if err != nil {
return false, errorFromDOMError(err)
}
v, ok := result.(goja.Value)
if !ok {
return false, fmt.Errorf("unexpected type %T", result)
}
//nolint:exhaustive
switch v.ExportType().Kind() {
case reflect.String: // Either we're done or an error happened (returned as "error:..." from JS)
if v.String() == "done" {
switch v := result.(type) {
case string: // Either we're done or an error happened (returned as "error:..." from JS)
if v == resultDone {
return true, nil
}
return false, errorFromDOMError(v.String())
case reflect.Bool:
return v.ToBoolean(), nil
return false, errorFromDOMError(v)
case bool:
return v, nil
}

return false, fmt.Errorf(
Expand Down Expand Up @@ -831,7 +819,7 @@ func (h *ElementHandle) Focus() {
}

// GetAttribute retrieves the value of specified element attribute.
func (h *ElementHandle) GetAttribute(name string) goja.Value {
func (h *ElementHandle) GetAttribute(name string) any {
fn := func(apiCtx context.Context, handle *ElementHandle) (any, error) {
return handle.getAttribute(apiCtx, name)
}
Expand All @@ -843,7 +831,9 @@ func (h *ElementHandle) GetAttribute(name string) goja.Value {
}
applySlowMo(h.ctx)

return asGojaValue(h.ctx, v)
// TODO: return any with error
inancgumus marked this conversation as resolved.
Show resolved Hide resolved

return v
}

// Hover scrolls element into view and hovers over its center point.
Expand Down Expand Up @@ -876,7 +866,9 @@ func (h *ElementHandle) InnerHTML() string {
}
applySlowMo(h.ctx)

return gojaValueToString(h.ctx, v)
// TODO: handle error
inancgumus marked this conversation as resolved.
Show resolved Hide resolved

return v.(string) //nolint:forcetypeassert
}

// InnerText returns the inner text of the element.
Expand All @@ -892,7 +884,9 @@ func (h *ElementHandle) InnerText() string {
}
applySlowMo(h.ctx)

return gojaValueToString(h.ctx, v)
// TODO: handle error
inancgumus marked this conversation as resolved.
Show resolved Hide resolved

return v.(string) //nolint:forcetypeassert
}

func (h *ElementHandle) InputValue(opts goja.Value) string {
Expand All @@ -910,7 +904,9 @@ func (h *ElementHandle) InputValue(opts goja.Value) string {
}
applySlowMo(h.ctx)

return gojaValueToString(h.ctx, v)
// TODO: return error
inancgumus marked this conversation as resolved.
Show resolved Hide resolved

return v.(string) //nolint:forcetypeassert
}

// IsChecked checks if a checkbox or radio is checked.
Expand Down Expand Up @@ -1212,7 +1208,6 @@ func (h *ElementHandle) ScrollIntoViewIfNeeded(opts goja.Value) {
}

func (h *ElementHandle) SelectOption(values goja.Value, opts goja.Value) []string {
rt := h.execCtx.vu.Runtime()
actionOpts := NewElementHandleBaseOptions(h.defaultTimeout())
if err := actionOpts.Parse(h.ctx, opts); err != nil {
k6ext.Panic(h.ctx, "parsing selectOption options: %w", err)
Expand All @@ -1226,7 +1221,7 @@ func (h *ElementHandle) SelectOption(values goja.Value, opts goja.Value) []strin
k6ext.Panic(h.ctx, "selecting options: %w", err)
}
var returnVal []string
if err := rt.ExportTo(asGojaValue(h.ctx, selectedOptions), &returnVal); err != nil {
if err := convert(selectedOptions, &returnVal); err != nil {
k6ext.Panic(h.ctx, "unpacking selected options: %w", err)
}

Expand Down Expand Up @@ -1287,7 +1282,9 @@ func (h *ElementHandle) TextContent() string {
}
applySlowMo(h.ctx)

return gojaValueToString(h.ctx, v)
// TODO: handle error
inancgumus marked this conversation as resolved.
Show resolved Hide resolved

return v.(string) //nolint:forcetypeassert
}

// Timeout will return the default timeout or the one set by the user.
Expand Down
27 changes: 16 additions & 11 deletions common/execution_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/chromedp/cdproto/dom"
"github.com/chromedp/cdproto/runtime"
"github.com/chromedp/cdproto/target"
"github.com/dop251/goja"
)

const evaluationScriptURL = "__xk6_browser_evaluation_script__"
Expand Down Expand Up @@ -155,6 +154,9 @@ func (e *ExecutionContext) adoptElementHandle(eh *ElementHandle) (*ElementHandle
func (e *ExecutionContext) eval(
apiCtx context.Context, opts evalOptions, js string, args ...any,
) (any, error) {
if escapesGojaValues(args...) {
return nil, errors.New("goja.Value escaped")
}
e.logger.Debugf(
"ExecutionContext:eval",
"sid:%s stid:%s fid:%s ectxid:%d furl:%q %s",
Expand Down Expand Up @@ -289,34 +291,37 @@ func (e *ExecutionContext) getInjectedScript(apiCtx context.Context) (JSHandleAP

// Eval evaluates the provided JavaScript within this execution context and
// returns a value or handle.
func (e *ExecutionContext) Eval(
apiCtx context.Context, js goja.Value, args ...goja.Value,
) (any, error) {
func (e *ExecutionContext) Eval(apiCtx context.Context, js string, args ...any) (any, error) {
if escapesGojaValues(args...) {
return nil, errors.New("goja.Value escaped")
}
opts := evalOptions{
forceCallable: true,
returnByValue: true,
}
evalArgs := make([]any, 0, len(args))
for _, a := range args {
evalArgs = append(evalArgs, a.Export())
evalArgs = append(evalArgs, a)
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
}
return e.eval(apiCtx, opts, js.ToString().String(), evalArgs...)

return e.eval(apiCtx, opts, js, evalArgs...)
}

// EvalHandle evaluates the provided JavaScript within this execution context
// and returns a JSHandle.
func (e *ExecutionContext) EvalHandle(
apiCtx context.Context, js goja.Value, args ...goja.Value,
) (JSHandleAPI, error) {
func (e *ExecutionContext) EvalHandle(apiCtx context.Context, js string, args ...any) (JSHandleAPI, error) {
if escapesGojaValues(args...) {
return nil, errors.New("goja.Value escaped")
}
opts := evalOptions{
forceCallable: true,
returnByValue: false,
}
evalArgs := make([]any, 0, len(args))
for _, a := range args {
evalArgs = append(evalArgs, a.Export())
evalArgs = append(evalArgs, a)
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
}
res, err := e.eval(apiCtx, opts, js.ToString().String(), evalArgs...)
res, err := e.eval(apiCtx, opts, js, evalArgs...)
if err != nil {
return nil, err
}
Expand Down
Loading
Loading