1204504 - Illegal exception is thrown on an invalid locator

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Alexei Barantsev]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.85 Safari/537.36

Steps to reproduce:

Search by an invalid locator


Actual results:

General webdriver error occurs:

INFO: Executing: findElement [e6e35c7c-0cff-4d9e-96f0-94a4a00cf6f4, findElement {using=css selector, value=#}]
1442239082133	Marionette	DEBUG	conn0 -> {"name":"findElement","parameters":{"using":"css selector","value":"#"},"sessionId":"e6e35c7c-0cff-4d9e-96f0-94a4a00cf6f4"}
1442239082339	Marionette	DEBUG	conn0 client <- {"error":"webdriver error","message":"An invalid or illegal string was specified","stacktrace":null}
Marionette threw an error: SyntaxError: An invalid or illegal string was specified
EM_findElement@chrome://marionette/content/elements.js:620:19
EM_find@chrome://marionette/content/elements.js:485:23
findElementContent/<@chrome://marionette/content/listener.js:1403:1
findElementContent@chrome://marionette/content/listener.js:1402:1
dispatch/</req<@chrome://marionette/content/listener.js:165:22
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Task_spawn@resource://gre/modules/Task.jsm:164:12
dispatch/<@chrome://marionette/content/listener.js:160:15



Expected results:

Invalid selector error should be returned
http://www.w3.org/TR/webdriver/#handling-errors
http://www.w3.org/TR/webdriver/#element-location-strategies

Comment 1

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=853840d0f3fe

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1204504 - Error on invalid selector; r=automatedtester

Bug 1204504: Wrap invalid CSS query error in invalid selector error

Thanks to Alexei Barantsev for reporting.

r=dburns

Comment 3

[David Burns :automatedtester]

Comment on attachment 8662332 [details]
MozReview Request: Bug 1204504 - Error on invalid selector; r=automatedtester

https://reviewboard.mozilla.org/r/19579/#review17541

::: testing/marionette/client/marionette/tests/unit/test_findelement.py:84
(Diff revision 1)
> +    def test_invalid_css_selector(self):

Please make the name slightly more descriptive.

::: testing/marionette/client/marionette/tests/unit/test_findelement.py:85
(Diff revision 1)
> +        with self.assertRaises(InvalidSelectorException):

This can be done on one line

::: testing/marionette/client/marionette/tests/unit/test_findelement.py:163
(Diff revision 1)
> -        test_html = self.marionette.absolute_url("test.html")
> +        with self.assertRaises(InvalidSelectorException):

This can be done on one line

Update the minor issues and good to go.

Comment 4

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/19579/#review17545

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8662332 [details]
MozReview Request: Bug 1204504 - Error on invalid selector; r=automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19579/diff/1-2/

Comment 6

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/19579/#review17541

> This can be done on one line

I find the one line version, where you pass the function reference and its arguments, and unittest will invoke it for you more unreadable because you need to understand what unittest does with it.

Doing it like this is clearer and more understandable, in my opinion, because it removes any doubt what is actually executed.

> This can be done on one line

I don’t particularly like the reflection style API that assertRaises provides to make this a single line.

IMO it adds an additional layer of abstraction that is unnecessary.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7025fe68e90

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7025fe68e90