Skip to content

Python: phantomjs: fix usage of WebDriverException #475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Python: phantomjs: fix usage of WebDriverException #475

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7459063
py: fix usage of WebDriverException in phantomjs driver
blueyed Apr 18, 2015
7281fea
py: phantomjs: add port information to WebDriverException
blueyed Apr 18, 2015
00b4120
py: phantomjs: service: remove unused import of signal
blueyed Apr 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions py/selenium/webdriver/phantomjs/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
# specific language governing permissions and limitations
# under the License.
import platform
import signal
import subprocess
import time
import traceback

from selenium.common.exceptions import WebDriverException
from selenium.webdriver.common import utils
Expand Down Expand Up @@ -67,21 +67,24 @@ def start(self):

:Exceptions:
- WebDriverException : Raised either when it can't start the service
or when it can't connect to the service
or when it can't connect to the service.
"""
try:
self.process = subprocess.Popen(self.service_args, stdin=subprocess.PIPE,
close_fds=platform.system() != 'Windows',
stdout=self._log, stderr=self._log)

except Exception as e:
raise WebDriverException("Unable to start phantomjs with ghostdriver.", e)
raise WebDriverException(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

msg="Unable to start phantomjs with ghostdriver: {}".format(e),
stacktrace=traceback.format_exc())
count = 0
while not utils.is_connectable(self.port):
count += 1
time.sleep(1)
if count == 30:
raise WebDriverException("Can not connect to GhostDriver")
raise WebDriverException(
"Can not connect to GhostDriver on port {}".format(self.port))

@property
def service_url(self):
Expand Down