Closed Bug 1241067 Opened 4 years ago Closed 4 years ago

Invalid Locators can lead to handled exceptions

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

Details

Attachments

(2 files)

There are about 12 tests in the Selenium test suite that fail because of this.
This allows us to now handle exceptions and return the appropriate
exceptions to the clients

Review commit: https://reviewboard.mozilla.org/r/31721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31721/
Attachment #8710321 - Flags: review?(ato)
Attachment #8710321 - Flags: review?(ato)
Comment on attachment 8710321 [details]
MozReview Request: Bug 1241067: Handle Invalid Selector values when finding elements r?ato

https://reviewboard.mozilla.org/r/31721/#review28457

You should point out that both invalid selectors strategies _and_ expressions are handled in the commit message.  You should also use the terminology “error” rather than “exception”, since this is a client side concept.

I also see you’re missing a test for invalid strategy.

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

Can you please shorten these test names and run the file through PEP8?

::: testing/marionette/elements.js:339
(Diff revision 1)
>    find: function EM_find(container, values, searchTimeout, all, on_success, on_error, command_id) {

You should also test if `values.value` is null or has a length of 0, and return an error in that case.

::: testing/marionette/elements.js:347
(Diff revision 1)
> -    let found = all ? this.findElements(values.using, values.value, rootNode, startNode) :
> +    let found = undefined;

`undefined` is implied.

::: testing/marionette/elements.js:352
(Diff revision 1)
> +      // Invalid Selectors throw when we try use them

This comment is superfluous.

::: testing/marionette/elements.js:353
(Diff revision 1)
> +      throw new InvalidSelectorError(`Invalid selectors were used: ${values.using} : ${values.value}`);

I think `"Invalid locator strategy: " + values.using` would be nicer.  No need to include the value here.

::: testing/marionette/elements.js:435
(Diff revision 1)
> +    } catch (e) {
> +      // do nothing, we have had a bad XPath passed in.
> +    }

This should probably return early because `iterateNext` isn’t a function on `[]`.
Attachment #8710322 - Flags: review?(ato) → review+
Comment on attachment 8710322 [details]
MozReview Request: Bug 1241067: Change string concats to use string templating. r?ato

https://reviewboard.mozilla.org/r/31723/#review28461
https://reviewboard.mozilla.org/r/31721/#review28457

Invalid selectors are already handled in the code. See https://dxr.mozilla.org/mozilla-central/source/testing/marionette/elements.js#344. The change is for when we throw due to poor values as the commit says.

> Can you please shorten these test names and run the file through PEP8?

I have shortened them but there are numerous PEP8 violations that have always been there that should be done in a separate commit. We also need to agree on what parts of PEP8 are useful. Line lengths are not since we get given big screens.

> You should also test if `values.value` is null or has a length of 0, and return an error in that case.

`""` is valid for all but XPATH and CSS. Null check added

> I think `"Invalid locator strategy: " + values.using` would be nicer.  No need to include the value here.

I disagree. Telling people this is wrong and not showing them the value that is wrong doesnt help them correct.

> This should probably return early because `iterateNext` isn’t a function on `[]`.

This was a change I meant to remove. Have fixed.
Comment on attachment 8710321 [details]
MozReview Request: Bug 1241067: Handle Invalid Selector values when finding elements r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31721/diff/1-2/
Attachment #8710321 - Flags: review?(ato)
Comment on attachment 8710322 [details]
MozReview Request: Bug 1241067: Change string concats to use string templating. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31723/diff/1-2/
Attachment #8710321 - Flags: review?(ato) → review+
Comment on attachment 8710321 [details]
MozReview Request: Bug 1241067: Handle Invalid Selector values when finding elements r?ato

https://reviewboard.mozilla.org/r/31721/#review28489

LGTM.  Ship it when you’re confident the tests are good.

::: testing/marionette/client/marionette/tests/unit/test_findelement.py:185
(Diff revision 2)
> +    def test_should_throw_invalidSelectorException_when_invalid_xPath_in_driver_find_element(self):

s/invalidSelectorException/invalidselectorexception/ to match other test names.

::: testing/marionette/client/marionette/tests/unit/test_findelement.py:190
(Diff revision 2)
> +    def test_should_throw_invalidSelectorException_when_invalid_xpath_in_driver_find_elements(self):

