From a7c68ac913f716c1896a0b700a7a576446fd4c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Fri, 22 Jul 2022 15:39:46 +0300 Subject: [PATCH 01/20] Fix k6ext.promise The callback returned from RegisterCallback should not be run as a goroutine. Since its job is to carry a task to the event loop, the task should be done in a goroutine first, and then let the task call the callback within the goroutine. --- k6ext/promise.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) 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 } From 8c17ed2dad2f2105cc84b4abbc4d1273675112c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 11:29:29 +0300 Subject: [PATCH 02/20] Refactor FrameManager.WaitForFrameNavigation --- common/frame.go | 6 +++++- common/frame_manager.go | 16 ++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/common/frame.go b/common/frame.go index a3b9123f6..4b5eed423 100644 --- a/common/frame.go +++ b/common/frame.go @@ -1822,7 +1822,11 @@ 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) + r, err := f.manager.waitForFrameNavigation(f, opts) + if err != nil { + k6ext.Panic(f.ctx, "%w", err) + } + return r } // 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. From c02e51e35534cde4d04f2a24af8d5e684aa3bd82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 11:31:44 +0300 Subject: [PATCH 03/20] Make api.Frame.WaitForNavigation async --- api/frame.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) } From 56782c514b3f3ba9faf55b9c2a1e3d3fa423f15e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 11:32:14 +0300 Subject: [PATCH 04/20] Make api.Page.WaitForNavigation async --- api/page.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 7ccd24c05ecbfb028541acfe1562f584e8306268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 11:34:53 +0300 Subject: [PATCH 05/20] Make Frame.WaitForNavigation async --- common/frame.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/common/frame.go b/common/frame.go index 4b5eed423..63896e332 100644 --- a/common/frame.go +++ b/common/frame.go @@ -1821,12 +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 { - r, err := f.manager.waitForFrameNavigation(f, opts) - if err != nil { - k6ext.Panic(f.ctx, "%w", err) - } - return r +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. From 163929444ca67bf1635418851500b273b0b8e24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 11:35:13 +0300 Subject: [PATCH 06/20] Make Page.WaitForNavigation async --- common/page.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From 7cb311cc9104c580b773124998c6e09aefcc00fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 16:58:52 +0300 Subject: [PATCH 07/20] Make fillform.js async for waitForNavigation --- examples/fillform.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/examples/fillform.js b/examples/fillform.js index 9fce34ce3..906a009b4 100644 --- a/examples/fillform.js +++ b/examples/fillform.js @@ -15,12 +15,13 @@ 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(); - }).then(() => { + }).then(() => + 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(); - + page.waitForNavigation() + ).then(() => { check(page, { 'header': page.$('h2').textContent() == 'Welcome, admin!', }); From 2b824cbe35da35ffc411589c793c13a3fe2e7167 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 17:05:54 +0300 Subject: [PATCH 08/20] Make locator.js async for waitForNavigation --- examples/locator.js | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/examples/locator.js b/examples/locator.js index 27549278c..d99f1ff12 100644 --- a/examples/locator.js +++ b/examples/locator.js @@ -35,19 +35,22 @@ export default function () { // Since clicking on each button causes page navigation, // waitForNavigation is needed. It's 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(); + page.waitForNavigation().then(() => { + console.log(currentBet.innerText()); + }).then(() => { + // the heads locator clicks on the heads button + // by using the locator's selector. + heads.click(); + return page.waitForNavigation() + }).then(() => { + console.log(currentBet.innerText()); + }).then(() => { + tails.click(); + return page.waitForNavigation() + }).then(() => { + console.log(currentBet.innerText()); + }).finally(() => { + page.close(); + browser.close(); + }) } From f2db16d3a10bce9e0aef1263aa70e7bc5f471866 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 17:06:01 +0300 Subject: [PATCH 09/20] Make locator_pom.js async for waitForNavigation --- examples/locator_pom.js | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/examples/locator_pom.js b/examples/locator_pom.js index 9b6427f95..ac95dea9c 100644 --- a/examples/locator_pom.js +++ b/examples/locator_pom.js @@ -23,12 +23,12 @@ export class Bet { heads() { this.headsButton.click(); - this.page.waitForNavigation(); + return this.page.waitForNavigation(); } tails() { this.tailsButton.click(); - this.page.waitForNavigation(); + return this.page.waitForNavigation(); } current() { @@ -46,18 +46,22 @@ 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()); + }).then(() => + bet.heads() + ).then(() => { + console.log("Current bet:", bet.current()); + }).then(() => + bet.tails() + ).then(() => { + console.log("Current bet:", bet.current()); + }).then(() => + bet.heads() + ).then(() => { + console.log("Current bet:", bet.current()); + }).finally(() => { + page.close(); + browser.close(); + }) } From 301e746c7d806b84e93c60b219973c6abc597402 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 17:07:57 +0300 Subject: [PATCH 10/20] Make README locator example async Since waitForNavigation is async now. --- README.md | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 433c316d6..70d3ff503 100644 --- a/README.md +++ b/README.md @@ -363,21 +363,24 @@ export default function () { // Since clicking on each button causes page navigation, // waitForNavigation is needed. It's 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(); + page.waitForNavigation().then(() => { + console.log(currentBet.innerText()); + }).then(() => { + // the heads locator clicks on the heads button + // by using the locator's selector. + heads.click(); + return page.waitForNavigation() + }).then(() => { + console.log(currentBet.innerText()); + }).then(() => { + tails.click(); + return page.waitForNavigation() + }).then(() => { + console.log(currentBet.innerText()); + }).finally(() => { + page.close(); + browser.close(); + }) } ``` From 59bed1e0215d46b6a8d4bbcf35705538de4b4917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Thu, 21 Jul 2022 17:20:12 +0300 Subject: [PATCH 11/20] Refactor TestWaitForFrameNavigationWithinDocument Since the Click and WaitForNavigation are now async, we don't need to call each in separate goroutines. Instead, we call them in the event loop, and the event loop will take care of async stuff for us. --- tests/frame_manager_test.go | 44 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 1ab3d21c4..2364e5032 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -33,35 +33,35 @@ 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) + t.Parallel() - resp := p.Goto(tb.staticURL("/nav_in_doc.html"), nil) - require.NotNil(t, resp) + errc := make(chan error, 1) + go func() { + tb := newTestBrowser(t, withFileServer()) + p := tb.NewPage(nil) - // 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 - }) - }) + resp := p.Goto(tb.staticURL("/nav_in_doc.html"), nil) + require.NotNil(t, resp) - done := make(chan struct{}, 1) - go func() { - require.NotPanics(t, func() { - p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: timeout * 3, // interpreted as ms + // 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.After(timeout * time.Millisecond) //nolint:durationcheck + + // if one of the promises panics, err will contain the error + errc <- tb.await(func() error { + _ = p.Click(tc.selector, nil) + _ = p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ + Timeout: 3 * timeout, // interpreted as ms })) + return nil }) - done <- struct{}{} }() - select { - case <-done: - case <-time.After(timeout * 5 * time.Millisecond): //nolint:durationcheck + case err := <-errc: + require.NoError(t, err) + case <-time.After(5 * timeout * time.Millisecond): //nolint:durationcheck t.Fatal("Test timed out") } }) From 41ae32705010912e9f0c605b5c1679af2b953533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0nan=C3=A7=20G=C3=BCm=C3=BC=C5=9F?= Date: Fri, 22 Jul 2022 07:18:33 +0300 Subject: [PATCH 12/20] Add TestWaitForFrameNavigation --- tests/frame_manager_test.go | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 2364e5032..8ec99a0b7 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -1,6 +1,8 @@ package tests import ( + "fmt" + "net/http" "os" "testing" "time" @@ -67,3 +69,45 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { }) } } + +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, + }))) + err := tb.await(func() error { + _ = p.Click(`a`, nil) + p.WaitForNavigation(nil) + return nil + }) + require.NoError(t, err) + require.Equal(t, p.Title(), "Second page") +} From 886eda55893b293ac9ae973e6ee6690cff9adb7d Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 8 Aug 2022 12:52:29 +0100 Subject: [PATCH 13/20] Update examples to use Promise.all We can now use Promise.all when we interact with an element that causes a navigation to occur, and so we need to also use waitForNavigation before carrying on to the next steps in the tests. This helps prevent race conditions between clicking and waiting. Resolves: https://github.com/grafana/xk6-browser/pull/467#discussion_r928722021 --- README.md | 26 +++++++++++++++++--------- examples/fillform.js | 15 ++++++++------- examples/locator.js | 26 +++++++++++++++++--------- examples/locator_pom.js | 12 ++++++++---- 4 files changed, 50 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 70d3ff503..a45d90864 100644 --- a/README.md +++ b/README.md @@ -357,24 +357,32 @@ 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().then(() => { + // 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()); }).then(() => { // the heads locator clicks on the heads button // by using the locator's selector. - heads.click(); - return page.waitForNavigation() + return Promise.all([ + page.waitForNavigation(), + heads.click(), + ]); }).then(() => { console.log(currentBet.innerText()); }).then(() => { - tails.click(); - return page.waitForNavigation() + return Promise.all([ + page.waitForNavigation(), + tails.click(), + ]); }).then(() => { console.log(currentBet.innerText()); }).finally(() => { diff --git a/examples/fillform.js b/examples/fillform.js index 906a009b4..c7aa8b89e 100644 --- a/examples/fillform.js +++ b/examples/fillform.js @@ -15,13 +15,14 @@ export default function() { // Enter login credentials and login page.$('input[name="login"]').type('admin'); page.$('input[name="password"]').type('123'); - }).then(() => - 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() - ).then(() => { + // 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(() => { check(page, { 'header': page.$('h2').textContent() == 'Welcome, admin!', }); diff --git a/examples/locator.js b/examples/locator.js index d99f1ff12..6455dc49a 100644 --- a/examples/locator.js +++ b/examples/locator.js @@ -29,24 +29,32 @@ 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().then(() => { + // 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()); }).then(() => { // the heads locator clicks on the heads button // by using the locator's selector. - heads.click(); - return page.waitForNavigation() + return Promise.all([ + page.waitForNavigation(), + heads.click(), + ]); }).then(() => { console.log(currentBet.innerText()); }).then(() => { - tails.click(); - return page.waitForNavigation() + return Promise.all([ + page.waitForNavigation(), + tails.click(), + ]); }).then(() => { console.log(currentBet.innerText()); }).finally(() => { diff --git a/examples/locator_pom.js b/examples/locator_pom.js index ac95dea9c..3b9537614 100644 --- a/examples/locator_pom.js +++ b/examples/locator_pom.js @@ -22,13 +22,17 @@ export class Bet { } heads() { - this.headsButton.click(); - return this.page.waitForNavigation(); + return Promise.all([ + this.page.waitForNavigation(), + this.headsButton.click(), + ]); } tails() { - this.tailsButton.click(); - return this.page.waitForNavigation(); + return Promise.all([ + this.page.waitForNavigation(), + this.tailsButton.click(), + ]); } current() { From 331465f855fb0dfad0ba05b267c4ec8cac20fd12 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Mon, 8 Aug 2022 17:27:27 +0100 Subject: [PATCH 14/20] Refactor test to check promise states The test now checks the state of the promise before passing the test. Resolves: https://github.com/grafana/xk6-browser/pull/467#discussion_r932044728 --- tests/frame_manager_test.go | 39 +++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 8ec99a0b7..83ad5711a 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -9,6 +9,8 @@ import ( "github.com/grafana/xk6-browser/common" + "github.com/dop251/goja" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -18,7 +20,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { } t.Parallel() - var timeout time.Duration = 200 + timeout := 2000 * time.Millisecond if os.Getenv("CI") == "true" { // Increase the timeout on underprovisioned CI machines to minimize // chances of intermittent failures. @@ -37,7 +39,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - errc := make(chan error, 1) + errc := make(chan error) go func() { tb := newTestBrowser(t, withFileServer()) p := tb.NewPage(nil) @@ -45,25 +47,32 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { 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.After(timeout * time.Millisecond) //nolint:durationcheck - - // if one of the promises panics, err will contain the error - errc <- tb.await(func() error { - _ = p.Click(tc.selector, nil) - _ = p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ - Timeout: 3 * timeout, // interpreted as ms + // 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) return nil }) + if err != nil { + errc <- err + } + + assert.Equal(t, goja.PromiseStateFulfilled, wfnPromise.State()) + assert.Equal(t, goja.PromiseStateFulfilled, cPromise.State()) + + errc <- nil }() + select { case err := <-errc: - require.NoError(t, err) - case <-time.After(5 * timeout * time.Millisecond): //nolint:durationcheck + assert.NoError(t, err) + case <-time.After(timeout): t.Fatal("Test timed out") } }) From e118dc37b12e71535085eb107cc8a069aa8281db Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 9 Aug 2022 10:59:57 +0100 Subject: [PATCH 15/20] Remove extra promise handler chaining Resolves: https://github.com/grafana/xk6-browser/pull/467#discussion_r931985383 --- README.md | 5 +---- examples/locator.js | 5 +---- examples/locator_pom.js | 16 ++++++---------- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index a45d90864..6cde7426f 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,6 @@ export default function () { tails.click(), ]).then(() => { console.log(currentBet.innerText()); - }).then(() => { // the heads locator clicks on the heads button // by using the locator's selector. return Promise.all([ @@ -378,14 +377,12 @@ export default function () { ]); }).then(() => { console.log(currentBet.innerText()); - }).then(() => { return Promise.all([ page.waitForNavigation(), tails.click(), ]); - }).then(() => { - console.log(currentBet.innerText()); }).finally(() => { + console.log(currentBet.innerText()); page.close(); browser.close(); }) diff --git a/examples/locator.js b/examples/locator.js index 6455dc49a..7d1a8ad57 100644 --- a/examples/locator.js +++ b/examples/locator.js @@ -41,7 +41,6 @@ export default function () { tails.click(), ]).then(() => { console.log(currentBet.innerText()); - }).then(() => { // the heads locator clicks on the heads button // by using the locator's selector. return Promise.all([ @@ -50,14 +49,12 @@ export default function () { ]); }).then(() => { console.log(currentBet.innerText()); - }).then(() => { return Promise.all([ page.waitForNavigation(), tails.click(), ]); - }).then(() => { - console.log(currentBet.innerText()); }).finally(() => { + console.log(currentBet.innerText()); page.close(); browser.close(); }) diff --git a/examples/locator_pom.js b/examples/locator_pom.js index 3b9537614..09492a7d8 100644 --- a/examples/locator_pom.js +++ b/examples/locator_pom.js @@ -52,19 +52,15 @@ export default function () { bet.tails().then(() => { console.log("Current bet:", bet.current()); - }).then(() => - bet.heads() - ).then(() => { + return bet.heads(); + }).then(() => { console.log("Current bet:", bet.current()); - }).then(() => - bet.tails() - ).then(() => { - console.log("Current bet:", bet.current()); - }).then(() => - bet.heads() - ).then(() => { + return bet.tails(); + }).then(() => { console.log("Current bet:", bet.current()); + return bet.heads(); }).finally(() => { + console.log("Current bet:", bet.current()); page.close(); browser.close(); }) From 3054bb37929e37ee2573f51d409e169359216327 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Tue, 9 Aug 2022 11:26:44 +0100 Subject: [PATCH 16/20] Refactor test so that we're checking the promise We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: https://github.com/grafana/xk6-browser/pull/467#discussion_r932044728 --- tests/frame_manager_test.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 83ad5711a..2c3fba18a 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -20,7 +20,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { } t.Parallel() - timeout := 2000 * time.Millisecond + var timeout time.Duration = 2000 // interpreted as ms if os.Getenv("CI") == "true" { // Increase the timeout on underprovisioned CI machines to minimize // chances of intermittent failures. @@ -57,6 +57,10 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { 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 }) if err != nil { @@ -72,7 +76,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { select { case err := <-errc: assert.NoError(t, err) - case <-time.After(timeout): + case <-time.After(timeout * time.Millisecond): t.Fatal("Test timed out") } }) @@ -112,11 +116,20 @@ func TestWaitForFrameNavigation(t *testing.T) { WaitUntil: common.LifecycleEventNetworkIdle, Timeout: common.DefaultTimeout, }))) + + var wfnPromise, cPromise *goja.Promise err := tb.await(func() error { - _ = p.Click(`a`, nil) - p.WaitForNavigation(nil) + wfnPromise = p.WaitForNavigation(nil) + 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) - require.Equal(t, p.Title(), "Second page") + + assert.Equal(t, goja.PromiseStateFulfilled, wfnPromise.State()) + assert.Equal(t, goja.PromiseStateFulfilled, cPromise.State()) + assert.Equal(t, "Second page", p.Title()) } From 61b84ee2a604ba405d2c1f8be457dc838de9d118 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Thu, 11 Aug 2022 16:16:31 +0100 Subject: [PATCH 17/20] Add WaitOnRegistered before Starting eventloop This will ensure that the envetloop has complete before starting it up again. We're going to avoid placing lock here, since we want to follow the eventloop API and not allow multiple threads accessing the one eventloop. --- k6ext/k6test/vu.go | 1 + 1 file changed, 1 insertion(+) 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) } From d7e15c55ec908845b52efa3785068acc0e476528 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Thu, 11 Aug 2022 16:25:48 +0100 Subject: [PATCH 18/20] Remove the CI specific timeout We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: https://github.com/grafana/xk6-browser/pull/467#discussion_r942230521 --- tests/frame_manager_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 2c3fba18a..3317754e5 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -20,12 +20,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { } t.Parallel() - var timeout time.Duration = 2000 // interpreted as ms - 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 @@ -76,7 +71,7 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { select { case err := <-errc: assert.NoError(t, err) - case <-time.After(timeout * time.Millisecond): + case <-time.After(time.Duration(int64(timeout)) * time.Millisecond): t.Fatal("Test timed out") } }) From fac54bdb6482191b444fe636a6d18769d6a042de Mon Sep 17 00:00:00 2001 From: ankur22 Date: Thu, 11 Aug 2022 16:35:03 +0100 Subject: [PATCH 19/20] Add timeout to WaitForNavigation --- tests/frame_manager_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index 3317754e5..d10320409 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -112,9 +112,13 @@ func TestWaitForFrameNavigation(t *testing.T) { Timeout: common.DefaultTimeout, }))) + var timeout time.Duration = 5000 // interpreted as ms + var wfnPromise, cPromise *goja.Promise err := tb.await(func() error { - wfnPromise = p.WaitForNavigation(nil) + wfnPromise = p.WaitForNavigation(tb.toGojaValue(&common.FrameWaitForNavigationOptions{ + Timeout: timeout, // interpreted as ms + })) cPromise = p.Click(`a`, nil) assert.Equal(t, goja.PromiseStatePending, wfnPromise.State()) From 3ea0c2557340864f9190a0f0c46625dbef2cb523 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Thu, 11 Aug 2022 17:20:26 +0100 Subject: [PATCH 20/20] Add timeout to Goto --- tests/frame_manager_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/frame_manager_test.go b/tests/frame_manager_test.go index d10320409..825c1e200 100644 --- a/tests/frame_manager_test.go +++ b/tests/frame_manager_test.go @@ -39,7 +39,10 @@ func TestWaitForFrameNavigationWithinDocument(t *testing.T) { tb := newTestBrowser(t, withFileServer()) p := tb.NewPage(nil) - resp := p.Goto(tb.staticURL("/nav_in_doc.html"), 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