playwright-go icon indicating copy to clipboard operation
playwright-go copied to clipboard

`Page.WaitForURL` / `Frame.WaitForURL` ignores default options

Open digiscape-bnu opened this issue 3 years ago • 0 comments

Hello,

frameImpl.WaitForURL and pageImpl.WaitForURL are faulty, as the pageImpl.WaitForURL calls the underlying frameImpl.WaitForURL, and the frame implementation only calls the actual wait if (non-mandatory) options are provided:

func (f *frameImpl) WaitForURL(url string, options ...FrameWaitForURLOptions) error {
	if len(options) > 0 {
		if _, err := f.WaitForNavigation(PageWaitForNavigationOptions{
			URL:       url,
			Timeout:   options[0].Timeout,
			WaitUntil: options[0].WaitUntil,
		}); err != nil {
			return err
		}
	}
	return nil
}

This in turn leads to "standard calls" being exited immediately without any wait, and causing unexpected behaviors in higher-level applications. However, the function signature in theory only requires a URL as a string, and my expectation would be that default timeouts, etc. are applied as fallbacks, as is the case with other comparable wait implementations, such as pageImpl.WaitForRequest:

func (p *pageImpl) WaitForRequest(url interface{}, options ...interface{}) Request {
	var matcher *urlMatcher
	if url != nil {
		matcher = newURLMatcher(url)
	}
	predicate := func(req *requestImpl) bool {
		if matcher != nil {
			return matcher.Matches(req.URL())
		}
		if len(options) == 1 {
			return reflect.ValueOf(options[0]).Call([]reflect.Value{reflect.ValueOf(req)})[0].Bool()
		}
		return true
	}
	return p.WaitForEvent("request", predicate).(*requestImpl)
}

Additionally, the url parameter of WaitForURL is defined as a string, which does not allow the use of regular expressions or other variants, contrary to the official JS documentation (see https://playwright.dev/docs/api/class-page#page-wait-for-url). As far as I can tell, this shouldn't be difficult to change, as the underlying frame implementation (f.WaitForNavigation) already allows for the expected / documented variants.

I have written a standalone, buildable, reproducible example that can be used for debugging, depending on contribution standards it should be trivial to replace panics with asserts, etc if a debuggable test implementation is desired:

package main

import (
	"fmt"
	"strings"

	"github.com/playwright-community/playwright-go"
)

func ptrB(value bool) *bool {
	return &value
}

func panicErr(forErr error) {
	if forErr == nil {
		return
	}
	panic(forErr)
}

func main() {
	handle, handleErr := playwright.Run()
	panicErr(handleErr)

	browser, launchErr := handle.Chromium.Launch(playwright.BrowserTypeLaunchOptions{Headless: ptrB(false)})
	panicErr(launchErr)

	context, contextErr := browser.NewContext()
	panicErr(contextErr)

	page, pageErr := context.NewPage()
	panicErr(pageErr)

	_, navErr := page.Goto("https://www.w3schools.com/html/html_forms.asp")
	panicErr(navErr)

	cookieBtnLocator, cookieBtnErr := page.Locator(`#accept-choices`)
	panicErr(cookieBtnErr)

	clickErr := cookieBtnLocator.Click()
	panicErr(clickErr)

	buttonLocator, buttonLocatorErr := page.Locator(`input[type="submit"]`)
	panicErr(buttonLocatorErr)

	clickErr = buttonLocator.Click()
	panicErr(handleErr)

	waitErr := page.WaitForURL("INVALID")

	if waitErr == nil {
		panic("Invalid url should cause default timeout!")
	}

	fmt.Printf("Expected Timeout error? %v", strings.Contains(waitErr.Error(), "Timeout"))
}

I would have submitted a pull request directly, but due to current time / capacity constraints I can't go any farther than this.

Thanks so much in advance, BNu

digiscape-bnu avatar Jul 18 '22 14:07 digiscape-bnu