remove log_path and use service_log_path consistently instead #180

BeyondEvil · 2018-08-13T14:44:10Z

Fixes #179

BeyondEvil · 2018-08-13T16:54:55Z

testing/conftest.py

@@ -24,7 +24,7 @@ def testdir(request, httpserver_base_url):
    if 'testdir' not in item.funcargnames:
        return

-    testdir = request.getfuncargvalue('testdir')
+    testdir = request.getfixturevalue('testdir')


This was caught when running with DeprecationWarning enabled. @davehunt

Thanks @BeyondEvil. This was introduced in pytest 3.0.0, which we already depend on, so this is a very safe change.

I'm going to be a pain and ask you to split this (and a few other things) out into a separate commit or pull request. This shouldn't break anything for anyone, but if it does it makes it easier to identify if it's not combined with an unrelated change.

BeyondEvil · 2018-08-13T16:55:45Z

testing/test_crossbrowsertesting.py

-    failure('--capability', 'browser_api_name', 'FF46')
+    out = failure('--capability', 'browser_api_name', 'FF46')
+    messages = ['missing auth', 'basic auth failed']
+    assert any(message in out for message in messages)


I added this because the tests was passing for the wrong reason.

Could you move these test fixes into a separate commit or pull request?

BeyondEvil · 2018-08-13T16:56:20Z

testing/test_testingbot.py

@@ -75,4 +75,4 @@ def test_invalid_host(failure, monkeypatch, tmpdir):
    monkeypatch.setattr(os.path, 'expanduser', lambda p: str(tmpdir))
    tmpdir.join('.testingbot').write('[credentials]\nkey=foo\nsecret=bar')
    out = failure('--host', 'foo.bar.com')
-    assert "urlopen error" in out
+    assert "[Errno 8] nodename nor servname provided, or not known" in out


The error message was different now due to Urllib3.

BeyondEvil · 2018-08-13T16:57:04Z

tox.ini

@@ -9,7 +9,7 @@ envlist = py{27,36,37,py,py3}, docs, flake8
 [testenv]
 passenv = DISPLAY PYTEST_ADDOPTS
 deps = pytest-localserver
-commands = pytest -v -r a {posargs}
+commands = pytest -W error::DeprecationWarning -v -r a {posargs}


This will cause any test to Fail if they cause any DeprecationWarnings to be invoked. @davehunt

davehunt

Thanks @BeyondEvil! Just a minor nit to address before merging.

davehunt · 2018-08-15T11:22:15Z

testing/conftest.py

@@ -24,7 +24,7 @@ def testdir(request, httpserver_base_url):
    if 'testdir' not in item.funcargnames:
        return

-    testdir = request.getfuncargvalue('testdir')
+    testdir = request.getfixturevalue('testdir')


Thanks @BeyondEvil. This was introduced in pytest 3.0.0, which we already depend on, so this is a very safe change.

davehunt · 2018-08-15T11:24:24Z

pytest_selenium/pytest_selenium.py

@@ -123,7 +123,6 @@ def driver_kwargs(request, capabilities, chrome_options, driver_args,
        host=pytestconfig.getoption('host'),
        port=pytestconfig.getoption('port'),
        request=request,
-        log_path=None,


I think we should change this to service_log_path instead of removing it. Otherwise a driver_log of None would result in the default Firefox driver's log path of geckodriver.log to be used.

BeyondEvil · 2018-08-17T13:14:38Z

I kept the driver_log fixture as to not introduce a breaking change for anyone that has overloaded it or uses directly.

davehunt · 2018-08-17T13:30:34Z

pytest_selenium/drivers/edge.py



-def driver_kwargs(capabilities, driver_log, driver_path, **kwargs):
-    kwargs = {}
+def driver_kwargs(capabilities, driver_path, service_log_path, **kwargs):


I'd like to keep the keyword argument as driver_log as this is already our abstraction from log_path and service_log_path.

davehunt · 2018-08-17T13:33:23Z

pytest_selenium/pytest_selenium.py

        driver_path=driver_path,
        firefox_options=firefox_options,
        firefox_profile=firefox_profile,
        host=pytestconfig.getoption('host'),
        port=pytestconfig.getoption('port'),
+        service_log_path=driver_log,


The idea here is that driver_log gets used within our driver_kwargs methods to update service_log_path, which should default to None.

davehunt

Looks good. Would you mind splitting some things into separate commits though? Otherwise we'll have a bunch of good but unrelated changes in a single commit.

davehunt · 2018-08-17T14:03:16Z

testing/conftest.py

@@ -24,7 +24,7 @@ def testdir(request, httpserver_base_url):
    if 'testdir' not in item.funcargnames:
        return

-    testdir = request.getfuncargvalue('testdir')
+    testdir = request.getfixturevalue('testdir')


I'm going to be a pain and ask you to split this (and a few other things) out into a separate commit or pull request. This shouldn't break anything for anyone, but if it does it makes it easier to identify if it's not combined with an unrelated change.

davehunt · 2018-08-17T14:04:06Z

testing/test_crossbrowsertesting.py

-    failure('--capability', 'browser_api_name', 'FF46')
+    out = failure('--capability', 'browser_api_name', 'FF46')
+    messages = ['missing auth', 'basic auth failed']
+    assert any(message in out for message in messages)


Could you move these test fixes into a separate commit or pull request?

davehunt · 2018-08-17T14:05:17Z

testing/test_safari.py

@@ -9,7 +9,7 @@

 @pytest.mark.safari
 def test_launch(testdir, httpserver):
-    httpserver.serve_content(content='<h1>Success!</h1>')
+    httpserver.serve_content(content='<body><h1>Success!</h1></body>')


Could you make this a separate commit or pull?

davehunt · 2018-08-17T14:05:33Z

tox.ini

@@ -8,6 +8,7 @@ envlist = py{27,36,37,py,py3}, docs, flake8

 [testenv]
 passenv = DISPLAY PYTEST_ADDOPTS
+setenv = MOZ_HEADLESS=1


Could you make this a separate commit or pull?

remove log_path and use service_log_path consistently instead

0d2b527

BeyondEvil mentioned this pull request Aug 13, 2018

DeprecationWarning: use service_log_path instead of log_path #179

Closed

revert some changes and fix some deprecation warnings

5975d35

BeyondEvil commented Aug 13, 2018

View reviewed changes

remove DeprecationWarning (will create new PR) and fix failing test

d2a51a2

BeyondEvil requested a review from davehunt August 14, 2018 15:46

davehunt requested changes Aug 15, 2018

View reviewed changes

add LooseVersion for service_log_path and fix safari test

2efbff6

davehunt requested changes Aug 17, 2018

View reviewed changes

fix misunderstanding

4571be2

davehunt approved these changes Aug 17, 2018

View reviewed changes

splitting out unrelated changes to separate PR

ccbc2bb

BeyondEvil merged commit 5f1c9d3 into pytest-dev:master Aug 17, 2018

BeyondEvil deleted the remove_log_path branch August 17, 2018 21:14

remove log_path and use service_log_path consistently instead #180

remove log_path and use service_log_path consistently instead #180

Uh oh!

Conversation

BeyondEvil commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davehunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davehunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BeyondEvil Aug 13, 2018 •

edited

Loading