Skip to content
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.

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

Be consistent with webdriver init kwarg service_log_path #5979

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions py/selenium/webdriver/edge/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
import warnings

from selenium.webdriver.common import utils
from selenium.webdriver.remote.webdriver import WebDriver as RemoteWebDriver
Expand All @@ -25,12 +26,16 @@
class WebDriver(RemoteWebDriver):

def __init__(self, executable_path='MicrosoftWebDriver.exe',
capabilities=None, port=0, verbose=False, log_path=None):
capabilities=None, port=0, verbose=False, service_log_path=None, log_path=None):
if log_path:
warnings.warn('use service_log_path instead of log_path', DeprecationWarning)

Choose a reason for hiding this comment

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

Adding a stacklevel here and in the similar instances below, would help make the resultant warnings clearer, since they would reference the offending caller rather than the .warn() line in these files. I've filed #6274 for this (but it would be good to watch out for this when reviewing future PRs).

service_log_path = log_path

self.port = port
if self.port == 0:
self.port = utils.free_port()

self.edge_service = Service(executable_path, port=self.port, verbose=verbose, log_path=log_path)
self.edge_service = Service(executable_path, port=self.port, verbose=verbose, log_path=service_log_path)
self.edge_service.start()

if capabilities is None:
Expand Down
11 changes: 7 additions & 4 deletions py/selenium/webdriver/firefox/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ class WebDriver(RemoteWebDriver):
def __init__(self, firefox_profile=None, firefox_binary=None,
timeout=30, capabilities=None, proxy=None,
executable_path="geckodriver", options=None,
log_path="geckodriver.log", firefox_options=None,
service_args=None, desired_capabilities=None):
service_log_path="geckodriver.log", firefox_options=None,
service_args=None, desired_capabilities=None, log_path=None):
"""Starts a new local session of Firefox.

Based on the combination and specificity of the various keyword
Expand Down Expand Up @@ -100,12 +100,15 @@ def __init__(self, firefox_profile=None, firefox_binary=None,
binary to use for Firefox 47.0.1 and greater, which
defaults to picking up the binary from the system path.
:param options: Instance of ``options.Options``.
:param log_path: Where to log information from the driver.
:param service_log_path: Where to log information from the driver.
:param desired_capabilities: alias of capabilities. In future
versions of this library, this will replace 'capabilities'.
This will make the signature consistent with RemoteWebDriver.

"""
if log_path:
warnings.warn('use service_log_path instead of log_path', DeprecationWarning)
service_log_path = log_path
if firefox_options:
warnings.warn('use options instead of firefox_options', DeprecationWarning)
options = firefox_options
Expand Down Expand Up @@ -156,7 +159,7 @@ def __init__(self, firefox_profile=None, firefox_binary=None,
self.service = Service(
executable_path,
service_args=service_args,
log_path=log_path)
log_path=service_log_path)
self.service.start()

capabilities.update(options.to_capabilities())
Expand Down
17 changes: 9 additions & 8 deletions py/selenium/webdriver/ie/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@
DEFAULT_PORT = 0
DEFAULT_HOST = None
DEFAULT_LOG_LEVEL = None
DEFAULT_LOG_FILE = None
DEFAULT_SERVICE_LOG_PATH = None


class WebDriver(RemoteWebDriver):
""" Controls the IEServerDriver and allows you to drive Internet Explorer """

def __init__(self, executable_path='IEDriverServer.exe', capabilities=None,
port=DEFAULT_PORT, timeout=DEFAULT_TIMEOUT, host=DEFAULT_HOST,
log_level=DEFAULT_LOG_LEVEL, log_file=DEFAULT_LOG_FILE, options=None,
ie_options=None, desired_capabilities=None):
log_level=DEFAULT_LOG_LEVEL, service_log_path=DEFAULT_SERVICE_LOG_PATH, options=None,
ie_options=None, desired_capabilities=None, log_file=None):
"""
Creates a new instance of the chrome driver.

Expand All @@ -45,19 +45,20 @@ def __init__(self, executable_path='IEDriverServer.exe', capabilities=None,
- capabilities: capabilities Dictionary object
- port - port you would like the service to run, if left as 0, a free port will be found.
- log_level - log level you would like the service to run.
- log_file - log file you would like the service to log to.
- service_log_path - target of logging of service, may be "stdout", "stderr" or file path.
- options: IE Options instance, providing additional IE options
- desired_capabilities: alias of capabilities; this will make the signature consistent with RemoteWebDriver.
"""
if log_file:
warnings.warn('use service_log_path instead of log_file', DeprecationWarning)
service_log_path = log_file
if ie_options:
warnings.warn('use options instead of ie_options', DeprecationWarning)
options = ie_options
self.port = port
if self.port == 0:
self.port = utils.free_port()
self.host = host
self.log_level = log_level
self.log_file = log_file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason to spare log_level and log_file instance vars?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a code relevance as to why they are included as instance vars other than the fact that you can check them after setting them since they aren't available on the Service

Copy link
Member

Choose a reason for hiding this comment

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

But you're right, I don't see them in any other webdrivers.


# If both capabilities and desired capabilities are set, ignore desired capabilities.
if capabilities is None and desired_capabilities:
Expand All @@ -77,8 +78,8 @@ def __init__(self, executable_path='IEDriverServer.exe', capabilities=None,
executable_path,
port=self.port,
host=self.host,
log_level=self.log_level,
log_file=self.log_file)
log_level=log_level,
log_file=service_log_path)

self.iedriver.start()

Expand Down
8 changes: 4 additions & 4 deletions py/test/selenium/webdriver/firefox/ff_launcher_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ def test_we_can_launch_multiple_firefox_instances(capabilities):
driver3.quit()


def test_launch_firefox_with_none_log_path(capabilities):
driver = Firefox(capabilities=capabilities, log_path=None)
def test_launch_firefox_with_none_service_log_path(capabilities):
driver = Firefox(capabilities=capabilities, service_log_path=None)
driver.quit()


def test_launch_firefox_with_empty_string_log_path(capabilities):
driver = Firefox(capabilities=capabilities, log_path="")
def test_launch_firefox_with_empty_string_service_log_path(capabilities):
driver = Firefox(capabilities=capabilities, service_log_path="")
driver.quit()
8 changes: 4 additions & 4 deletions py/test/selenium/webdriver/marionette/mn_launcher_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ def test_we_can_launch_multiple_firefox_instances(capabilities):
driver3.quit()


def test_launch_firefox_with_none_log_path(capabilities):
driver = Firefox(capabilities=capabilities, log_path=None)
def test_launch_firefox_with_none_service_log_path(capabilities):
driver = Firefox(capabilities=capabilities, service_log_path=None)
driver.quit()


def test_launch_firefox_with_empty_string_log_path(capabilities):
driver = Firefox(capabilities=capabilities, log_path="")
def test_launch_firefox_with_empty_string_service_log_path(capabilities):
driver = Firefox(capabilities=capabilities, service_log_path="")
driver.quit()