s/invalidSelectorException/invalidselectorexception/ to match other test names.

::: testing/marionette/elements.js:347
(Diff revision 2)
> -    let found = all ? this.findElements(values.using, values.value, rootNode, startNode) :
> +    if (!values.value == null) {

This seems wrong.  First you probably don’t want to negate the value, and secondly you want to do explicit null checks with `===`.

You also need to test for string length.

::: testing/marionette/elements.js:348
(Diff revision 2)
> +      throw new InvalidSelectorError("Invalid selector value of null was used.");

It would be nicer if this could say “Selector expression cannot be null or empty” or something similar.

::: testing/marionette/elements.js:434
(Diff revision 2)
> -    let values = root.evaluate(value, node, null,
> +    let values = [];
> +    values = root.evaluate(value, node, null,

You can assign to `values` directly here.
https://reviewboard.mozilla.org/r/31721/#review28457

> I have shortened them but there are numerous PEP8 violations that have always been there that should be done in a separate commit. We also need to agree on what parts of PEP8 are useful. Line lengths are not since we get given big screens.

That’s probably fine then.

> `""` is valid for all but XPATH and CSS. Null check added

You’re right.  Reading the spec again, it doesn’t actually mandate the null test for anything but XPath.

> I disagree. Telling people this is wrong and not showing them the value that is wrong doesnt help them correct.

I was confusing this with the strategy.  You’re right.

But the message is still not good enough.  You can only have one selector, so “selectors” should be “selector”.

I suggest: `Given XPath expression "//foo" is invalid`
Comment on attachment 8710321 [details]
MozReview Request: Bug 1241067: Handle Invalid Selector values when finding elements r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31721/diff/2-3/
Comment on attachment 8710322 [details]
MozReview Request: Bug 1241067: Change string concats to use string templating. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31723/diff/2-3/
Backed out the invalid selector part (left the string templates in the tree) for failing Marionette tests:

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/429e6d01ae22

Failing job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1b87e7c116b7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=20217441&repo=mozilla-inbound

09:53:50     INFO -  TEST-START | test_anonymous_content.py TestAnonymousContent.test_find_anonymous_children
09:53:50     INFO -  Failed to gather test failure debug.
09:53:50     INFO -  Traceback (most recent call last):
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette/runner/base.py", line 591, in gather_debug
09:53:50     INFO -      rv['source'] = marionette.page_source
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1300, in page_source
09:53:50     INFO -      return self._send_message("getPageSource", key="value")
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
09:53:50     INFO -      return func(*args, **kwargs)
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 747, in _send_message
09:53:50     INFO -      self._handle_error(err)
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 808, in _handle_error
09:53:50     INFO -      raise errors.lookup(error)(message, stacktrace=stacktrace)
09:53:50     INFO -  MarionetteException: MarionetteException: Can not send call to listener as it doesnt exist
09:53:50    ERROR -  TEST-UNEXPECTED-ERROR | test_anonymous_content.py TestAnonymousContent.test_find_anonymous_children | InvalidSelectorException: InvalidSelectorException: Invalid selector value of null was used.
09:53:50     INFO -  Traceback (most recent call last):
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette/marionette_test.py", line 344, in run
09:53:50     INFO -      testMethod()
09:53:50     INFO -    File "/builds/slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit/test_anonymous_content.py", line 57, in test_find_anonymous_children
09:53:50     INFO -      self.assertEquals(HTMLElement, type(el.find_element("anon", None)))
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 52, in find_element
09:53:50     INFO -      return self.marionette.find_element(method, target, self.id)
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1735, in find_element
09:53:50     INFO -      return self._send_message("findElement", body, key="value")
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
09:53:50     INFO -      return func(*args, **kwargs)
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 747, in _send_message
09:53:50     INFO -      self._handle_error(err)
09:53:50     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 808, in _handle_error
09:53:50     INFO -      raise errors.lookup(error)(message, stacktrace=stacktrace)
09:53:50     INFO -  TEST-INFO took 603ms
Flags: needinfo?(dburns)
I removed the issues that caused the test to fail which was a request review request item and pushed again.
Flags: needinfo?(dburns)
https://hg.mozilla.org/mozilla-central/rev/1b87e7c116b7
https://hg.mozilla.org/mozilla-central/rev/efa0471875af
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.