Fix for screenshots with function scoped browsers

[pelme]

See issue #74

---

This change is

[pelme]

I'm not sure why https://travis-ci.org/pytest-dev/pytest-splinter/jobs/165209264#L331-L346 happens, it only seems to happen when multiple tests run.

I can run test_browser_screenshot_xdist locally without failures with and without xdist installed:

‹pytest-splinter› ‹function-scoped-screenshot› ~/code/pytest-splinter → py.test -k test_browser_screenshot_xdist
=============================================================== test session starts ===============================================================
platform darwin -- Python 3.5.0, pytest-3.0.3, py-1.4.31, pluggy-0.4.0 -- /Users/andreas/.virtualenvs/pytest-splinter/bin/python
cachedir: .cache
rootdir: /Users/andreas/code/pytest-splinter, inifile: tox.ini
plugins: pylama-7.1.0, cov-2.3.1, localserver-0.3.6, xdist-1.15.0, splinter-1.7.6
collected 34 items

tests/mocked_browser/test_screenshot.py::test_browser_screenshot_xdist PASSED

=============================================================== 33 tests deselected ===============================================================
===================================================== 1 passed, 33 deselected in 2.92 seconds =====================================================

‹pytest-splinter› ‹function-scoped-screenshot› ~/code/pytest-splinter → pip uninstall pytest-xdist
Uninstalling pytest-xdist:
  ....
Proceed (y/n)? y
  Successfully uninstalled pytest-xdist

‹pytest-splinter› ‹function-scoped-screenshot› ~/code/pytest-splinter → py.test -k test_browser_screenshot_xdist
=============================================================== test session starts ===============================================================
platform darwin -- Python 3.5.0, pytest-3.0.3, py-1.4.31, pluggy-0.4.0 -- /Users/andreas/.virtualenvs/pytest-splinter/bin/python
cachedir: .cache
rootdir: /Users/andreas/code/pytest-splinter, inifile: tox.ini
plugins: pylama-7.1.0, cov-2.3.1, localserver-0.3.6, splinter-1.7.6
collected 34 items

tests/mocked_browser/test_screenshot.py::test_browser_screenshot_xdist SKIPPED

=============================================================== 33 tests deselected ===============================================================
==================================================== 1 skipped, 33 deselected in 0.20 seconds =====================================================

[bubenkoff]

tests are failing, please fix

[pelme]

Can you give the diff/failure a quick look, maybe you could figure out why? I will try to dig deeper but it was not straightforward to reproduce.

[bubenkoff]

tests/test_plugin.py:198:0: C0301 Line too long (123/120) [pylint] :)

[pelme]

@bubenkoff the line length is now fixed

I am still puzzled about the failure which only happens when I run the full test suite and not just the xdist test. I think the take_screenshot finalizer sometimes ends up as a session finalizer (which is not desirable). Do you have any ideas on where to put the screenshot-taking code?

[bubenkoff]

so for session scoped browsers, this fails due to the pytest_runtest_makereport hook doing nothing useful (as it's session's request and not node's request)

[coveralls]

Coverage increased (+0.2%) to 90.734% when pulling 21de5e4 on pelme:function-scoped-screenshot into c4cf386 on pytest-dev:master.

[pelme]

@bubenkoff please have another look. I've moved things around a bit to call the screenshot capture at the correct location for both session and function scoped browsers. I can cleanup the commit history a bit if you think this approach is good.

[bubenkoff]

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

pytest_splinter/plugin.py, line 339 at r4 (raw file):

def _take_screenshot(

document the function

---

pytest_splinter/plugin.py, line 349 at r4 (raw file):

):

    if splinter_make_screenshot_on_failure and request.node.splinter_failure:

move the condition to the fixture
this function then should be clearer what it does - makes screenshots
and the decision whether to make a screenshot or not stays outside

---

pytest_splinter/plugin.py, line 422 at r4 (raw file):

    for name, value in fixture_values.items():
        if hasattr(value, '__splinter_browser__'):

as commented, here should be the decision whether to make a screenshot

---

Comments from Reviewable

[pelme]

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

pytest_splinter/plugin.py, line 455 at r2 (raw file):

Previously, bubenkoff (Anatoly Bubenkov) wrote…

so for session scoped browsers, this fails due to the pytest_runtest_makereport hook doing nothing useful (as it's session's request and not node's request)

Done.

---

pytest_splinter/plugin.py, line 339 at r4 (raw file):

Previously, bubenkoff (Anatoly Bubenkov) wrote…

document the function

Done.

---

pytest_splinter/plugin.py, line 349 at r4 (raw file):

Previously, bubenkoff (Anatoly Bubenkov) wrote…

move the condition to the fixture
this function then should be clearer what it does - makes screenshots
and the decision whether to make a screenshot or not stays outside

Done.

---

pytest_splinter/plugin.py, line 422 at r4 (raw file):

Previously, bubenkoff (Anatoly Bubenkov) wrote…

as commented, here should be the decision whether to make a screenshot

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 90.84% when pulling f8451fe on pelme:function-scoped-screenshot into c4cf386 on pytest-dev:master.

[coveralls]

Coverage increased (+0.3%) to 90.84% when pulling 554cc28 on pelme:function-scoped-screenshot into c4cf386 on pytest-dev:master.

[bubenkoff]

Reviewed 1 of 2 files at r4, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bubenkoff]

thanks!
please release it (and add changelog entry)