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.

Sign up for GitHub

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

browser: fix possible races #4528

Merged
merged 6 commits into from
Feb 10, 2025
Merged

browser: fix possible races #4528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3eb8e5
browser: partially get rid of sobek.Values usages in selectOption
olegbespalov Feb 10, 2025
f868a5f
browser: get rid of sobek.Values usages in SetChecked
olegbespalov Feb 10, 2025
631853c
browser: partially get rid of sobek.Values usages in SetInputFiles
olegbespalov Feb 10, 2025
036442f
browser: get rid of sobek.Values usages in TextContent
olegbespalov Feb 10, 2025
743d2f3
chore: omit cyclop in mapFrame
olegbespalov Feb 10, 2025
5a802dd
chore: omit nilnil in textContent
olegbespalov Feb 10, 2025
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
50 changes: 36 additions & 14 deletions internal/js/modules/k6/browser/browser/frame_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

// mapFrame to the JS module.
//
//nolint:funlen,gocognit
//nolint:funlen,gocognit,cyclop
func mapFrame(vu moduleVU, f *common.Frame) mapping {
maps := mapping{
"check": func(selector string, opts sobek.Value) *sobek.Promise {
Expand Down Expand Up @@ -193,25 +193,42 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
return nil, f.Press(selector, key, opts) //nolint:wrapcheck
})
},
"selectOption": func(selector string, values sobek.Value, opts sobek.Value) *sobek.Promise {
"selectOption": func(selector string, values sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSelectOptionOptions(f.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing select option options: %w", err)
}

// TODO: don't use sobek Values in a separate goroutine: finish migrating values
return k6ext.Promise(vu.Context(), func() (any, error) {
return f.SelectOption(selector, values, opts) //nolint:wrapcheck
})
return f.SelectOption(selector, values, popts) //nolint:wrapcheck
}), nil
},
"setChecked": func(selector string, checked bool, opts sobek.Value) *sobek.Promise {
"setChecked": func(selector string, checked bool, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameCheckOptions(f.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing frame set check options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
return nil, f.SetChecked(selector, checked, opts) //nolint:wrapcheck
})
return nil, f.SetChecked(selector, checked, popts) //nolint:wrapcheck
}), nil
},
"setContent": func(html string, opts sobek.Value) *sobek.Promise {
return k6ext.Promise(vu.Context(), func() (any, error) {
return nil, f.SetContent(html, opts) //nolint:wrapcheck
})
},
"setInputFiles": func(selector string, files sobek.Value, opts sobek.Value) *sobek.Promise {
"setInputFiles": func(selector string, files sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSetInputFilesOptions(f.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing setInputFiles options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
return nil, f.SetInputFiles(selector, files, opts) //nolint:wrapcheck
})
// TODO: don't use sobek Values in a separate goroutine, complete files migration
return nil, f.SetInputFiles(selector, files, popts) //nolint:wrapcheck
}), nil
},
"tap": func(selector string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameTapOptions(f.Timeout())
Expand All @@ -222,17 +239,22 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping {
return nil, f.Tap(selector, popts) //nolint:wrapcheck
}), nil
},
"textContent": func(selector string, opts sobek.Value) *sobek.Promise {
"textContent": func(selector string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameTextContentOptions(f.Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing text content options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
s, ok, err := f.TextContent(selector, opts)
s, ok, err := f.TextContent(selector, popts)
if err != nil {
return nil, err //nolint:wrapcheck
}
if !ok {
return nil, nil
return nil, nil //nolint:nilnil
}
return s, nil
})
}), nil
},
"title": func() *sobek.Promise {
return k6ext.Promise(vu.Context(), func() (any, error) {
Expand Down
52 changes: 35 additions & 17 deletions internal/js/modules/k6/browser/browser/page_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,17 +276,26 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
return &ab, nil
}), nil
},
"selectOption": func(selector string, values sobek.Value, opts sobek.Value) *sobek.Promise {
"selectOption": func(selector string, values sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSelectOptionOptions(p.MainFrame().Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing select option options: %w", err)
}

// TODO: don't use sobek Values in a separate goroutine: finish migrating values
return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
return p.SelectOption(selector, values, opts) //nolint:wrapcheck
})
return p.SelectOption(selector, values, popts) //nolint:wrapcheck
}), nil
},
"setChecked": func(selector string, checked bool, opts sobek.Value) *sobek.Promise {
"setChecked": func(selector string, checked bool, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameCheckOptions(p.MainFrame().Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing frame set check options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
return nil, p.SetChecked(selector, checked, opts) //nolint:wrapcheck
})
return nil, p.SetChecked(selector, checked, popts) //nolint:wrapcheck
}), nil
},
"setContent": func(html string, opts sobek.Value) *sobek.Promise {
return k6ext.Promise(vu.Context(), func() (any, error) {
Expand All @@ -301,11 +310,16 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
return nil, p.SetExtraHTTPHeaders(headers) //nolint:wrapcheck
})
},
"setInputFiles": func(selector string, files sobek.Value, opts sobek.Value) *sobek.Promise {
"setInputFiles": func(selector string, files sobek.Value, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameSetInputFilesOptions(p.MainFrame().Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing setInputFiles options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
return nil, p.SetInputFiles(selector, files, opts) //nolint:wrapcheck
})
// TODO: don't use sobek Values in a separate goroutine, complete files migration
return nil, p.SetInputFiles(selector, files, popts) //nolint:wrapcheck
}), nil
},
"setViewportSize": func(viewportSize sobek.Value) *sobek.Promise {
return k6ext.Promise(vu.Context(), func() (any, error) {
Expand All @@ -321,18 +335,22 @@ func mapPage(vu moduleVU, p *common.Page) mapping { //nolint:gocognit,cyclop
return nil, p.Tap(selector, popts) //nolint:wrapcheck
}), nil
},
"textContent": func(selector string, opts sobek.Value) *sobek.Promise {
"textContent": func(selector string, opts sobek.Value) (*sobek.Promise, error) {
popts := common.NewFrameTextContentOptions(p.MainFrame().Timeout())
if err := popts.Parse(vu.Context(), opts); err != nil {
return nil, fmt.Errorf("parsing text content options: %w", err)
}

return k6ext.Promise(vu.Context(), func() (any, error) {
// TODO(@mstoykov): don't use sobek Values in a separate goroutine
s, ok, err := p.TextContent(selector, opts)
s, ok, err := p.TextContent(selector, popts)
if err != nil {
return nil, err //nolint:wrapcheck
}
if !ok {
return nil, nil
return nil, nil //nolint:nilnil
}
return s, nil
})
}), nil
},
"throttleCPU": func(cpuProfile common.CPUProfile) *sobek.Promise {
return k6ext.Promise(vu.Context(), func() (any, error) {
Expand Down
25 changes: 4 additions & 21 deletions internal/js/modules/k6/browser/common/frame.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,13 +637,9 @@ func (f *Frame) setChecked(selector string, checked bool, opts *FrameCheckOption
}

// SetChecked sets the checked state of the first element found that matches the selector.
func (f *Frame) SetChecked(selector string, checked bool, opts sobek.Value) error {
func (f *Frame) SetChecked(selector string, checked bool, popts *FrameCheckOptions) error {
f.log.Debugf("Frame:SetChecked", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)

popts := NewFrameCheckOptions(f.defaultTimeout())
if err := popts.Parse(f.ctx, opts); err != nil {
return fmt.Errorf("parsing frame set check options: %w", err)
}
if err := f.setChecked(selector, checked, popts); err != nil {
return fmt.Errorf("setting checked %q: %w", selector, err)
}
Expand Down Expand Up @@ -1488,13 +1484,9 @@ func (f *Frame) press(selector, key string, opts *FramePressOptions) error {

// SelectOption selects the given options and returns the array of
// option values of the first element found that matches the selector.
func (f *Frame) SelectOption(selector string, values sobek.Value, opts sobek.Value) ([]string, error) {
func (f *Frame) SelectOption(selector string, values sobek.Value, popts *FrameSelectOptionOptions) ([]string, error) {
f.log.Debugf("Frame:SelectOption", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)

popts := NewFrameSelectOptionOptions(f.defaultTimeout())
if err := popts.Parse(f.ctx, opts); err != nil {
return nil, fmt.Errorf("parsing select option options: %w", err)
}
v, err := f.selectOption(selector, values, popts)
if err != nil {
return nil, fmt.Errorf("selecting option on %q: %w", selector, err)
Expand Down Expand Up @@ -1579,14 +1571,9 @@ func (f *Frame) SetContent(html string, opts sobek.Value) error {
}

// SetInputFiles sets input files for the selected element.
func (f *Frame) SetInputFiles(selector string, files sobek.Value, opts sobek.Value) error {
func (f *Frame) SetInputFiles(selector string, files sobek.Value, popts *FrameSetInputFilesOptions) error {
f.log.Debugf("Frame:SetInputFiles", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)

popts := NewFrameSetInputFilesOptions(f.defaultTimeout())
if err := popts.Parse(f.ctx, opts); err != nil {
return fmt.Errorf("parsing setInputFiles options: %w", err)
}

pfiles := &Files{}
if err := pfiles.Parse(f.ctx, files); err != nil {
return fmt.Errorf("parsing setInputFiles parameter: %w", err)
Expand Down Expand Up @@ -1648,13 +1635,9 @@ func (f *Frame) setInputFiles(selector string, files *Files, opts *FrameSetInput
// TextContent returns the textContent attribute of the first element found
// that matches the selector. The second return value is true if the returned
// text content is not null or empty, and false otherwise.
func (f *Frame) TextContent(selector string, opts sobek.Value) (string, bool, error) {
func (f *Frame) TextContent(selector string, popts *FrameTextContentOptions) (string, bool, error) {
f.log.Debugf("Frame:TextContent", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)

popts := NewFrameTextContentOptions(f.defaultTimeout())
if err := popts.Parse(f.ctx, opts); err != nil {
return "", false, fmt.Errorf("parsing text content options: %w", err)
}
v, ok, err := f.textContent(selector, popts)
if err != nil {
return "", false, fmt.Errorf("getting text content of %q: %w", selector, err)
Expand Down
14 changes: 7 additions & 7 deletions internal/js/modules/k6/browser/common/page.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,10 +813,10 @@ func (p *Page) BringToFront() error {
}

// SetChecked sets the checked state of the element matching the provided selector.
func (p *Page) SetChecked(selector string, checked bool, opts sobek.Value) error {
func (p *Page) SetChecked(selector string, checked bool, popts *FrameCheckOptions) error {
p.logger.Debugf("Page:SetChecked", "sid:%v selector:%s checked:%t", p.sessionID(), selector, checked)

return p.MainFrame().SetChecked(selector, checked, opts)
return p.MainFrame().SetChecked(selector, checked, popts)
}

// Check checks an element matching the provided selector.
Expand Down Expand Up @@ -1340,10 +1340,10 @@ func (p *Page) Screenshot(opts *PageScreenshotOptions, sp ScreenshotPersister) (

// SelectOption selects the given options and returns the array of
// option values of the first element found that matches the selector.
func (p *Page) SelectOption(selector string, values sobek.Value, opts sobek.Value) ([]string, error) {
func (p *Page) SelectOption(selector string, values sobek.Value, popts *FrameSelectOptionOptions) ([]string, error) {
p.logger.Debugf("Page:SelectOption", "sid:%v selector:%s", p.sessionID(), selector)

return p.MainFrame().SelectOption(selector, values, opts)
return p.MainFrame().SelectOption(selector, values, popts)
}

// SetContent replaces the entire HTML document content.
Expand Down Expand Up @@ -1376,7 +1376,7 @@ func (p *Page) SetExtraHTTPHeaders(headers map[string]string) error {
}

// SetInputFiles sets input files for the selected element.
func (p *Page) SetInputFiles(selector string, files sobek.Value, opts sobek.Value) error {
func (p *Page) SetInputFiles(selector string, files sobek.Value, opts *FrameSetInputFilesOptions) error {
p.logger.Debugf("Page:SetInputFiles", "sid:%v selector:%s", p.sessionID(), selector)

return p.MainFrame().SetInputFiles(selector, files, opts)
Expand Down Expand Up @@ -1407,10 +1407,10 @@ func (p *Page) Tap(selector string, opts *FrameTapOptions) error {
// TextContent returns the textContent attribute of the first element found
// that matches the selector. The second return value is true if the returned
// text content is not null or empty, and false otherwise.
func (p *Page) TextContent(selector string, opts sobek.Value) (string, bool, error) {
func (p *Page) TextContent(selector string, popts *FrameTextContentOptions) (string, bool, error) {
p.logger.Debugf("Page:TextContent", "sid:%v selector:%s", p.sessionID(), selector)

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

// Timeout will return the default timeout or the one set by the user.
Expand Down
10 changes: 5 additions & 5 deletions internal/js/modules/k6/browser/tests/frame_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestFrameDismissDialogBox(t *testing.T) {
require.NoError(t, err)
}

result, ok, err := p.TextContent("#textField", nil)
result, ok, err := p.TextContent("#textField", common.NewFrameTextContentOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
require.True(t, ok)
assert.EqualValues(t, tt+" dismissed", result)
Expand Down Expand Up @@ -102,7 +102,7 @@ func TestFrameNoPanicWithEmbeddedIFrame(t *testing.T) {
_, err := p.Goto(tb.staticURL("embedded_iframe.html"), opts)
require.NoError(t, err)

result, ok, err := p.TextContent("#doneDiv", nil)
result, ok, err := p.TextContent("#doneDiv", common.NewFrameTextContentOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
require.True(t, ok)
assert.EqualValues(t, "Done!", result)
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestFrameNoPanicNavigateAndClickOnPageWithIFrames(t *testing.T) {
)
require.NoError(t, err)

result, ok, err := p.TextContent("#doneDiv", nil)
result, ok, err := p.TextContent("#doneDiv", common.NewFrameTextContentOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
require.True(t, ok)
assert.EqualValues(t, "Sign In Page", result)
Expand Down Expand Up @@ -226,13 +226,13 @@ func TestFrameSetChecked(t *testing.T) {
require.NoError(t, err)
assert.False(t, checked)

err = p.Frames()[0].SetChecked("#el", true, nil)
err = p.Frames()[0].SetChecked("#el", true, common.NewFrameCheckOptions(p.Frames()[0].Timeout()))
require.NoError(t, err)
checked, err = p.Frames()[0].IsChecked("#el", nil)
require.NoError(t, err)
assert.True(t, checked)

err = p.Frames()[0].SetChecked("#el", false, nil)
err = p.Frames()[0].SetChecked("#el", false, common.NewFrameCheckOptions(p.Frames()[0].Timeout()))
require.NoError(t, err)
checked, err = p.Frames()[0].IsChecked("#el", nil)
require.NoError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions internal/js/modules/k6/browser/tests/launch_options_slowmo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testPageSlowMoImpl(t, tb, func(_ *testBrowser, p *common.Page) {
_, err := p.SelectOption("select", tb.toSobekValue("foo"), nil)
_, err := p.SelectOption("select", tb.toSobekValue("foo"), common.NewFrameSelectOptionOptions(p.MainFrame().Timeout()))
require.NoError(t, err)
})
})
Expand Down Expand Up @@ -281,7 +281,7 @@ func TestBrowserOptionsSlowMo(t *testing.T) {
t.Parallel()
tb := newTestBrowser(t, withFileServer())
testFrameSlowMoImpl(t, tb, func(_ *testBrowser, f *common.Frame) {
_, err := f.SelectOption("select", tb.toSobekValue("foo"), nil)
_, err := f.SelectOption("select", tb.toSobekValue("foo"), common.NewFrameSelectOptionOptions(f.Timeout()))
require.NoError(t, err)
})
})
Expand Down
Loading
Loading