The-Compiler / pytest-xvfb

A pytest plugin to run Xvfb (or Xephyr/Xvnc) for tests.
MIT License
70 stars 11 forks source link

Xvfb warning #8

Closed mandeep closed 7 years ago

mandeep commented 8 years ago

This issue fixes #2 pertaining to an Xvfb warning. I added a pytest warning in pytest_configure, additional information in the readme, and a test that checks to make sure pytest.warns works as expected. The only thing I was't sure how to test was whether or not the warning displays in the test_xvfb_unavailable function.

The-Compiler commented 8 years ago

Sorry for the delay on this! I was busy with university stuff, and then got sick too...

I feel bad for noticing this only now, but I actually think both pytest and python warnings are unsuitable - the reason for that is that they affect the exit status (in the latter case only with the pytest-warnings plugin, but that's commonly recommended and might get part of pytest core at some point).

So I guess a simple print() would probably be the best solution, and the only one which doesn't affect the test outcome?

mandeep commented 8 years ago

No worries. :smiley:

I agree that the pytest warnings are a bit intrusive compared to the python warnings. As far as I know, print() won't show to the user unless they've used an option to display print messages. You could get around that with capsys disabled, but I'm not sure the pytest plugin allows it in the configure function.

The-Compiler commented 8 years ago

Output capturing isn't active yet during pytest_configure, otherwise the Python warnings wouldn't show up either.

FWIW for testsuites using pytest-warnings with -Wall to raise exceptions on Python warnings (like qutebrowser's), python warnings are even more intrusive :wink:

mandeep commented 8 years ago

With Python warnings I was able to see an output without having to configure pytest_configure. Were you seeing otherwise?

The-Compiler commented 8 years ago

No - what I'm saying is that if a warning worked, a print() will work too.

mandeep commented 8 years ago

I don't remember where I mentioned this, but print() does not work without capsys disabled while the Python warnings work fine without user interaction.

The-Compiler commented 8 years ago

Where are you seeing that behaviour? I just did a quick-and-dirty try with your PR:

diff --git a/pytest_xvfb.py b/pytest_xvfb.py
index 87c8a9c..037b08e 100644
--- a/pytest_xvfb.py
+++ b/pytest_xvfb.py
@@ -135,7 +135,7 @@ def pytest_configure(config):
         config.xvfb = None
         if (sys.platform.startswith('linux') and 'DISPLAY' in os.environ and
                 not config.getoption('--no-xvfb')):
-            config.warn('-XvfbNotFound',
+            print(
                         'pytest-xvfb could not find Xvfb. '
                         'You can install it to avoid windows from opening.')
     else:
diff --git a/tests/test_xvfb.py b/tests/test_xvfb.py
index d20717f..8a14538 100644
--- a/tests/test_xvfb.py
+++ b/tests/test_xvfb.py
@@ -55,7 +55,7 @@ def test_xvfb_unavailable(testdir, monkeypatch):
     """)
     assert os.environ['DISPLAY'] == ':42'
     result = testdir.runpytest()
-    result.stdout.fnmatch_lines('* 1 pytest-warnings *')
+    result.stdout.fnmatch_lines('* could not find Xvfb.*')
     assert result.ret == 0

And the test passes like I'd expect it to.

mandeep commented 7 years ago

Yep, it seems to work. When I tried it back in September I was not getting any output from print(), but I can't remember now what the issue was. I pushed a commit with the changes. If all is okay, I'll rebase so that you don't have to merge all these commits.

The-Compiler commented 7 years ago

Just a small style nitpick left, looks good otherwise! Thanks for your patience :smile:

The-Compiler commented 7 years ago

LGTM now, please rebase :smile:

The-Compiler commented 7 years ago

Thanks again for your patience and the contribution! :+1:

mandeep commented 7 years ago

Thanks for letting me contribute! :smile: