From 41592b932ca8cfc636b46816d8e4150aca613d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 30 Jun 2022 12:14:20 +0300 Subject: [PATCH] Fix type assertions and remove nolint This commit properly type asserts and removes nolint directives. There are new helpers to reduce code bloat and possible error messages refactoring work: + asGojaValue: returns a given value as a goja value or panics. + gojaValueToString: returns a given a value as string or panics. This commit also refactors tests to remove nolint directives. Fixes: #417 --- common/browser_test.go | 10 ++-- common/element_handle.go | 91 +++++++++++++++++++++++------------- common/frame.go | 34 ++++++-------- common/frame_session.go | 6 ++- common/helpers.go | 16 +++++++ common/page.go | 5 +- common/page_test.go | 5 +- common/screenshotter.go | 7 +-- tests/element_handle_test.go | 2 +- tests/page_test.go | 16 ++++--- tests/test_browser.go | 7 ++- 11 files changed, 127 insertions(+), 72 deletions(-) diff --git a/common/browser_test.go b/common/browser_test.go index b9b893f46..e1d0d305e 100644 --- a/common/browser_test.go +++ b/common/browser_test.go @@ -52,13 +52,17 @@ func TestBrowserNewPageInContext(t *testing.T) { ctx context.Context, method string, params easyjson.Marshaler, res easyjson.Unmarshaler, ) error { require.Equal(t, target.CommandCreateTarget, method) - tp := params.(*target.CreateTargetParams) //nolint:forcetypeassert + require.IsType(t, params, &target.CreateTargetParams{}) + tp, _ := params.(*target.CreateTargetParams) require.Equal(t, "about:blank", tp.URL) require.Equal(t, browserContextID, tp.BrowserContextID) // newPageInContext event handler will catch this target ID, and compare it to - // the new page's target ID to detect whether the page is loaded. - res.(*target.CreateTargetReturns).TargetID = targetID //nolint:forcetypeassert + // the new page's target ID to detect whether the page + // is loaded. + require.IsType(t, res, &target.CreateTargetReturns{}) + v, _ := res.(*target.CreateTargetReturns) + v.TargetID = targetID // for the event handler to work, there needs to be an event called // EventBrowserContextPage to be fired. this normally happens when the browser's diff --git a/common/element_handle.go b/common/element_handle.go index f7258723e..127d2e840 100644 --- a/common/element_handle.go +++ b/common/element_handle.go @@ -63,7 +63,13 @@ func (h *ElementHandle) boundingBox() (*Rect, error) { func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position) (bool, error) { frame := h.ownerFrame(apiCtx) if frame != nil && frame.parentFrame != nil { - element := h.frame.FrameElement().(*ElementHandle) //nolint:forcetypeassert + var ( + el = h.frame.FrameElement() + element, ok = el.(*ElementHandle) + ) + if !ok { + return false, fmt.Errorf("unexpected type %T", el) + } box, err := element.boundingBox() if err != nil { return false, err @@ -91,9 +97,11 @@ func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position) // Either we're done or an error happened (returned as "error:..." from JS) const done = "done" - //nolint:forcetypeassert - value := result.(goja.Value) - if value.ExportType().Kind() != reflect.String { + v, ok := result.(goja.Value) + if !ok { + return false, fmt.Errorf("unexpected type %T", result) + } + if v.ExportType().Kind() != reflect.String { // We got a { hitTargetDescription: ... } result // Meaning: Another element is preventing pointer events. // @@ -102,14 +110,14 @@ func (h *ElementHandle) checkHitTargetAt(apiCtx context.Context, point Position) // because we don't need any more functionality from this JS function // right now. return false, errorFromDOMError("error:intercept") - } else if value.String() != done { - return false, errorFromDOMError(value.String()) + } else if v.String() != done { + return false, errorFromDOMError(v.String()) } return true, nil } -func (h *ElementHandle) checkElementState(apiCtx context.Context, state string) (*bool, error) { +func (h *ElementHandle) checkElementState(_ context.Context, state string) (*bool, error) { fn := ` (node, injected, state) => { return injected.checkElementState(node, state); @@ -123,16 +131,20 @@ func (h *ElementHandle) checkElementState(apiCtx context.Context, state string) if err != nil { return nil, err } - - value := result.(goja.Value) - switch value.ExportType().Kind() { + v, ok := result.(goja.Value) + if !ok { + return nil, fmt.Errorf("unexpected type %T", result) + } + //nolint:exhaustive + switch v.ExportType().Kind() { case reflect.String: // An error happened (returned as "error:..." from JS) - return nil, errorFromDOMError(value.String()) + return nil, errorFromDOMError(v.String()) case reflect.Bool: returnVal := new(bool) - *returnVal = value.ToBoolean() + *returnVal = v.ToBoolean() return returnVal, nil } + return nil, fmt.Errorf("cannot check state %q of element: %q", state, reflect.TypeOf(result)) } @@ -263,11 +275,11 @@ func (h *ElementHandle) fill(_ context.Context, value string) error { if err != nil { return err } - r, ok := result.(goja.Value) + v, ok := result.(goja.Value) if !ok { - return fmt.Errorf("expected goja value; got %T", result) + return fmt.Errorf("unexpected type %T", result) } - if s := r.String(); s != resultDone { + if s := v.String(); s != resultDone { // Either we're done or an error happened (returned as "error:..." from JS) return errorFromDOMError(s) } @@ -642,7 +654,9 @@ func (h *ElementHandle) waitAndScrollIntoViewIfNeeded(apiCtx context.Context, fo return nil } -func (h *ElementHandle) waitForElementState(apiCtx context.Context, states []string, timeout time.Duration) (bool, error) { +func (h *ElementHandle) waitForElementState( + apiCtx context.Context, states []string, timeout time.Duration, +) (bool, error) { fn := ` (node, injected, states, timeout) => { return injected.waitForElementStates(node, states, timeout); @@ -656,17 +670,21 @@ func (h *ElementHandle) waitForElementState(apiCtx context.Context, states []str if err != nil { return false, errorFromDOMError(err.Error()) } - - value := result.(goja.Value) - switch value.ExportType().Kind() { + v, ok := result.(goja.Value) + if !ok { + return false, fmt.Errorf("unexpected type %T", result) + } + //nolint:exhaustive + switch v.ExportType().Kind() { case reflect.String: // Either we're done or an error happened (returned as "error:..." from JS) - if value.String() == "done" { + if v.String() == "done" { return true, nil } - return false, errorFromDOMError(value.String()) + return false, errorFromDOMError(v.String()) case reflect.Bool: - return value.ToBoolean(), nil + return v.ToBoolean(), nil } + return false, fmt.Errorf("cannot check states %v of element: %q", states, reflect.TypeOf(result)) } @@ -815,12 +833,13 @@ func (h *ElementHandle) GetAttribute(name string) goja.Value { } opts := NewElementHandleBaseOptions(h.defaultTimeout()) actFn := h.newAction([]string{}, fn, opts.Force, opts.NoWaitAfter, opts.Timeout) - value, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) + v, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) if err != nil { k6ext.Panic(h.ctx, "cannot get attribute of %q: %q", name, err) } applySlowMo(h.ctx) - return value.(goja.Value) + + return asGojaValue(h.ctx, v) } // Hover scrolls element into view and hovers over its center point. @@ -847,12 +866,13 @@ func (h *ElementHandle) InnerHTML() string { } opts := NewElementHandleBaseOptions(h.defaultTimeout()) actFn := h.newAction([]string{}, fn, opts.Force, opts.NoWaitAfter, opts.Timeout) - value, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) + v, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) if err != nil { k6ext.Panic(h.ctx, "cannot get element's inner HTML: %w", err) } applySlowMo(h.ctx) - return value.(goja.Value).String() + + return gojaValueToString(h.ctx, v) } // InnerText returns the inner text of the element. @@ -862,12 +882,13 @@ func (h *ElementHandle) InnerText() string { } opts := NewElementHandleBaseOptions(h.defaultTimeout()) actFn := h.newAction([]string{}, fn, opts.Force, opts.NoWaitAfter, opts.Timeout) - value, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) + v, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) if err != nil { k6ext.Panic(h.ctx, "cannot get element's inner text: %w", err) } applySlowMo(h.ctx) - return value.(goja.Value).String() + + return gojaValueToString(h.ctx, v) } func (h *ElementHandle) InputValue(opts goja.Value) string { @@ -879,12 +900,13 @@ func (h *ElementHandle) InputValue(opts goja.Value) string { return handle.inputValue(apiCtx) } actFn := h.newAction([]string{}, fn, actionOpts.Force, actionOpts.NoWaitAfter, actionOpts.Timeout) - value, err := callApiWithTimeout(h.ctx, actFn, actionOpts.Timeout) + v, err := callApiWithTimeout(h.ctx, actFn, actionOpts.Timeout) if err != nil { k6ext.Panic(h.ctx, "cannot get element's input value: %w", err) } applySlowMo(h.ctx) - return value.(goja.Value).String() + + return gojaValueToString(h.ctx, v) } // IsChecked checks if a checkbox or radio is checked. @@ -1180,10 +1202,12 @@ func (h *ElementHandle) SelectOption(values goja.Value, opts goja.Value) []strin k6ext.Panic(h.ctx, "cannot handle element select option: %w", err) } var returnVal []string - if err := rt.ExportTo(selectedOptions.(goja.Value), &returnVal); err != nil { + if err := rt.ExportTo(asGojaValue(h.ctx, selectedOptions), &returnVal); err != nil { k6ext.Panic(h.ctx, "cannot unpack options in element select option: %w", err) } + applySlowMo(h.ctx) + return returnVal } @@ -1232,12 +1256,13 @@ func (h *ElementHandle) TextContent() string { } opts := NewElementHandleBaseOptions(h.defaultTimeout()) actFn := h.newAction([]string{}, fn, opts.Force, opts.NoWaitAfter, opts.Timeout) - value, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) + v, err := callApiWithTimeout(h.ctx, actFn, opts.Timeout) if err != nil { k6ext.Panic(h.ctx, "cannot get text content of element: %w", err) } applySlowMo(h.ctx) - return value.(goja.Value).String() + + return gojaValueToString(h.ctx, v) } // Type scrolls element into view, focuses element and types text. diff --git a/common/frame.go b/common/frame.go index 2816c16ef..b919b5c07 100644 --- a/common/frame.go +++ b/common/frame.go @@ -817,23 +817,17 @@ func (f *Frame) Content() string { rt := f.vu.Runtime() js := `() => { - let content = ''; - if (document.doctype) { - content = new XMLSerializer().serializeToString(document.doctype); - } - if (document.documentElement) { - content += document.documentElement.outerHTML; - } - return content; - }` - - c := f.Evaluate(rt.ToValue(js)) - content, ok := c.(goja.Value) - if !ok { - k6ext.Panic(f.ctx, "unexpected content() value type: %T", c) - } + let content = ''; + if (document.doctype) { + content = new XMLSerializer().serializeToString(document.doctype); + } + if (document.documentElement) { + content += document.documentElement.outerHTML; + } + return content; + }` - return content.ToString().String() + return gojaValueToString(f.ctx, f.Evaluate(rt.ToValue(js))) } // Dblclick double clicks an element matching provided selector. @@ -1113,7 +1107,7 @@ func (f *Frame) innerHTML(selector string, opts *FrameInnerHTMLOptions) (string, return "", fmt.Errorf("unexpected type %T", v) } - return gv.ToString().String(), nil + return gv.String(), nil } // InnerText returns the inner text of the first element found @@ -1155,7 +1149,7 @@ func (f *Frame) innerText(selector string, opts *FrameInnerTextOptions) (string, return "", fmt.Errorf("unexpected type %T", v) } - return gv.ToString().String(), nil + return gv.String(), nil } // InputValue returns the input value of the first element found @@ -1192,7 +1186,7 @@ func (f *Frame) inputValue(selector string, opts *FrameInputValueOptions) (strin return "", fmt.Errorf("unexpected type %T", v) } - return gv.ToString().String(), nil + return gv.String(), nil } // IsDetached returns whether the frame is detached or not. @@ -1669,7 +1663,7 @@ func (f *Frame) textContent(selector string, opts *FrameTextContentOptions) (str return "", fmt.Errorf("unexpected type %T", v) } - return gv.ToString().String(), nil + return gv.String(), nil } func (f *Frame) Title() string { diff --git a/common/frame_session.go b/common/frame_session.go index c7c4bf486..7851a91f3 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -866,7 +866,11 @@ func (fs *FrameSession) onTargetCrashed(event *inspector.EventTargetCrashed) { fs.logger.Debugf("FrameSession:onTargetCrashed", "sid:%v tid:%v", fs.session.ID(), fs.targetID) // TODO:? - fs.session.(*Session).markAsCrashed() //nolint:forcetypeassert + s, ok := fs.session.(*Session) + if !ok { + k6ext.Panic(fs.ctx, "unexpected type %T", fs.session) + } + s.markAsCrashed() fs.page.didCrash() } diff --git a/common/helpers.go b/common/helpers.go index 6e1164d92..febc898f1 100644 --- a/common/helpers.go +++ b/common/helpers.go @@ -223,3 +223,19 @@ func TrimQuotes(s string) string { func gojaValueExists(v goja.Value) bool { return v != nil && !goja.IsUndefined(v) && !goja.IsNull(v) } + +// asGojaValue return v as a goja value. +// panics if v is not a goja value. +func asGojaValue(ctx context.Context, v interface{}) goja.Value { + gv, ok := v.(goja.Value) + if !ok { + k6ext.Panic(ctx, "unexpected type %T", v) + } + return gv +} + +// gojaValueToString returns v as string. +// panics if v is not a goja value. +func gojaValueToString(ctx context.Context, v interface{}) string { + return asGojaValue(ctx, v).String() +} diff --git a/common/page.go b/common/page.go index eef9e12c7..5760b2b49 100644 --- a/common/page.go +++ b/common/page.go @@ -814,9 +814,8 @@ func (p *Page) TextContent(selector string, opts goja.Value) string { func (p *Page) Title() string { p.logger.Debugf("Page:Title", "sid:%v", p.sessionID()) - rt := p.vu.Runtime() - js := `() => document.title` - return p.Evaluate(rt.ToValue(js)).(goja.Value).String() + v := p.vu.Runtime().ToValue(`() => document.title`) + return gojaValueToString(p.ctx, p.Evaluate(v)) } func (p *Page) Type(selector string, text string, opts goja.Value) { diff --git a/common/page_test.go b/common/page_test.go index 7d0d339b0..d6df78248 100644 --- a/common/page_test.go +++ b/common/page_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestPageLocator can be removed later on when we add integration @@ -23,7 +24,9 @@ func TestPageLocator(t *testing.T) { mainFrame: &Frame{id: wantMainFrameID, ctx: ctx}, }, } - l := p.Locator(wantSelector, nil).(*Locator) //nolint:forcetypeassert + v := p.Locator(wantSelector, nil) + require.IsType(t, v, &Locator{}) + l, _ := v.(*Locator) assert.Equal(t, wantSelector, l.selector) assert.Equal(t, wantMainFrameID, string(l.frame.id)) diff --git a/common/screenshotter.go b/common/screenshotter.go index 9c6b97168..359663f76 100644 --- a/common/screenshotter.go +++ b/common/screenshotter.go @@ -71,11 +71,12 @@ func (s *screenshotter) fullPageSize(p *Page) (*Size, error) { if err != nil { return nil, err } - r, ok := result.(goja.Value) + v, ok := result.(goja.Value) if !ok { - return nil, fmt.Errorf("cannot convert result to goja value") + return nil, fmt.Errorf("unexpected type %T", result) } - o := r.ToObject(rt) + o := v.ToObject(rt) + return &Size{ Width: o.Get("width").ToFloat(), Height: o.Get("height").ToFloat(), diff --git a/tests/element_handle_test.go b/tests/element_handle_test.go index fa0920fd1..431309787 100644 --- a/tests/element_handle_test.go +++ b/tests/element_handle_test.go @@ -189,7 +189,7 @@ func TestElementHandleClickConcealedLink(t *testing.T) { () => window.clickResult ` cr := p.Evaluate(tb.toGojaValue(cmd)) - return cr.(goja.Value).String() //nolint:forcetypeassert + return tb.asGojaValue(cr).String() } require.NotNil(t, p.Goto(tb.staticURL("/concealed_link.html"), nil)) require.Equal(t, wantBefore, clickResult()) diff --git a/tests/page_test.go b/tests/page_test.go index a0f33568b..2406361c1 100644 --- a/tests/page_test.go +++ b/tests/page_test.go @@ -172,9 +172,11 @@ func TestPageGotoWaitUntilLoad(t *testing.T) { WaitUntil string `js:"waitUntil"` }{WaitUntil: "load"})) - results := p.Evaluate(b.toGojaValue("() => window.results")) - var actual []string - _ = b.runtime().ExportTo(results.(goja.Value), &actual) //nolint:forcetypeassert + var ( + results = p.Evaluate(b.toGojaValue("() => window.results")) + actual []string + ) + _ = b.runtime().ExportTo(b.asGojaValue(results), &actual) assert.EqualValues(t, []string{"DOMContentLoaded", "load"}, actual, `expected "load" event to have fired`) } @@ -188,9 +190,11 @@ func TestPageGotoWaitUntilDOMContentLoaded(t *testing.T) { WaitUntil string `js:"waitUntil"` }{WaitUntil: "domcontentloaded"})) - results := p.Evaluate(b.toGojaValue("() => window.results")) - var actual []string - _ = b.runtime().ExportTo(results.(goja.Value), &actual) //nolint:forcetypeassert + var ( + results = p.Evaluate(b.toGojaValue("() => window.results")) + actual []string + ) + _ = b.runtime().ExportTo(b.asGojaValue(results), &actual) assert.EqualValues(t, "DOMContentLoaded", actual[0], `expected "DOMContentLoaded" event to have fired`) } diff --git a/tests/test_browser.go b/tests/test_browser.go index 68e46ce48..13766918e 100644 --- a/tests/test_browser.go +++ b/tests/test_browser.go @@ -22,6 +22,7 @@ package tests import ( "context" + "fmt" "net/http" "os" "strconv" @@ -123,7 +124,11 @@ func newTestBrowser(tb testing.TB, opts ...interface{}) *testBrowser { } // launch the browser - bt := chromium.NewBrowserType(vu.Context()).(*chromium.BrowserType) //nolint:forcetypeassert + v := chromium.NewBrowserType(vu.Context()) + bt, ok := v.(*chromium.BrowserType) + if !ok { + panic(fmt.Errorf("testBrowser: unexpected browser type %T", v)) + } b := bt.Launch(rt.ToValue(launchOpts)) tb.Cleanup(func() { select {