Skip to content

Commit

Permalink
Refactor parse our of page.click
Browse files Browse the repository at this point in the history
This helps avoid using the goja runtime when it comes to parsing the
options. The options parsing has been moved to the mapping layer and so
has been moved back on to the main goja thread, which will mitigate the
possibility of a panic which could occur when multiple goroutines try
to work with the goja runtime (which is not thread safe).

There is another issue to tackle more of the goja refactoring in
  • Loading branch information
ankur22 committed Jan 19, 2024
1 parent 4ceb3be commit 7875f68
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 22 deletions.
11 changes: 8 additions & 3 deletions browser/mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,16 @@ func mapPage(vu moduleVU, p *common.Page) mapping {
"addStyleTag": p.AddStyleTag,
"bringToFront": p.BringToFront,
"check": p.Check,
"click": func(selector string, opts goja.Value) *goja.Promise {
"click": func(selector string, opts goja.Value) (*goja.Promise, error) {
popts, err := parseFrameClickOptions(vu.Context(), opts, p.Timeout())
if err != nil {
return nil, err
}

return k6ext.Promise(vu.Context(), func() (any, error) {
err := p.Click(selector, opts)
err := p.Click(selector, popts)
return nil, err //nolint:wrapcheck
})
}), nil
},
"close": func(opts goja.Value) error {
vu.taskQueueRegistry.close(p.TargetID())
Expand Down
9 changes: 2 additions & 7 deletions common/page.go
Original file line number Diff line number Diff line change
Expand Up @@ -649,15 +649,10 @@ func (p *Page) IsChecked(selector string, opts goja.Value) bool {
}

// Click clicks an element matching provided selector.
func (p *Page) Click(selector string, opts goja.Value) error {
func (p *Page) Click(selector string, opts *FrameClickOptions) error {
p.logger.Debugf("Page:Click", "sid:%v selector:%s", p.sessionID(), selector)

popts := NewFrameClickOptions(p.defaultTimeout())
if err := popts.Parse(p.ctx, opts); err != nil {
k6ext.Panic(p.ctx, "parsing click options %q: %w", selector, err)
}

return p.MainFrame().Click(selector, popts)
return p.MainFrame().Click(selector, opts)
}

// Close closes the page.
Expand Down
4 changes: 2 additions & 2 deletions tests/element_handle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ func TestElementHandleClickConcealedLink(t *testing.T) {
require.NoError(t, err)
require.Equal(t, wantBefore, clickResult())

err = p.Click("#concealed", nil)
err = p.Click("#concealed", common.NewFrameClickOptions(p.Timeout()))
require.NoError(t, err)
require.Equal(t, wantAfter, clickResult())
}
Expand All @@ -224,7 +224,7 @@ func TestElementHandleNonClickable(t *testing.T) {
require.NotNil(t, resp)
require.NoError(t, err)

err = p.Click("#non-clickable", nil)
err = p.Click("#non-clickable", common.NewFrameClickOptions(p.Timeout()))
require.Errorf(t, err, "element should not be clickable")
}

Expand Down
4 changes: 2 additions & 2 deletions tests/frame_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) {
return err
}
click := func() error {
return p.Click(tc.selector, nil)
return p.Click(tc.selector, common.NewFrameClickOptions(p.Timeout()))
}
ctx, cancel := context.WithTimeout(tb.ctx, timeout)
defer cancel()
Expand Down Expand Up @@ -105,7 +105,7 @@ func TestWaitForFrameNavigation(t *testing.T) {
return err
}
click := func() error {
return p.Click(`a`, nil)
return p.Click(`a`, common.NewFrameClickOptions(p.Timeout()))
}
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
Expand Down
5 changes: 3 additions & 2 deletions tests/frame_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/grafana/xk6-browser/common"
"github.com/grafana/xk6-browser/env"
)

Expand Down Expand Up @@ -60,7 +61,7 @@ func TestFrameDismissDialogBox(t *testing.T) {
require.NoError(t, err)

if tt == "beforeunload" {
err = p.Click("#clickHere", nil)
err = p.Click("#clickHere", common.NewFrameClickOptions(p.Timeout()))
require.NoError(t, err)
}

Expand Down Expand Up @@ -136,7 +137,7 @@ func TestFrameNoPanicNavigateAndClickOnPageWithIFrames(t *testing.T) {

err = tb.run(
ctx,
func() error { return p.Click(`a[href="/iframeSignIn"]`, nil) },
func() error { return p.Click(`a[href="/iframeSignIn"]`, common.NewFrameClickOptions(p.Timeout())) },
func() error { _, err := p.WaitForNavigation(nil); return err },
)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion tests/launch_options_slowmo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
err := p.Click("button", nil)
err := p.Click("button", common.NewFrameClickOptions(p.Timeout()))
assert.NoError(t, err)
})
})
Expand Down
2 changes: 1 addition & 1 deletion tests/lifecycle_wait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func TestLifecycleWaitForNavigation(t *testing.T) {
return err
}
click := func() error {
return p.Click(`a`, nil)
return p.Click(`a`, common.NewFrameClickOptions(p.Timeout()))
}

ctx, cancel := context.WithTimeout(tb.ctx, 5*time.Second)
Expand Down
4 changes: 1 addition & 3 deletions tests/locator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,11 +474,9 @@ func TestLocatorShadowDOM(t *testing.T) {

tb := newTestBrowser(t, withFileServer())
p := tb.NewPage(nil)
opts := tb.toGojaValue(jsFrameBaseOpts{Timeout: "1000"})

_, err := p.Goto(tb.staticURL("shadow_dom_link.html"), nil)
require.NoError(t, err)

err = p.Click("#inner-link", opts)
err = p.Click("#inner-link", common.NewFrameClickOptions(time.Second))
require.NoError(t, err)
}
3 changes: 2 additions & 1 deletion tests/webvital_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"
"time"

"github.com/grafana/xk6-browser/common"

Check failure on line 8 in tests/webvital_test.go

View workflow job for this annotation

GitHub Actions / lint

File is not `gci`-ed with --skip-generated -s standard,prefix(github.com/grafana/xk6-browser),prefix(go.k6.io),default (gci)
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -61,7 +62,7 @@ func TestWebVitalMetric(t *testing.T) {
// also helps the web vital library to measure CLS.
err = browser.run(
ctx,
func() error { return page.Click("#clickMe", nil) },
func() error { return page.Click("#clickMe", common.NewFrameClickOptions(page.Timeout())) },
func() error { _, err := page.WaitForNavigation(nil); return err },
)
require.NoError(t, err)
Expand Down

0 comments on commit 7875f68

Please sign in to comment.