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.

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

Move goja option parsing out of frame and page goto opts #1179

Merged
merged 3 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 20 additions & 6 deletions browser/mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,15 +366,22 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
return mapElementHandle(vu, fe), nil
},
"getAttribute": f.GetAttribute,
"goto": func(url string, opts goja.Value) *goja.Promise {
"goto": func(url string, opts goja.Value) (*goja.Promise, error) {
gopts := common.NewFrameGotoOptions(
inancgumus marked this conversation as resolved.
Show resolved Hide resolved
f.Referrer(),
f.NavigationTimeout(),
)
if err := gopts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing frame navigation options to %q: %w", url, err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
resp, err := f.Goto(url, opts)
resp, err := f.Goto(url, gopts)
if err != nil {
return nil, err //nolint:wrapcheck
}

return mapResponse(vu, resp), nil
})
}), nil
},
"hover": f.Hover,
"innerHTML": f.InnerHTML,
Expand Down Expand Up @@ -529,15 +536,22 @@ func mapPage(vu moduleVU, p *common.Page) mapping {
})
},
"goForward": p.GoForward,
"goto": func(url string, opts goja.Value) *goja.Promise {
"goto": func(url string, opts goja.Value) (*goja.Promise, error) {
gopts := common.NewFrameGotoOptions(
p.Referrer(),
p.NavigationTimeout(),
)
if err := gopts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing page navigation options to %q: %w", url, err)
}
return k6ext.Promise(vu.Context(), func() (any, error) {
resp, err := p.Goto(url, opts)
resp, err := p.Goto(url, gopts)
if err != nil {
return nil, err //nolint:wrapcheck
}

return mapResponse(vu, resp), nil
})
}), nil
},
"hover": p.Hover,
"innerHTML": p.InnerHTML,
Expand Down
31 changes: 17 additions & 14 deletions common/frame.go
Original file line number Diff line number Diff line change
Expand Up @@ -937,20 +937,23 @@ func (f *Frame) getAttribute(selector, name string, opts *FrameBaseOptions) (goj
return gv, nil
}

// Referrer returns the referrer of the frame from the network manager
// of the frame's session.
// It's an internal method not to be exposed as a JS API.
func (f *Frame) Referrer() string {
nm := f.manager.page.mainFrameSession.getNetworkManager()
return nm.extraHTTPHeaders["referer"]
}

// NavigationTimeout returns the navigation timeout of the frame.
// It's an internal method not to be exposed as a JS API.
func (f *Frame) NavigationTimeout() time.Duration {
return f.manager.timeoutSettings.navigationTimeout()
}

