From 3f26d5cd37bac7a306a99e3e730ee093a7e8f12f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Fri, 21 Jun 2024 14:29:59 +0300 Subject: [PATCH] Desobekify Browser - Moves BrowserContextOptions out of the business logic. - Sets default options in NewBrowserContext to prevent nil pointer issues in the rest of the module. Later, we might think about turning some of these pointer fields into value types to take advantage of their zero values. --- browser/browser_mapping.go | 24 +++++++++++++---------- browser/mapping_test.go | 4 ++-- browser/sync_browser_mapping.go | 18 +++++++++++++++-- common/browser.go | 14 +++----------- common/browser_context.go | 7 ++++++- tests/browser_context_options_test.go | 28 +++++++++++++-------------- tests/element_handle_test.go | 18 ++++++++--------- tests/network_manager_test.go | 19 +++++++++--------- tests/test_browser.go | 2 +- 9 files changed, 72 insertions(+), 62 deletions(-) diff --git a/browser/browser_mapping.go b/browser/browser_mapping.go index bfab5190b..19569c09c 100644 --- a/browser/browser_mapping.go +++ b/browser/browser_mapping.go @@ -10,7 +10,7 @@ import ( ) // mapBrowser to the JS module. -func mapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop +func mapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop,gocognit return mapping{ "context": func() (mapping, error) { b, err := vu.browser() @@ -36,23 +36,24 @@ func mapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop return b.IsConnected(), nil }, "newContext": func(opts sobek.Value) (*sobek.Promise, error) { + popts := common.NewBrowserContextOptions() + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing browser.newContext options: %w", err) + } return k6ext.Promise(vu.Context(), func() (any, error) { b, err := vu.browser() if err != nil { return nil, err } - bctx, err := b.NewContext(opts) + bctx, err := b.NewContext(popts) if err != nil { return nil, err //nolint:wrapcheck } - if err := initBrowserContext(bctx, vu.testRunID); err != nil { return nil, err } - m := mapBrowserContext(vu, bctx) - - return m, nil + return mapBrowserContext(vu, bctx), nil }), nil }, "userAgent": func() (string, error) { @@ -69,23 +70,26 @@ func mapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop } return b.Version(), nil }, - "newPage": func(opts sobek.Value) *sobek.Promise { + "newPage": func(opts sobek.Value) (*sobek.Promise, error) { + popts := common.NewBrowserContextOptions() + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing browser.newPage options: %w", err) + } return k6ext.Promise(vu.Context(), func() (any, error) { b, err := vu.browser() if err != nil { return nil, err } - page, err := b.NewPage(opts) + page, err := b.NewPage(popts) if err != nil { return nil, err //nolint:wrapcheck } - if err := initBrowserContext(b.Context(), vu.testRunID); err != nil { return nil, err } return mapPage(vu, page), nil - }) + }), nil }, } } diff --git a/browser/mapping_test.go b/browser/mapping_test.go index ccea1f55c..87d7cce90 100644 --- a/browser/mapping_test.go +++ b/browser/mapping_test.go @@ -264,8 +264,8 @@ type browserAPI interface { Context() *common.BrowserContext CloseContext() IsConnected() bool - NewContext(opts sobek.Value) (*common.BrowserContext, error) - NewPage(opts sobek.Value) (*common.Page, error) + NewContext(opts *common.BrowserContextOptions) (*common.BrowserContext, error) + NewPage(opts *common.BrowserContextOptions) (*common.Page, error) On(string) (bool, error) UserAgent() string Version() string diff --git a/browser/sync_browser_mapping.go b/browser/sync_browser_mapping.go index 400e31ca6..689e610a1 100644 --- a/browser/sync_browser_mapping.go +++ b/browser/sync_browser_mapping.go @@ -1,7 +1,11 @@ package browser import ( + "fmt" + "github.com/grafana/sobek" + + "github.com/grafana/xk6-browser/common" ) // syncMapBrowser is like mapBrowser but returns synchronous functions. @@ -30,11 +34,16 @@ func syncMapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop return b.IsConnected(), nil }, "newContext": func(opts sobek.Value) (*sobek.Object, error) { + popts := common.NewBrowserContextOptions() + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing browser.newContext options: %w", err) + } + b, err := vu.browser() if err != nil { return nil, err } - bctx, err := b.NewContext(opts) + bctx, err := b.NewContext(popts) if err != nil { return nil, err //nolint:wrapcheck } @@ -62,11 +71,16 @@ func syncMapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop return b.Version(), nil }, "newPage": func(opts sobek.Value) (mapping, error) { + popts := common.NewBrowserContextOptions() + if err := popts.Parse(vu.Context(), opts); err != nil { + return nil, fmt.Errorf("parsing browser.newContext options: %w", err) + } + b, err := vu.browser() if err != nil { return nil, err } - page, err := b.NewPage(opts) + page, err := b.NewPage(popts) if err != nil { return nil, err //nolint:wrapcheck } diff --git a/common/browser.go b/common/browser.go index c1b9bc98c..c8e617c0c 100644 --- a/common/browser.go +++ b/common/browser.go @@ -14,7 +14,6 @@ import ( "github.com/chromedp/cdproto/cdp" "github.com/chromedp/cdproto/target" "github.com/gorilla/websocket" - "github.com/grafana/sobek" "github.com/grafana/xk6-browser/k6ext" "github.com/grafana/xk6-browser/log" @@ -569,7 +568,7 @@ func (b *Browser) IsConnected() bool { } // NewContext creates a new incognito-like browser context. -func (b *Browser) NewContext(opts sobek.Value) (*BrowserContext, error) { +func (b *Browser) NewContext(opts *BrowserContextOptions) (*BrowserContext, error) { _, span := TraceAPICall(b.ctx, "", "browser.newContext") defer span.End() @@ -588,14 +587,7 @@ func (b *Browser) NewContext(opts sobek.Value) (*BrowserContext, error) { return nil, err } - browserCtxOpts := NewBrowserContextOptions() - if err := browserCtxOpts.Parse(b.ctx, opts); err != nil { - err := fmt.Errorf("parsing newContext options: %w", err) - spanRecordError(span, err) - return nil, err - } - - browserCtx, err := NewBrowserContext(b.ctx, b, browserContextID, browserCtxOpts, b.logger) + browserCtx, err := NewBrowserContext(b.ctx, b, browserContextID, opts, b.logger) if err != nil { err := fmt.Errorf("new context: %w", err) spanRecordError(span, err) @@ -610,7 +602,7 @@ func (b *Browser) NewContext(opts sobek.Value) (*BrowserContext, error) { } // NewPage creates a new tab in the browser window. -func (b *Browser) NewPage(opts sobek.Value) (*Page, error) { +func (b *Browser) NewPage(opts *BrowserContextOptions) (*Page, error) { _, span := TraceAPICall(b.ctx, "", "browser.newPage") defer span.End() diff --git a/common/browser_context.go b/common/browser_context.go index 30a10a89a..b2a0ef8ab 100644 --- a/common/browser_context.go +++ b/common/browser_context.go @@ -90,6 +90,11 @@ type BrowserContext struct { func NewBrowserContext( ctx context.Context, browser *Browser, id cdp.BrowserContextID, opts *BrowserContextOptions, logger *log.Logger, ) (*BrowserContext, error) { + // set the default options if none provided. + if opts == nil { + opts = NewBrowserContextOptions() + } + b := BrowserContext{ BaseEventEmitter: NewBaseEventEmitter(ctx), ctx: ctx, @@ -101,7 +106,7 @@ func NewBrowserContext( timeoutSettings: NewTimeoutSettings(nil), } - if opts != nil && len(opts.Permissions) > 0 { + if len(opts.Permissions) > 0 { err := b.GrantPermissions(opts.Permissions, NewGrantPermissionsOptions()) if err != nil { return nil, err diff --git a/tests/browser_context_options_test.go b/tests/browser_context_options_test.go index 4741bb547..c74d5ac1d 100644 --- a/tests/browser_context_options_test.go +++ b/tests/browser_context_options_test.go @@ -51,14 +51,12 @@ func TestBrowserContextOptionsSetViewport(t *testing.T) { t.Parallel() tb := newTestBrowser(t) - bctx, err := tb.NewContext(tb.toSobekValue(struct { - Viewport common.Viewport `js:"viewport"` - }{ - Viewport: common.Viewport{ - Width: 800, - Height: 600, - }, - })) + opts := common.NewBrowserContextOptions() + opts.Viewport = &common.Viewport{ + Width: 800, + Height: 600, + } + bctx, err := tb.NewContext(opts) require.NoError(t, err) t.Cleanup(func() { if err := bctx.Close(); err != nil { @@ -77,19 +75,19 @@ func TestBrowserContextOptionsExtraHTTPHeaders(t *testing.T) { t.Parallel() tb := newTestBrowser(t, withHTTPServer()) - bctx, err := tb.NewContext(tb.toSobekValue(struct { - ExtraHTTPHeaders map[string]string `js:"extraHTTPHeaders"` - }{ - ExtraHTTPHeaders: map[string]string{ - "Some-Header": "Some-Value", - }, - })) + + opts := common.NewBrowserContextOptions() + opts.ExtraHTTPHeaders = map[string]string{ + "Some-Header": "Some-Value", + } + bctx, err := tb.NewContext(opts) require.NoError(t, err) t.Cleanup(func() { if err := bctx.Close(); err != nil { t.Log("closing browser context:", err) } }) + p, err := bctx.NewPage() require.NoError(t, err) diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index 89b966363..da6803fa5 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -181,17 +181,15 @@ func TestElementHandleClickConcealedLink(t *testing.T) { ) tb := newTestBrowser(t, withFileServer()) - bc, err := tb.NewContext( - tb.toSobekValue(struct { - Viewport common.Viewport `js:"viewport"` - }{ - Viewport: common.Viewport{ - Width: 500, - Height: 240, - }, - }), - ) + + bcopts := common.NewBrowserContextOptions() + bcopts.Viewport = &common.Viewport{ + Width: 500, + Height: 240, + } + bc, err := tb.NewContext(bcopts) require.NoError(t, err) + p, err := bc.NewPage() require.NoError(t, err) diff --git a/tests/network_manager_test.go b/tests/network_manager_test.go index 7416c1e90..0fb003154 100644 --- a/tests/network_manager_test.go +++ b/tests/network_manager_test.go @@ -112,21 +112,20 @@ func TestBasicAuth(t *testing.T) { tb.Helper() browser := newTestBrowser(t, withHTTPServer()) - bc, err := browser.NewContext( - browser.toSobekValue(struct { - HttpCredentials *common.Credentials `js:"httpCredentials"` //nolint:revive - }{ - HttpCredentials: &common.Credentials{ - Username: user, - Password: pass, - }, - })) + + bcopts := common.NewBrowserContextOptions() + bcopts.HttpCredentials = &common.Credentials{ + Username: validUser, + Password: validPassword, + } + bc, err := browser.NewContext(bcopts) require.NoError(t, err) + p, err := bc.NewPage() require.NoError(t, err) url := browser.url( - fmt.Sprintf("/basic-auth/%s/%s", validUser, validPassword), + fmt.Sprintf("/basic-auth/%s/%s", user, pass), ) opts := &common.FrameGotoOptions{ WaitUntil: common.LifecycleEventLoad, diff --git a/tests/test_browser.go b/tests/test_browser.go index d405d3665..767add7bf 100644 --- a/tests/test_browser.go +++ b/tests/test_browser.go @@ -253,7 +253,7 @@ func withSkipClose() func(*testBrowser) { // NewPage is a wrapper around Browser.NewPage that fails the test if an // error occurs. Added this helper to avoid boilerplate code in tests. -func (b *testBrowser) NewPage(opts sobek.Value) *common.Page { +func (b *testBrowser) NewPage(opts *common.BrowserContextOptions) *common.Page { b.t.Helper() p, err := b.Browser.NewPage(opts)