Added a catching of the SahiClient exception to wrap it in DriverException #42
Conversation
| @@ -268,7 +277,11 @@ public function getText($xpath) | |||
| */ | |||
| public function getHtml($xpath) | |||
| { | |||
| return $this->client->findByXPath($xpath)->getHTML(); | |||
| try { | |||
| return $this->client->findByXPath($xpath)->getHTML(); | |||
There was a problem hiding this comment.
Can't you just wrap $this->client->findByXPath($xpath) call into helper method that will to try/catch as with other drivers?
There was a problem hiding this comment.
$this->client->findByXPath($xpath) does not call Sahi yet. It gives you an accessor object, which will be represented as _sahi._byXPath($xpath) when performing operations on it. This is precisely why building a good error handling is a pain (we would have to build more complex JS code in SahiClient by embedding error handling in the JS we send)
There was a problem hiding this comment.
But SahiClient is already giving an exception back. And it's not SahiClient business to return DriverException instead. Let's keep the proposed (in this PR) code then.
There was a problem hiding this comment.
no. SahiClient gives you an exception back in getHTML because of the JS exception when accessing .innerHTML on undefined when running _sahi._byXPath($xpath).innerHTML. There is no exception in $this->client->findByXPath($xpath), which just instantiate a ByXPathAccessor object and returns it.
There was a problem hiding this comment.
Can we somehow invert the process to make error handling easier without a full SahiClient API rewrite?
There was a problem hiding this comment.
So by placing try/catch around $this->client->findByXPath($xpath)->getHTML(); we ensure that we catch exception for 2 cases:
- the element can't be found
- unable to get HTML of element
I wonder if handle both cases also in Selenium2Driver, where we only try-catch around the filterByXpath.
There was a problem hiding this comment.
well, being unable to get the html of an existing element would mean that something is broken in you Selenium stack IMO. Catching such case is not really necessary.
I would prefer giving a finer error handling for Sahi, but this is not possible unfortunately (this try/catch currently even catches cases where Sahi is down for instance, because there is no finer checks)
There was a problem hiding this comment.
well, being unable to get the html of an existing element would mean that something is broken in you Selenium stack IMO. Catching such case is not really necessary.
That maybe was a bad example. But if you attempt to check the DIV element, then the DriverException should be raised in every driver.
I would prefer giving a finer error handling for Sahi, but this is not possible unfortunately (this try/catch currently even catches cases where Sahi is down for instance, because there is no finer checks)
Then let's keep current checks and then (in other PR) introduce more fine grained exception in SahiClient.
There was a problem hiding this comment.
But if you attempt to check the DIV element, then the DriverException should be raised in every driver.
Indeed. And it is one of the remaining test failures. But the fix is different so I will do it in a separate PR for readability (this PR is changing most methods to catch the exception, so changing some logic in them would be hard to review)
|
@aik099 Are you OK for merging this as it fixes lots of tests for the driver ? I'm afraid we won't be able to provide nicer error messages here. Once done, I will try to look at the remaining testsuite failures |
|
Sure. Go ahead and merge. |
Added a catching of the SahiClient exception to wrap it in DriverException
Refs https://github.com/Behat/Mink/issues/473
The error message are not detailed because Sahi does not let SahiClient handle element not founds in a nice way (
findByXPathonly gives you an accessor for the element, it does not perform any JS action to check whether this XPath matches)