// Goto will navigate the frame to the specified URL and return a HTTP response object.
func (f *Frame) Goto(url string, opts goja.Value) (*Response, error) {
var (
netMgr = f.manager.page.mainFrameSession.getNetworkManager()
defaultReferer = netMgr.extraHTTPHeaders["referer"]
parsedOpts = NewFrameGotoOptions(
defaultReferer,
f.manager.timeoutSettings.navigationTimeout(),
)
)
if err := parsedOpts.Parse(f.ctx, opts); err != nil {
return nil, fmt.Errorf("parsing frame navigation options to %q: %w", url, err)
}
resp, err := f.manager.NavigateFrame(f, url, parsedOpts)
func (f *Frame) Goto(url string, opts *FrameGotoOptions) (*Response, error) {
resp, err := f.manager.NavigateFrame(f, url, opts)
if err != nil {
return nil, fmt.Errorf("navigating frame to %q: %w", url, err)
}
Expand All @@ -959,7 +962,7 @@ func (f *Frame) Goto(url string, opts goja.Value) (*Response, error) {
// Since response will be in an interface, it will never be nil,
// so we need to return nil explicitly.
if resp == nil {
return nil, nil
return nil, nil //nolint:nilnil
}

return resp, nil
Expand Down
15 changes: 14 additions & 1 deletion common/page.go
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ func (p *Page) GoForward(_ goja.Value) *Response {
}

// Goto will navigate the page to the specified URL and return a HTTP response object.
func (p *Page) Goto(url string, opts goja.Value) (*Response, error) {
func (p *Page) Goto(url string, opts *FrameGotoOptions) (*Response, error) {
p.logger.Debugf("Page:Goto", "sid:%v url:%q", p.sessionID(), url)
_, span := TraceAPICall(
p.ctx,
Expand Down Expand Up @@ -963,6 +963,19 @@ func (p *Page) MainFrame() *Frame {
return mf
}

// Referrer returns the page's referrer.
// It's an internal method not to be exposed as a JS API.
func (p *Page) Referrer() string {
nm := p.mainFrameSession.getNetworkManager()
return nm.extraHTTPHeaders["referer"]
}

// NavigationTimeout returns the page's navigation timeout.
// It's an internal method not to be exposed as a JS API.
func (p *Page) NavigationTimeout() time.Duration {
return p.frameManager.timeoutSettings.navigationTimeout()
}

// On subscribes to a page event for which the given handler will be executed
// passing in the ConsoleMessage associated with the event.
// The only accepted event value is 'console'.
Expand Down
8 changes: 7 additions & 1 deletion tests/browser_context_options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,13 @@ func TestBrowserContextOptionsExtraHTTPHeaders(t *testing.T) {
require.NoError(t, err)

err = tb.awaitWithTimeout(time.Second*5, func() error {
resp, err := p.Goto(tb.url("/get"), nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
Comment on lines +89 to +91
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

page.Goto could implicitly set a default timeout. But I think this makes it explicit and less error-prone. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I was wondering the same. I went with the option of being explicit for now, and ensuring it matched the current behaviour. I think in a future refactor we can make it implicit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the side of keeping it explicit this way. This also gives transparent direct control to tests. Previously, we had our fair share of bugs because of time management and timeouts. Let's see :)

resp, err := p.Goto(
tb.url("/get"),
opts,
)
if err != nil {
return err
}
Expand Down
33 changes: 26 additions & 7 deletions tests/browser_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,13 @@ func TestBrowserContextCookies(t *testing.T) {
// the setupHandler can set some cookies
// that will be received by the browser context.
tb.withHandler("/empty", tt.setupHandler)
_, err := p.Goto(tb.url("/empty"), nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(
tb.url("/empty"),
opts,
)
require.NoErrorf(t,
err, "failed to open an empty page",
)
Expand Down Expand Up @@ -626,7 +632,13 @@ func TestK6Object(t *testing.T) {
p := b.NewPage(nil)

url := b.staticURL("empty.html")
r, err := p.Goto(url, nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
r, err := p.Goto(
url,
opts,
)
require.NoError(t, err)
require.NotNil(t, r)

Expand Down Expand Up @@ -674,17 +686,24 @@ func TestBrowserContextTimeout(t *testing.T) {
bc, err := tb.NewContext(nil)
require.NoError(t, err)

p, err := bc.NewPage()
require.NoError(t, err)

var timeout time.Duration
if tc.defaultTimeout != 0 {
timeout = tc.defaultTimeout
bc.SetDefaultTimeout(tc.defaultTimeout.Milliseconds())
}
if tc.defaultNavigationTimeout != 0 {
timeout = tc.defaultNavigationTimeout
bc.SetDefaultNavigationTimeout(tc.defaultNavigationTimeout.Milliseconds())
}

p, err := bc.NewPage()
require.NoError(t, err)

res, err := p.Goto(tb.url("/slow"), nil)
res, err := p.Goto(
tb.url("/slow"),
&common.FrameGotoOptions{
Timeout: timeout,
},
)
require.Nil(t, res)
assert.ErrorContains(t, err, "timed out after")
})
Expand Down
16 changes: 14 additions & 2 deletions tests/element_handle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,13 @@ func TestElementHandleClickConcealedLink(t *testing.T) {
cr := p.Evaluate(tb.toGojaValue(cmd))
return tb.asGojaValue(cr).String()
}
resp, err := p.Goto(tb.staticURL("/concealed_link.html"), nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
resp, err := p.Goto(
tb.staticURL("/concealed_link.html"),
opts,
)
require.NotNil(t, resp)
require.NoError(t, err)
require.Equal(t, wantBefore, clickResult())
Expand All @@ -214,7 +220,13 @@ func TestElementHandleNonClickable(t *testing.T) {
p, err := bctx.NewPage()
require.NoError(t, err)

resp, err := p.Goto(tb.staticURL("/non_clickable.html"), nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
resp, err := p.Goto(
tb.staticURL("/non_clickable.html"),
opts,
)
require.NotNil(t, resp)
require.NoError(t, err)

Expand Down
10 changes: 5 additions & 5 deletions tests/frame_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) {
tb := newTestBrowser(t, withFileServer())
p := tb.NewPage(nil)

opts := tb.toGojaValue(&common.FrameGotoOptions{
opts := &common.FrameGotoOptions{
WaitUntil: common.LifecycleEventNetworkIdle,
Timeout: time.Duration(timeout.Milliseconds()), // interpreted as ms
})
Timeout: timeout,
}
resp, err := p.Goto(tb.staticURL("/nav_in_doc.html"), opts)
require.NoError(t, err)
require.NotNil(t, resp)
Expand Down Expand Up @@ -90,10 +90,10 @@ func TestWaitForFrameNavigation(t *testing.T) {
`)
})

opts := tb.toGojaValue(&common.FrameGotoOptions{
opts := &common.FrameGotoOptions{
WaitUntil: common.LifecycleEventNetworkIdle,
Timeout: common.DefaultTimeout,
})
}
_, err := p.Goto(tb.url("/first"), opts)
require.NoError(t, err)

Expand Down
35 changes: 17 additions & 18 deletions tests/frame_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,11 @@ func TestFrameDismissDialogBox(t *testing.T) {
p = tb.NewPage(nil)
)

opts := tb.toGojaValue(struct {
WaitUntil string `js:"waitUntil"`
}{
WaitUntil: "networkidle",
})
_, err := p.Goto(
tb.staticURL("dialog.html?dialogType="+tt),
opts,
)
opts := &common.FrameGotoOptions{
WaitUntil: common.LifecycleEventNetworkIdle,
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(tb.staticURL("dialog.html?dialogType="+tt), opts)
require.NoError(t, err)

if tt == "beforeunload" {
Expand Down Expand Up @@ -91,14 +87,11 @@ func TestFrameNoPanicWithEmbeddedIFrame(t *testing.T) {
)

p := tb.NewPage(nil)
_, err := p.Goto(
tb.staticURL("embedded_iframe.html"),
tb.toGojaValue(struct {
WaitUntil string `js:"waitUntil"`
}{
WaitUntil: "load",
}),
)
opts := &common.FrameGotoOptions{
WaitUntil: common.LifecycleEventDOMContentLoad,
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(tb.staticURL("embedded_iframe.html"), opts)
require.NoError(t, err)

result := p.TextContent("#doneDiv", nil)
Expand Down Expand Up @@ -129,7 +122,13 @@ func TestFrameNoPanicNavigateAndClickOnPageWithIFrames(t *testing.T) {
http.Redirect(w, r, tb.staticURL("iframe_signin.html"), http.StatusMovedPermanently)
})

_, err := p.Goto(tb.staticURL("iframe_home.html"), nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(
tb.staticURL("iframe_home.html"),
opts,
)
require.NoError(t, err)

ctx, cancel := context.WithTimeout(tb.context(), 5*time.Second)
Expand Down
16 changes: 14 additions & 2 deletions tests/launch_options_slowmo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,13 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
_, err := p.Goto(common.BlankPage, nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(
common.BlankPage,
opts,
)
require.NoError(t, err)
})
})
Expand Down Expand Up @@ -222,7 +228,13 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testFrameSlowMoImpl(t, tb, func(_ *testBrowser, f *common.Frame) {
_, _ = f.Goto(common.BlankPage, nil)
opts := &common.FrameGotoOptions{
Timeout: common.DefaultTimeout,
}
_, _ = f.Goto(
common.BlankPage,
opts,
)
})
})
t.Run("hover", func(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions tests/lifecycle_wait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -711,10 +711,10 @@ func assertHome(

var resolved, rejected bool
err := func() error {
opts := tb.toGojaValue(common.FrameGotoOptions{
opts := &common.FrameGotoOptions{
WaitUntil: waitUntil,
Timeout: 30 * time.Second,
})
Timeout: common.DefaultTimeout,
}
_, err := p.Goto(tb.url("/home"), opts)
if err == nil {
resolved = true
Expand Down