Python: phantomjs: fix usage of WebDriverException

[blueyed]

In case of an exception from subprocess.Popen it would pass the
exception as positional argument for screen.

This fixes it to use the exception in the message.

From https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/common/exceptions.py:

class WebDriverException(Exception):
    """
    Base webdriver exception.
    """

    def __init__(self, msg=None, screen=None, stacktrace=None):
        self.msg = msg
        self.screen = screen
        self.stacktrace = stacktrace

    def __str__(self):
        exception_msg = "Message: %s\n" % self.msg
        if self.screen is not None:
            exception_msg += "Screenshot: available via screen\n"
        if self.stacktrace is not None:
            stacktrace = "\n".join(self.stacktrace)
            exception_msg += "Stacktrace:\n%s" % stacktrace
        return exception_msg

So it seems like it is not used correctly here?!

        raise WebDriverException("Unable to start phantomjs with ghostdriver.", e)

(Source: https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/phantomjs/service.py#L78)

Another commit improves the message when it cannot connect to GhostDriver.

[lukeis]

yes, this isn't doing the right thing, but your change isn't either... use the kwargs and call it with WebDriverException(msg="...", stacktrace=traceback.format_exc())

[blueyed]

@lukeis
Thanks for your feedback.
stacktrace does not appear to be used in other places where the exception is raised?
(neither is msg being used as kwargs)
I will push a fixup/squash-commit for this though.

[lukeis]

it was originally put in there to handle stacktraces that would be returned by the remote end (the browser drivers themselves) and then put into the python Exception.

This is essentially piggy-backing on that 'feature' of the WebDriverException class.

[blueyed]

But why does it have to add the traceback to the msg?
It will be available with the exception itself..

[lukeis]

if you raise a new exception here, you will lose the originating traceback. The way as currently coded will show the full traceback when either the user prints it or their test / application quits because they don't handle the exception.

[blueyed]

The exception will be from subprocess.Popen, which is in the same place.
Adding the traceback to the message will it make more noisy (basically duplicating the traceback information).

[lukeis]

also... have you signed the cla?

[blueyed]

also... have you signed the cla?

Just did so.
The PR is not good to merge currently though - needs discussion and cleanup.

[lukeis]

looks good to merge to me? let me know if you still have hessitations

[blueyed]

See my comment above, but I might be missing something.
I've squashed the commit therefore, and if you think it's good like this than it can be merged.
Otherwise I'll revert the change (and re-push).

[lukeis]

So, it needs one more tweak.

This gist shows what I was trying to talk about:
https://gist.github.com/lukeis/34f1691e759804acea89

if we don't include the traceback, we lose the information about the originating exception (the 'as_is.py' one.) While if we include the stacktrace= in it, we'll also get the 'root' cause exception (the 'modified.py' in the gist).

[blueyed]

The only information being "lost" is the difference in the line number, isn't it?

[blueyed]

Ok, that would be different in a real world example.

However it still seems unnecessary to me to duplicate the traceback.

Maybe six.reraise should be used?

(But then this place is still not in line with the other places throwing that exception.)

[isaulv]

Did you test this on Python 3?

[blueyed]

@Dude-x
IIRC I was using Python 3 for the project when I've found it, but don't remember.

Is there something Python 2/3 specific in there?

What's your opinion/feedback on the previous discussion?

[isaulv]

Python 3 actually keeps track of the exception call chain, which is why I asked. If you have tested it in Python 3, then test the response in Python 2.

[blueyed]

@lukeis
You committed a similar fix like my initial one (which you've said was not good enough) in 6fadbcc?!

I will close this PR then, and create new ones for the other changes in here.