diff --git a/README.md b/README.md index 433c316d6..6cde7426f 100644 --- a/README.md +++ b/README.md @@ -357,27 +357,35 @@ export default function () { const currentBet = page.locator("//p[starts-with(text(),'Your bet: ')]"); - // the tails locator clicks on the tails button by using the - // locator's selector. - tails.click(); + // In the following Promise.all the tails locator clicks + // on the tails button by using the locator's selector. // Since clicking on each button causes page navigation, - // waitForNavigation is needed. It's because the page + // waitForNavigation is needed -- this is because the page // won't be ready until the navigation completes. - page.waitForNavigation(); - console.log(currentBet.innerText()); - - // the heads locator clicks on the heads button by using the - // locator's selector. - heads.click(); - page.waitForNavigation(); - console.log(currentBet.innerText()); - - tails.click(); - page.waitForNavigation(); - console.log(currentBet.innerText()); - - page.close(); - browser.close(); + // Setting up the waitForNavigation first before the click + // is important to avoid race conditions. + Promise.all([ + page.waitForNavigation(), + tails.click(), + ]).then(() => { + console.log(currentBet.innerText()); + // the heads locator clicks on the heads button + // by using the locator's selector. + return Promise.all([ + page.waitForNavigation(), + heads.click(), + ]); + }).then(() => { + console.log(currentBet.innerText()); + return Promise.all([ + page.waitForNavigation(), + tails.click(), + ]); + }).finally(() => { + console.log(currentBet.innerText()); + page.close(); + browser.close(); + }) } ``` diff --git a/api/frame.go b/api/frame.go index da9690f71..07542bf91 100644 --- a/api/frame.go +++ b/api/frame.go @@ -51,7 +51,7 @@ type Frame interface { URL() string WaitForFunction(pageFunc, opts goja.Value, args ...goja.Value) *goja.Promise WaitForLoadState(state string, opts goja.Value) - WaitForNavigation(opts goja.Value) Response + WaitForNavigation(opts goja.Value) *goja.Promise WaitForSelector(selector string, opts goja.Value) ElementHandle WaitForTimeout(timeout int64) } diff --git a/api/page.go b/api/page.go index 0fdb7238e..348be14f5 100644 --- a/api/page.go +++ b/api/page.go @@ -92,7 +92,7 @@ type Page interface { WaitForEvent(event string, optsOrPredicate goja.Value) interface{} WaitForFunction(fn, opts goja.Value, args ...goja.Value) *goja.Promise WaitForLoadState(state string, opts goja.Value) - WaitForNavigation(opts goja.Value) Response + WaitForNavigation(opts goja.Value) *goja.Promise WaitForRequest(urlOrPredicate, opts goja.Value) Request WaitForResponse(urlOrPredicate, opts goja.Value) Response WaitForSelector(selector string, opts goja.Value) ElementHandle diff --git a/common/frame.go b/common/frame.go index a3b9123f6..63896e332 100644 --- a/common/frame.go +++ b/common/frame.go @@ -1821,8 +1821,10 @@ func (f *Frame) WaitForLoadState(state string, opts goja.Value) { } // WaitForNavigation waits for the given navigation lifecycle event to happen. -func (f *Frame) WaitForNavigation(opts goja.Value) api.Response { - return f.manager.WaitForFrameNavigation(f, opts) +func (f *Frame) WaitForNavigation(opts goja.Value) *goja.Promise { + return k6ext.Promise(f.ctx, func() (result interface{}, reason error) { + return f.manager.waitForFrameNavigation(f, opts) + }) } // WaitForSelector waits for the given selector to match the waiting criteria. diff --git a/common/frame_manager.go b/common/frame_manager.go index 63c3d56ea..fc6b00553 100644 --- a/common/frame_manager.go +++ b/common/frame_manager.go @@ -685,8 +685,8 @@ func (m *FrameManager) Page() api.Page { return nil } -// WaitForFrameNavigation waits for the given navigation lifecycle event to happen. -func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api.Response { +// waitForFrameNavigation waits for the given navigation lifecycle event to happen. +func (m *FrameManager) waitForFrameNavigation(frame *Frame, opts goja.Value) (api.Response, error) { m.logger.Debugf("FrameManager:WaitForFrameNavigation", "fmid:%d fid:%s furl:%s", m.ID(), frame.ID(), frame.URL()) @@ -696,7 +696,7 @@ func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api parsedOpts := NewFrameWaitForNavigationOptions(time.Duration(m.timeoutSettings.timeout()) * time.Second) if err := parsedOpts.Parse(m.ctx, opts); err != nil { - k6ext.Panic(m.ctx, "parsing wait for frame navigation options: %v", err) + return nil, fmt.Errorf("parsing wait for frame navigation options: %w", err) } ch, evCancelFn := createWaitForEventHandler(m.ctx, frame, []string{EventFrameNavigation}, @@ -712,9 +712,9 @@ func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api m.logger.Warnf("FrameManager:WaitForFrameNavigation:<-ctx.Done", "fmid:%d furl:%s err:%v", m.ID(), frame.URL(), m.ctx.Err()) - return nil + return nil, nil case <-time.After(parsedOpts.Timeout): - k6ext.Panic(m.ctx, "waiting for frame navigation timed out after %s", parsedOpts.Timeout) + return nil, fmt.Errorf("waiting for frame navigation timed out after %s", parsedOpts.Timeout) case data := <-ch: event = data.(*NavigationEvent) } @@ -723,7 +723,7 @@ func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api // In case of navigation within the same document (e.g. via an anchor // link or the History API), there is no new document and a // LifecycleEvent will not be fired, so we don't need to wait for it. - return nil + return nil, nil } if frame.hasSubtreeLifecycleEventFired(parsedOpts.WaitUntil) { @@ -735,7 +735,7 @@ func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api return data.(LifecycleEvent) == parsedOpts.WaitUntil }, parsedOpts.Timeout) if err != nil { - k6ext.Panic(m.ctx, "waiting for frame navigation until %q: %v", parsedOpts.WaitUntil, err) + return nil, fmt.Errorf("waiting for frame navigation until %q: %w", parsedOpts.WaitUntil, err) } } @@ -743,7 +743,7 @@ func (m *FrameManager) WaitForFrameNavigation(frame *Frame, opts goja.Value) api req.responseMu.RLock() defer req.responseMu.RUnlock() - return req.response + return req.response, nil } // ID returns the unique ID of a FrameManager value. diff --git a/common/page.go b/common/page.go index dd0c98da8..514825ce7 100644 --- a/common/page.go +++ b/common/page.go @@ -874,7 +874,7 @@ func (p *Page) WaitForLoadState(state string, opts goja.Value) { } // WaitForNavigation waits for the given navigation lifecycle event to happen. -func (p *Page) WaitForNavigation(opts goja.Value) api.Response { +func (p *Page) WaitForNavigation(opts goja.Value) *goja.Promise { p.logger.Debugf("Page:WaitForNavigation", "sid:%v", p.sessionID()) return p.frameManager.MainFrame().WaitForNavigation(opts) diff --git a/examples/fillform.js b/examples/fillform.js index 9fce34ce3..c7aa8b89e 100644 --- a/examples/fillform.js +++ b/examples/fillform.js @@ -15,12 +15,14 @@ export default function() { // Enter login credentials and login page.$('input[name="login"]').type('admin'); page.$('input[name="password"]').type('123'); - return page.$('input[type="submit"]').click(); + // We expect the form submission to trigger a navigation, so to prevent a + // race condition, setup a waiter concurrently while waiting for the click + // to resolve. + return Promise.all([ + page.waitForNavigation(), + page.$('input[type="submit"]').click(), + ]); }).then(() => { - // We expect the above form submission to trigger a navigation, so wait for it - // and the page to be loaded. - page.waitForNavigation(); - check(page, { 'header': page.$('h2').textContent() == 'Welcome, admin!', }); diff --git a/examples/locator.js b/examples/locator.js index 27549278c..7d1a8ad57 100644 --- a/examples/locator.js +++ b/examples/locator.js @@ -29,25 +29,33 @@ export default function () { const currentBet = page.locator("//p[starts-with(text(),'Your bet: ')]"); - // the tails locator clicks on the tails button by using the - // locator's selector. - tails.click(); + // In the following Promise.all the tails locator clicks + // on the tails button by using the locator's selector. // Since clicking on each button causes page navigation, - // waitForNavigation is needed. It's because the page + // waitForNavigation is needed -- this is because the page // won't be ready until the navigation completes. - page.waitForNavigation(); - console.log(currentBet.innerText()); - - // the heads locator clicks on the heads button by using the - // locator's selector. - heads.click(); - page.waitForNavigation(); - console.log(currentBet.innerText()); - - tails.click(); - page.waitForNavigation(); - console.log(currentBet.innerText()); - - page.close(); - browser.close(); + // Setting up the waitForNavigation first before the click + // is important to avoid race conditions. + Promise.all([ + page.waitForNavigation(), + tails.click(), + ]).then(() => { + console.log(currentBet.innerText()); + // the heads locator clicks on the heads button + // by using the locator's selector. + return Promise.all([ + page.waitForNavigation(), + heads.click(), + ]); + }).then(() => { + console.log(currentBet.innerText()); + return Promise.all([ + page.waitForNavigation(), + tails.click(), + ]); + }).finally(() => { + console.log(currentBet.innerText()); + page.close(); + browser.close(); + }) } diff --git a/examples/locator_pom.js b/examples/locator_pom.js index 9b6427f95..09492a7d8 100644 --- a/examples/locator_pom.js +++ b/examples/locator_pom.js @@ -22,13 +22,17 @@ export class Bet { } heads() { - this.headsButton.click(); - this.page.waitForNavigation(); + return Promise.all([ + this.page.waitForNavigation(), + this.headsButton.click(), + ]); } tails() { - this.tailsButton.click(); - this.page.waitForNavigation(); + return Promise.all([ + this.page.waitForNavigation(), + this.tailsButton.click(), + ]); } current() { @@ -46,18 +50,18 @@ export default function () { const bet = new Bet(page); bet.goto(); - bet.tails(); - console.log("Current bet:", bet.current()); - - bet.heads(); - console.log("Current bet:", bet.current()); - - bet.tails(); - console.log("Current bet:", bet.current()); - - bet.heads(); - console.log("Current bet:", bet.current()); - - page.close(); - browser.close(); + bet.tails().then(() => { + console.log("Current bet:", bet.current()); + return bet.heads(); + }).then(() => { + console.log("Current bet:", bet.current()); + return bet.tails(); + }).then(() => { + console.log("Current bet:", bet.current()); + return bet.heads(); + }).finally(() => { + console.log("Current bet:", bet.current()); + page.close(); + browser.close(); + }) } diff --git a/k6ext/k6test/vu.go b/k6ext/k6test/vu.go index b737561d3..027a7d8a0 100644 --- a/k6ext/k6test/vu.go +++ b/k6ext/k6test/vu.go @@ -30,6 +30,7 @@ func (v *VU) ToGojaValue(i interface{}) goja.Value { return v.Runtime().ToValue( // RunLoop is a convenience method for running fn in the event loop. func (v *VU) RunLoop(fn func() error) error { + v.Loop.WaitOnRegistered() return v.Loop.Start(fn) } diff --git a/k6ext/promise.go b/k6ext/promise.go index 8452fb2a9..2319f4a6e 100644 --- a/k6ext/promise.go +++ b/k6ext/promise.go @@ -37,18 +37,20 @@ func promise(ctx context.Context, fn PromisifiedFunc, d eventLoopDirective) *goj cb = vu.RegisterCallback() p, resolve, reject = vu.Runtime().NewPromise() ) - go cb(func() error { + go func() { v, err := fn() - if err != nil { - reject(err) - } else { - resolve(v) - } - if d == continueEventLoop { - err = nil - } - return err - }) + cb(func() error { + if err != nil { + reject(err) + } else { + resolve(v) + } + if d == continueEventLoop { + err = nil + } + return err + }) + }() return p } diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 1ab3d21c4..825c1e200 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -1,12 +1,16 @@ package tests import ( + "fmt" + "net/http" "os" "testing" "time" "github.com/grafana/xk6-browser/common" + "github.com/dop251/goja" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -16,12 +20,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { } t.Parallel() - var timeout time.Duration = 200 - if os.Getenv("CI") == "true" { - // Increase the timeout on underprovisioned CI machines to minimize - // chances of intermittent failures. - timeout *= 3 - } + var timeout time.Duration = 5000 // interpreted as ms testCases := []struct { name, selector string @@ -33,37 +32,106 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - tb := newTestBrowser(t, withFileServer()) - p := tb.NewPage(nil) - - resp := p.Goto(tb.staticURL("/nav_in_doc.html"), nil) - require.NotNil(t, resp) - - // A click right away could possibly trigger navigation before we - // had a chance to call WaitForNavigation below, so give it some - // time to simulate the JS overhead, waiting for XHR response, etc. - time.AfterFunc(timeout*time.Millisecond, func() { //nolint:durationcheck - _ = tb.await(func() error { - _ = p.Click(tc.selector, nil) - return nil - }) - }) + t.Parallel() - done := make(chan struct{}, 1) + errc := make(chan error) go func() { - require.NotPanics(t, func() { - p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: timeout * 3, // interpreted as ms + tb := newTestBrowser(t, withFileServer()) + p := tb.NewPage(nil) + + resp := p.Goto(tb.staticURL("/nav_in_doc.html"), tb.toGojaValue(&common.FrameGotoOptions{ + WaitUntil: common.LifecycleEventNetworkIdle, + Timeout: timeout, // interpreted as ms + })) + require.NotNil(t, resp) + + // Callbacks that are initiated internally by click and WaitForNavigation + // need to be called from the event loop itself, otherwise the callback + // doesn't work. The await below needs to first return before the callback + // will resolve/reject. + var wfnPromise, cPromise *goja.Promise + err := tb.await(func() error { + wfnPromise = p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ + Timeout: timeout, // interpreted as ms })) + cPromise = p.Click(tc.selector, nil) + + assert.Equal(t, goja.PromiseStatePending, wfnPromise.State()) + assert.Equal(t, goja.PromiseStatePending, cPromise.State()) + + return nil }) - done <- struct{}{} + if err != nil { + errc <- err + } + + assert.Equal(t, goja.PromiseStateFulfilled, wfnPromise.State()) + assert.Equal(t, goja.PromiseStateFulfilled, cPromise.State()) + + errc <- nil }() select { - case <-done: - case <-time.After(timeout * 5 * time.Millisecond): //nolint:durationcheck + case err := <-errc: + assert.NoError(t, err) + case <-time.After(time.Duration(int64(timeout)) * time.Millisecond): t.Fatal("Test timed out") } }) } } + +func TestWaitForFrameNavigation(t *testing.T) { + tb := newTestBrowser(t, withHTTPServer()) + p := tb.NewPage(nil) + + tb.withHandler("/first", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprintf(w, ` + + + First page + + + click me + + + `) + }) + tb.withHandler("/second", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprintf(w, ` + + + Second page + + + click me + + + `) + }) + + require.NotNil(t, p.Goto(tb.URL("/first"), tb.toGojaValue(&common.FrameGotoOptions{ + WaitUntil: common.LifecycleEventNetworkIdle, + Timeout: common.DefaultTimeout, + }))) + + var timeout time.Duration = 5000 // interpreted as ms + + var wfnPromise, cPromise *goja.Promise + err := tb.await(func() error { + wfnPromise = p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ + Timeout: timeout, // interpreted as ms + })) + cPromise = p.Click(`a`, nil) + + assert.Equal(t, goja.PromiseStatePending, wfnPromise.State()) + assert.Equal(t, goja.PromiseStatePending, cPromise.State()) + + return nil + }) + require.NoError(t, err) + + assert.Equal(t, goja.PromiseStateFulfilled, wfnPromise.State()) + assert.Equal(t, goja.PromiseStateFulfilled, cPromise.State()) + assert.Equal(t, "Second page", p.Title()) +}