From a0e53f4403503ca1de4d789e21c211c9a58fb0d2 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 29 Jan 2024 16:31:47 +0000 Subject: [PATCH 1/5] Refactor goja parse for elementHandle.screenshot --- browser/mapping.go | 19 +++++++++++++++++-- common/element_handle.go | 19 +++++++++---------- tests/element_handle_test.go | 8 ++++++-- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/browser/mapping.go b/browser/mapping.go index 4acbd702e..fc1bdcc5c 100644 --- a/browser/mapping.go +++ b/browser/mapping.go @@ -232,6 +232,7 @@ func mapJSHandle(vu moduleVU, jsh common.JSHandleAPI) mapping { // //nolint:funlen func mapElementHandle(vu moduleVU, eh *common.ElementHandle) mapping { + rt := vu.Runtime() maps := mapping{ "boundingBox": eh.BoundingBox, "check": eh.Check, @@ -280,8 +281,22 @@ func mapElementHandle(vu moduleVU, eh *common.ElementHandle) mapping { return mapFrame(vu, f), nil }, "press": eh.Press, - "screenshot": func(opts goja.Value) goja.ArrayBuffer { - return eh.Screenshot(opts, vu.LocalFilePersister) + "screenshot": func(opts goja.Value) (*goja.ArrayBuffer, error) { + ctx := vu.Context() + + popts := common.NewElementHandleScreenshotOptions(eh.Timeout()) + if err := popts.Parse(ctx, opts); err != nil { + return nil, fmt.Errorf("parsing frame screenshot options: %w", err) + } + + bb, err := eh.Screenshot(popts, vu.LocalFilePersister) + if err != nil { + return nil, err //nolint:wrapcheck + } + + ab := rt.NewArrayBuffer(bb) + + return &ab, nil }, "scrollIntoViewIfNeeded": eh.ScrollIntoViewIfNeeded, "selectOption": eh.SelectOption, diff --git a/common/element_handle.go b/common/element_handle.go index c769f4627..c4870140e 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -1178,7 +1178,10 @@ func (h *ElementHandle) setChecked(apiCtx context.Context, checked bool, p *Posi } // Screenshot will instruct Chrome to save a screenshot of the current element and save it to specified file. -func (h *ElementHandle) Screenshot(opts goja.Value, fp *storage.LocalFilePersister) goja.ArrayBuffer { +func (h *ElementHandle) Screenshot( + opts *ElementHandleScreenshotOptions, + fp *storage.LocalFilePersister, +) (*[]byte, error) { spanCtx, span := TraceAPICall( h.ctx, h.frame.page.targetID.String(), @@ -1186,19 +1189,15 @@ func (h *ElementHandle) Screenshot(opts goja.Value, fp *storage.LocalFilePersist ) defer span.End() - rt := h.execCtx.vu.Runtime() - parsedOpts := NewElementHandleScreenshotOptions(h.defaultTimeout()) - if err := parsedOpts.Parse(h.ctx, opts); err != nil { - k6ext.Panic(h.ctx, "parsing screenshot options: %w", err) - } - span.SetAttributes(attribute.String("screenshot.path", parsedOpts.Path)) + span.SetAttributes(attribute.String("screenshot.path", opts.Path)) s := newScreenshotter(spanCtx, fp) - buf, err := s.screenshotElement(h, parsedOpts) + buf, err := s.screenshotElement(h, opts) if err != nil { - k6ext.Panic(h.ctx, "taking screenshot: %w", err) + return nil, fmt.Errorf("taking screenshot of elementHandle: %w", err) } - return rt.NewArrayBuffer(*buf) + + return buf, err } func (h *ElementHandle) ScrollIntoViewIfNeeded(opts goja.Value) { diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index 182f72226..f700b33d7 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -375,9 +375,13 @@ func TestElementHandleScreenshot(t *testing.T) { elem, err := p.Query("div") require.NoError(t, err) - buf := elem.Screenshot(nil, &storage.LocalFilePersister{}) + buf, err := elem.Screenshot( + common.NewElementHandleScreenshotOptions(elem.Timeout()), + &storage.LocalFilePersister{}, + ) + require.NoError(t, err) - reader := bytes.NewReader(buf.Bytes()) + reader := bytes.NewReader(*buf) img, err := png.Decode(reader) assert.Nil(t, err) From 50fee95803f9e5046aaf10b0cabe322d0f76893d Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 29 Jan 2024 16:40:29 +0000 Subject: [PATCH 2/5] Refactor goja parse for page.screenshot --- browser/mapping.go | 18 ++++++++++++++++-- common/page.go | 16 ++++++---------- tests/page_test.go | 11 +++++------ 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/browser/mapping.go b/browser/mapping.go index fc1bdcc5c..6dca230d3 100644 --- a/browser/mapping.go +++ b/browser/mapping.go @@ -704,8 +704,22 @@ func mapPage(vu moduleVU, p *common.Page) mapping { return rt.ToValue(r).ToObject(rt) }, "route": p.Route, - "screenshot": func(opts goja.Value) goja.ArrayBuffer { - return p.Screenshot(opts, vu.LocalFilePersister) + "screenshot": func(opts goja.Value) (*goja.ArrayBuffer, error) { + ctx := vu.Context() + + popts := common.NewPageScreenshotOptions() + if err := popts.Parse(ctx, opts); err != nil { + return nil, fmt.Errorf("parsing page screenshot options: %w", err) + } + + bb, err := p.Screenshot(popts, vu.LocalFilePersister) + if err != nil { + return nil, err //nolint:wrapcheck + } + + ab := rt.NewArrayBuffer(bb) + + return &ab, nil }, "selectOption": p.SelectOption, "setContent": p.SetContent, diff --git a/common/page.go b/common/page.go index 2306a0dbd..4133bd532 100644 --- a/common/page.go +++ b/common/page.go @@ -1121,23 +1121,19 @@ func (p *Page) Route(url goja.Value, handler goja.Callable) { } // Screenshot will instruct Chrome to save a screenshot of the current page and save it to specified file. -func (p *Page) Screenshot(opts goja.Value, fp *storage.LocalFilePersister) goja.ArrayBuffer { +func (p *Page) Screenshot(opts *PageScreenshotOptions, fp *storage.LocalFilePersister) (*[]byte, error) { spanCtx, span := TraceAPICall(p.ctx, p.targetID.String(), "page.screenshot") defer span.End() - parsedOpts := NewPageScreenshotOptions() - if err := parsedOpts.Parse(p.ctx, opts); err != nil { - k6ext.Panic(p.ctx, "parsing screenshot options: %w", err) - } - span.SetAttributes(attribute.String("screenshot.path", parsedOpts.Path)) + span.SetAttributes(attribute.String("screenshot.path", opts.Path)) s := newScreenshotter(spanCtx, fp) - buf, err := s.screenshotPage(p, parsedOpts) + buf, err := s.screenshotPage(p, opts) if err != nil { - k6ext.Panic(p.ctx, "capturing screenshot: %w", err) + return nil, fmt.Errorf("taking screenshot of page: %w", err) } - rt := p.vu.Runtime() - return rt.NewArrayBuffer(*buf) + + return buf, err } func (p *Page) SelectOption(selector string, values goja.Value, opts goja.Value) []string { diff --git a/tests/page_test.go b/tests/page_test.go index dbd304818..413c44a53 100644 --- a/tests/page_test.go +++ b/tests/page_test.go @@ -474,13 +474,12 @@ func TestPageScreenshotFullpage(t *testing.T) { } `) - buf := p.Screenshot(tb.toGojaValue(struct { - FullPage bool `js:"fullPage"` - }{ - FullPage: true, - }), &storage.LocalFilePersister{}) + opts := common.NewPageScreenshotOptions() + opts.FullPage = true + buf, err := p.Screenshot(opts, &storage.LocalFilePersister{}) + require.NoError(t, err) - reader := bytes.NewReader(buf.Bytes()) + reader := bytes.NewReader(*buf) img, err := png.Decode(reader) assert.Nil(t, err) From 8f43269cbb239906d7f151426f29c0ee4b8d793b Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 29 Jan 2024 16:48:21 +0000 Subject: [PATCH 3/5] Refactor screenshotter to return []byte As far as I can tell there is no reason to return a pointer to the slice of bytes, since the slice it self is referenced with a pointer. --- common/element_handle.go | 2 +- common/page.go | 2 +- common/screenshotter.go | 10 ++++++---- tests/element_handle_test.go | 2 +- tests/page_test.go | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/common/element_handle.go b/common/element_handle.go index c4870140e..19b646cf0 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -1181,7 +1181,7 @@ func (h *ElementHandle) setChecked(apiCtx context.Context, checked bool, p *Posi func (h *ElementHandle) Screenshot( opts *ElementHandleScreenshotOptions, fp *storage.LocalFilePersister, -) (*[]byte, error) { +) ([]byte, error) { spanCtx, span := TraceAPICall( h.ctx, h.frame.page.targetID.String(), diff --git a/common/page.go b/common/page.go index 4133bd532..3bda07b73 100644 --- a/common/page.go +++ b/common/page.go @@ -1121,7 +1121,7 @@ func (p *Page) Route(url goja.Value, handler goja.Callable) { } // Screenshot will instruct Chrome to save a screenshot of the current page and save it to specified file. -func (p *Page) Screenshot(opts *PageScreenshotOptions, fp *storage.LocalFilePersister) (*[]byte, error) { +func (p *Page) Screenshot(opts *PageScreenshotOptions, fp *storage.LocalFilePersister) ([]byte, error) { spanCtx, span := TraceAPICall(p.ctx, p.targetID.String(), "page.screenshot") defer span.End() diff --git a/common/screenshotter.go b/common/screenshotter.go index 4bb9dd8d1..08c8c6553 100644 --- a/common/screenshotter.go +++ b/common/screenshotter.go @@ -141,7 +141,7 @@ func (s *screenshotter) restoreViewport(p *Page, originalViewport *Size) error { //nolint:funlen,cyclop func (s *screenshotter) screenshot( sess session, doc, viewport *Rect, format ImageFormat, omitBackground bool, quality int64, path string, -) (*[]byte, error) { +) ([]byte, error) { var ( buf []byte clip *cdppage.Viewport @@ -222,10 +222,11 @@ func (s *screenshotter) screenshot( } } - return &buf, nil + return buf, nil } -func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleScreenshotOptions) (*[]byte, error) { +//nolint:funlen,cyclop +func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleScreenshotOptions) ([]byte, error) { format := opts.Format viewportSize, originalViewportSize, err := s.originalViewportSize(h.frame.page) if err != nil { @@ -296,7 +297,8 @@ func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleS return buf, nil } -func (s *screenshotter) screenshotPage(p *Page, opts *PageScreenshotOptions) (*[]byte, error) { +//nolint:funlen,cyclop +func (s *screenshotter) screenshotPage(p *Page, opts *PageScreenshotOptions) ([]byte, error) { format := opts.Format // Infer file format by path diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index f700b33d7..dba71d7fc 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -381,7 +381,7 @@ func TestElementHandleScreenshot(t *testing.T) { ) require.NoError(t, err) - reader := bytes.NewReader(*buf) + reader := bytes.NewReader(buf) img, err := png.Decode(reader) assert.Nil(t, err) diff --git a/tests/page_test.go b/tests/page_test.go index 413c44a53..b83c3141c 100644 --- a/tests/page_test.go +++ b/tests/page_test.go @@ -479,7 +479,7 @@ func TestPageScreenshotFullpage(t *testing.T) { buf, err := p.Screenshot(opts, &storage.LocalFilePersister{}) require.NoError(t, err) - reader := bytes.NewReader(*buf) + reader := bytes.NewReader(buf) img, err := png.Decode(reader) assert.Nil(t, err) From 0cddf7c8f0a1e6ab988a2461b234d31120b88ec6 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 29 Jan 2024 17:20:36 +0000 Subject: [PATCH 4/5] Remove goja type assertion The remote parser no longer works with goja so the result from evaluate will not return a goja value. Now just type assert without goja. --- common/screenshotter.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/common/screenshotter.go b/common/screenshotter.go index 08c8c6553..b1ea66d8f 100644 --- a/common/screenshotter.go +++ b/common/screenshotter.go @@ -12,7 +12,6 @@ import ( "github.com/chromedp/cdproto/cdp" "github.com/chromedp/cdproto/emulation" cdppage "github.com/chromedp/cdproto/page" - "github.com/dop251/goja" "github.com/grafana/xk6-browser/storage" ) @@ -121,12 +120,14 @@ func (s *screenshotter) originalViewportSize(p *Page) (*Size, *Size, error) { if err != nil { return nil, nil, fmt.Errorf("getting viewport dimensions: %w", err) } - r, ok := result.(goja.Object) - if !ok { - return nil, nil, fmt.Errorf("cannot convert to goja object: %w", err) + + var returnVal Size + if err := convert(result, &returnVal); err != nil { + return nil, nil, fmt.Errorf("unpacking window size: %w", err) } - viewportSize.Width = r.Get("width").ToFloat() - viewportSize.Height = r.Get("height").ToFloat() + + viewportSize.Width = returnVal.Width + viewportSize.Height = returnVal.Height return &viewportSize, &originalViewportSize, nil } @@ -277,14 +278,16 @@ func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleS } documentRect := bbox - rt := h.execCtx.vu.Runtime() scrollOffset := h.Evaluate(`() => { return {x: window.scrollX, y: window.scrollY};}`) - switch s := scrollOffset.(type) { - case goja.Value: - documentRect.X += s.ToObject(rt).Get("x").ToFloat() - documentRect.Y += s.ToObject(rt).Get("y").ToFloat() + + var returnVal Position + if err := convert(scrollOffset, &returnVal); err != nil { + return nil, fmt.Errorf("unpacking scroll offset: %w", err) } + documentRect.X += returnVal.X + documentRect.Y += returnVal.Y + buf, err := s.screenshot(h.frame.page.session, documentRect.enclosingIntRect(), nil, format, opts.OmitBackground, opts.Quality, opts.Path) if err != nil { return nil, err From 43acc02367b0306cc66701d788378e4295734c00 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Wed, 31 Jan 2024 15:24:38 +0000 Subject: [PATCH 5/5] Add skip gocognit To fix the linter issue --- common/screenshotter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/screenshotter.go b/common/screenshotter.go index b1ea66d8f..04a83acc8 100644 --- a/common/screenshotter.go +++ b/common/screenshotter.go @@ -300,7 +300,7 @@ func (s *screenshotter) screenshotElement(h *ElementHandle, opts *ElementHandleS return buf, nil } -//nolint:funlen,cyclop +//nolint:funlen,cyclop,gocognit func (s *screenshotter) screenshotPage(p *Page, opts *PageScreenshotOptions) ([]byte, error) { format := opts.Format