Bug 174497 - Web Automation: FindNodes should throw an error in case of invalid strategy
Summary: Web Automation: FindNodes should throw an error in case of invalid strategy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-14 00:50 PDT by Carlos Garcia Campos
Modified: 2017-07-17 00:01 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2017-07-14 00:55 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (3.55 KB, patch)
2017-07-14 01:45 PDT, Carlos Garcia Campos
bburg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (1.73 MB, application/zip)
2017-07-14 03:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (991.20 KB, application/zip)
2017-07-14 04:17 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-07-14 00:50:46 PDT
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'>
Comment 1 Carlos Garcia Campos 2017-07-14 00:55:59 PDT
Created attachment 315408 [details]
Patch
Comment 2 Carlos Garcia Campos 2017-07-14 01:45:20 PDT
Created attachment 315411 [details]
Patch
Comment 3 Build Bot 2017-07-14 03:03:13 PDT
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
Comment 4 Build Bot 2017-07-14 03:03:15 PDT
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
Comment 5 Build Bot 2017-07-14 04:17:30 PDT
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
Comment 6 Build Bot 2017-07-14 04:17:31 PDT
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
Comment 7 BJ Burg 2017-07-14 08:37:21 PDT
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.
Comment 8 Carlos Garcia Campos 2017-07-14 09:08:51 PDT
(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;
Comment 9 Carlos Garcia Campos 2017-07-17 00:01:22 PDT
Committed r219554: <http://trac.webkit.org/changeset/219554>