Refactor ExecutionContext.eval() and related code
#293
Conversation
Some exceptions were previously not exposed to the script (e.g. promise rejection) so this makes sure the exception message is properly exposed.
This was causing data races if the runtime was also being used by the event loop goroutine, since goja is not thread-safe. It also moves towards removing the dependency on goja from internal methods, which is what we're aiming for. A good rule of thumb to follow from now on is to never use goja on unexported methods, and only on those that are directly exposed to JS. Part of #271
|
@olegbespalov Correct, it doesn't fix the issue, but it avoids a couple of issues found while working on the fix, and does some general improvements. See the commit messages for details. I hope to create the fix PR later today. |
There was a problem hiding this comment.
It's great to see that we're making progress towards #271 👏
I have some findings.
| expErr := fmt.Errorf("%w", errors.New(tc.errMsg)) | ||
| require.IsType(t, expErr, got) | ||
| gotErr, ok := got.(error) | ||
| require.True(t, ok) | ||
| assert.Contains(t, gotErr.Error(), expErr.Error()) |
There was a problem hiding this comment.
I'm curious whether there was a reason for wrapping the error.
gotErr, ok := got.(error)
require.True(t, ok)
expErr := errors.New(tc.errMsg)
assert.Contains(t, gotErr.Error(), expErr.Error())
There was a problem hiding this comment.
I needed to wrap it for IsType() to pass. But while working on #300 that was failing as the error wasn't wrapped, so instead of handling both cases, I just removed it altogether to simplify. The type check is not important anyway.
| t.Run("ok/func", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tb := newTestBrowser(t) | ||
| p := tb.NewPage(nil) | ||
|
|
||
| got := p.Evaluate(tb.rt.ToValue("() => document.location.toString()")) | ||
|
|
||
| require.IsType(t, tb.rt.ToValue(""), got) | ||
| gotVal, _ := got.(goja.Value) | ||
| assert.Equal(t, "about:blank", gotVal.Export()) | ||
| }) |
There was a problem hiding this comment.
While this test can check that Evaluate can execute a proper function, it doesn't test whether Evaluate handles arguments correctly.
For example, the test can't detect the following:
func (f *Frame) evaluate(
apiCtx context.Context,
world executionWorld,
opts evalOptions, pageFunc goja.Value, args ...goja.Value,
) (interface{}, error) {
...
for _, a := range args {
// evalArgs = append(evalArgs, a.Export())
_ = a
}
...An improved version of the test can be as follows (or as another test):
...
got := p.Evaluate(
tb.rt.ToValue("(v) => { window['v'] = v; return window['v']; }"),
tb.rt.ToValue("hey"),
)
require.IsType(t, tb.rt.ToValue("hey"), got)
gotVal, _ := got.(goja.Value)
assert.Equal(t, "hey", gotVal.Export())
...There was a problem hiding this comment.
Right, good point. I changed it in 72a7ff4.
This was done as part of the work for #291, but I split it off into a separate PR to make reviewing easier. See the commits for details.