Skip to content
Closed
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
12 changes: 11 additions & 1 deletion pytest_splinter/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
from .webdriver_patches import patch_webdriver # pragma: no cover
from .splinter_patches import patch_webdriverelement # pragma: no cover

import logging
LOGGER = logging.getLogger(__name__)


NAME_RE = re.compile('[\W]')

Expand Down Expand Up @@ -308,7 +311,10 @@ def prepare_browser(parent):
prepare_browser(parent)

def make_screenshot_on_failure():
if splinter_make_screenshot_on_failure and request.node.splinter_failure:
if not splinter_make_screenshot_on_failure or not request.node.splinter_failure:
Copy link
Member

Choose a reason for hiding this comment

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

please change back, it decreases the coverage, also the readability - those multiple returns in a single function

return

try:
slaveoutput = getattr(request.config, 'slaveoutput', None)
names = junitxml.mangle_testnames(request.node.nodeid.split("::"))
classname = '.'.join(names[:-1])
Expand All @@ -321,6 +327,7 @@ def make_screenshot_on_failure():
else:
screenshot_dir = tmpdir.mkdir('screenshots').strpath
screenshot_path = os.path.join(screenshot_dir, screenshot_file_name)
LOGGER.info('Saving screenshot to {0}'.format(screenshot_path))
browser.driver.save_screenshot(screenshot_path)
with open(screenshot_path) as fd:
if slaveoutput is not None:
Expand All @@ -329,6 +336,9 @@ def make_screenshot_on_failure():
'file_name': screenshot_file_name,
'content': fd.read()
})
except Exception as e:
request.config.warn('splinter', "Could not save screenshot: {0}".format(e))
pass
Copy link
Member

Choose a reason for hiding this comment

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

pass makes no sense

request.addfinalizer(make_screenshot_on_failure)

return browser
Expand Down
16 changes: 16 additions & 0 deletions tests/test_screenshot.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Browser screenshot tests."""
import pytest

from mock import patch


def test_browser_screenshot_normal(testdir, mocked_browser):
"""Test making screenshots on test failure if the commandline option is passed.
Expand All @@ -15,6 +17,20 @@ def test_screenshot(browser):
assert testdir.tmpdir.join('test_browser_screenshot_normal', 'test_screenshot-browser.png').isfile()


@patch('_pytest.config.Config.warn')
def test_browser_screenshot_error(testdir, mocked_browser, request):
"""Test warning with error during taking screenshots on test failure."""
testdir.inline_runsource("""
def test_screenshot(browser):
# Create a file here, so makedirs in make_screenshot_on_failure will fail.
open('isafile', 'w').close()
assert False

def test_warn_called(request):
assert request.config.warn.call_count == 1
Copy link
Member

Choose a reason for hiding this comment

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

assertion should be moved out inside of the 'root' test, then it will be ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried that first, but it did not work - the warning gets done in the inner test.

I've committed it in 008728f, and added an sentence to the doc.

""", "-vl", "--splinter-session-scoped-browser=false")


@pytest.mark.skipif('not config.pluginmanager.getplugin("xdist")', reason='pytest-xdist is not installed')
def test_browser_screenshot_xdist(testdir, mocked_browser):
"""Test making screenshots on test failure if the commandline option is passed.
Expand Down