Bug 174497 – Web Automation: FindNodes should throw an error in case of invalid strategy

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are currently returning null or empty list. According to the spec in 12.2 Find Element and 12.3 Find Elements, step 4.

"If location strategy is not present as a keyword in the table of location strategies, return error with error code invalid argument."

This is causing test test_should_throw_an_error_if_user_passes_in_invalid_by_when_find_elements to fail.

FAIL selenium/webdriver/common/children_finding_tests.py::test_should_throw_an_error_if_user_passes_in_invalid_by_when_find_elements[WebKitGTK]

========================================================================================== FAILURES ==========================================================================================
___________________________________________________ test_should_throw_an_error_if_user_passes_in_invalid_by_when_find_elements[WebKitGTK] ____________________________________________________

driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="4d2ddead-0495-4dcb-bccd-6e8182b7b291")>, pages = <conftest.Pages object at 0x7f1957b53490>

    def test_should_throw_an_error_if_user_passes_in_invalid_by_when_find_elements(driver, pages):
        pages.load("nestedElements.html")
        element = driver.find_element_by_name("form2")
        with pytest.raises(WebDriverException):
>           element.find_elements("foo", "bar")
E           Failed: DID NOT RAISE <class 'selenium.common.exceptions.WebDriverException'>

[Carlos Garcia Campos]

Created attachment 315408 [details]
Patch

[Carlos Garcia Campos]

Created attachment 315411 [details]
Patch

[Build Bot]

Comment on attachment 315411 [details]
Patch

Attachment 315411 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4118086

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315413 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315411 [details]
Patch

Attachment 315411 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4118475

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315416 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Blaze Burg]

Comment on attachment 315411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315411&action=review

r=me

This does not affect safaridriver because it validates the strategy argument prior to calling the atom. But this shouldn't do any harm, either.

> Source/WebKit/UIProcess/Automation/atoms/FindNodes.js:55
> +        // 12.2 Find Element and 12.3 Find Elements: If location strategy is not present as a keyword

Please write section numbers like §12.2 if possible. and mention step 4. I don't think you need to include the URL here if it's in the commit message for the change.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:277
> +        else if (exceptionName->string() == "InvalidParameter")

As long as this maps to "invalid argument", then this should be fine to reuse.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #7)
> Comment on attachment 315411 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315411&action=review
> 
> r=me
> 
> This does not affect safaridriver because it validates the strategy argument
> prior to calling the atom. But this shouldn't do any harm, either.

I thought about doing the same, but since the atom is already doing the check and it's actually a corner case in the end, it's simpler to let the atom handle it.

> > Source/WebKit/UIProcess/Automation/atoms/FindNodes.js:55
> > +        // 12.2 Find Element and 12.3 Find Elements: If location strategy is not present as a keyword
> 
> Please write section numbers like §12.2 if possible. and mention step 4. I
> don't think you need to include the URL here if it's in the commit message
> for the change.

Ok. I'll have to update the web driver patch to use § too . . . 

> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:277
> > +        else if (exceptionName->string() == "InvalidParameter")
> 
> As long as this maps to "invalid argument", then this should be fine to
> reuse.

Yes, I have this in web driver:

if (errorName == "MissingParameter" || errorName == "InvalidParameter")
    m_errorCode = ErrorCode::InvalidArgument;

[Carlos Garcia Campos]

Committed r219554: <http://trac.webkit.org/changeset/219554>