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

Turn Browser panics into errors #1360

Merged
merged 3 commits into from
May 30, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions browser/browser_mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ func mapBrowser(vu moduleVU) mapping { //nolint:funlen,cyclop
if err != nil {
return "", err
}
return b.UserAgent(), nil
return b.UserAgent() //nolint:wrapcheck
},
"version": func() (string, error) {
b, err := vu.browser()
if err != nil {
return "", err
}
return b.Version(), nil
return b.Version() //nolint:wrapcheck
},
"newPage": func(opts goja.Value) (mapping, error) {
b, err := vu.browser()
Expand Down
4 changes: 2 additions & 2 deletions browser/mapping_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ type browserAPI interface {
NewContext(opts goja.Value) (*common.BrowserContext, error)
NewPage(opts goja.Value) (*common.Page, error)
On(string) (bool, error)
UserAgent() string
Version() string
UserAgent() (string, error)
Version() (string, error)
}

// browserContextAPI is the public interface of a CDP browser context.
Expand Down
38 changes: 22 additions & 16 deletions common/browser.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (b *Browser) getPages() []*Page {
return pages
}

func (b *Browser) initEvents() error {
func (b *Browser) initEvents() error { //nolint:cyclop
var cancelCtx context.Context
cancelCtx, b.evCancelFn = context.WithCancel(b.ctx)
chHandler := make(chan Event)
Expand All @@ -198,7 +198,9 @@ func (b *Browser) initEvents() error {
case event := <-chHandler:
if ev, ok := event.data.(*target.EventAttachedToTarget); ok {
b.logger.Debugf("Browser:initEvents:onAttachedToTarget", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
b.onAttachedToTarget(ev)
if err := b.onAttachedToTarget(ev); err != nil {
k6ext.Panic(b.ctx, "browser is attaching to target: %w", err)
}
} else if ev, ok := event.data.(*target.EventDetachedFromTarget); ok {
b.logger.Debugf("Browser:initEvents:onDetachedFromTarget", "sid:%v", ev.SessionID)
b.onDetachedFromTarget(ev)
Expand Down Expand Up @@ -247,7 +249,7 @@ func (b *Browser) connectionOnAttachedToTarget(eva *target.EventAttachedToTarget
}

// onAttachedToTarget is called when a new page is attached to the browser.
func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) error {
b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v",
ev.SessionID, ev.TargetInfo.TargetID, ev.TargetInfo.BrowserContextID)

Expand All @@ -257,14 +259,14 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
)

if !b.isAttachedPageValid(ev, browserCtx) {
return // Ignore this page.
return nil // Ignore this page.
}
session := b.conn.getSession(ev.SessionID)
if session == nil {
b.logger.Debugf("Browser:onAttachedToTarget",
"session closed before attachToTarget is handled. sid:%v tid:%v",
ev.SessionID, targetPage.TargetID)
return // ignore
return nil // ignore
}

var (
Expand All @@ -281,17 +283,21 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
}
p, err := NewPage(b.ctx, session, browserCtx, targetPage.TargetID, opener, isPage, b.logger)
if err != nil && b.isPageAttachmentErrorIgnorable(ev, session, err) {
return // Ignore this page.
return nil // Ignore this page.
}
if err != nil {
k6ext.Panic(b.ctx, "creating a new %s: %w", targetPage.Type, err)
return fmt.Errorf("creating a new %s: %w", targetPage.Type, err)
}

b.attachNewPage(p, ev) // Register the page as an active page.

// Emit the page event only for pages, not for background pages.
// Background pages are created by extensions.
if isPage {
browserCtx.emit(EventBrowserContextPage, p)
}

return nil
}

// attachNewPage registers the page as an active page and attaches the sessionID with the targetID.
Expand Down Expand Up @@ -622,27 +628,27 @@ func (b *Browser) On(event string) (bool, error) {
}

// UserAgent returns the controlled browser's user agent string.
func (b *Browser) UserAgent() string {
func (b *Browser) UserAgent() (string, error) {
action := cdpbrowser.GetVersion()
_, _, _, ua, _, err := action.Do(cdp.WithExecutor(b.ctx, b.conn))
_, _, _, ua, _, err := action.Do(cdp.WithExecutor(b.ctx, b.conn)) //nolint:dogsled
if err != nil {
k6ext.Panic(b.ctx, "getting browser user agent: %w", err)
return "", fmt.Errorf("getting browser user agent: %w", err)
}
return ua
return ua, nil
}

// Version returns the controlled browser's version.
func (b *Browser) Version() string {
func (b *Browser) Version() (string, error) {
action := cdpbrowser.GetVersion()
_, product, _, _, _, err := action.Do(cdp.WithExecutor(b.ctx, b.conn))
_, product, _, _, _, err := action.Do(cdp.WithExecutor(b.ctx, b.conn)) //nolint:dogsled
if err != nil {
k6ext.Panic(b.ctx, "getting browser version: %w", err)
return "", fmt.Errorf("getting browser version: %w", err)
}
i := strings.Index(product, "/")
if i == -1 {
return product
return product, nil
}
return product[i+1:]
return product[i+1:], nil
}

// WsURL returns the Websocket URL that the browser is listening on for CDP clients.
Expand Down
9 changes: 6 additions & 3 deletions tests/browser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,10 @@ func TestBrowserVersion(t *testing.T) {
t.Parallel()

const re = `^\d+\.\d+\.\d+\.\d+$`
r, _ := regexp.Compile(re)
ver := newTestBrowser(t).Version()
r, err := regexp.Compile(re) //nolint:gocritic
inancgumus marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)
ver, err := newTestBrowser(t).Version()
require.NoError(t, err)
assert.Regexp(t, r, ver, "expected browser version to match regex %q, but found %q", re, ver)
}

Expand All @@ -210,7 +212,8 @@ func TestBrowserUserAgent(t *testing.T) {

// testBrowserVersion() tests the version already
// just look for "Headless" in UserAgent
ua := b.UserAgent()
ua, err := b.UserAgent()
require.NoError(t, err)
if prefix := "Mozilla/5.0"; !strings.HasPrefix(ua, prefix) {
t.Errorf("UserAgent should start with %q, but got: %q", prefix, ua)
}
Expand Down
Loading