Fix pytest backend #151
Fix pytest backend #151
Conversation
Instead of writing events for each phase (setup, call, teardown) of a test (and eventually displaying them in the datatree widget) we now accumulate results over the individual test phases and only then write the final result of the entire test upon test completion using the pytest_runtest_logfinish hook. This ensures the following: 1) Capturing of output from the teardown phase even if it passes without an error. This fixes spyder-ide#127. 2) Differentiating between the outcome of the test itself (the call phase) and the entire test as a whole. An error during teardown, for instance, used to overwrite the (potentially passed) test result of the call phase. Now we get a passed test with an error during teardown. So in the datatree widget, the test status (first column) is no longer 1:1 linked with the color of this line (see also 3)). 3) Better handling of xfailed and xpassed tests which now show the correct status `xfailed` or `xpassed` and are colored green and red respectively (instead of status `skipped` and gray color for both of them). This fixes spyder-ide#47. 4) Better messages: the first message, which is usually the most important one, will be placed in the message column of the datatree widget. Further messages will be _appended_ to the extra_text field (instead of overwriting messages from previous test phases). If the first message spans over multiple lines then only its first line will be displayed in the message column and the complete message will be repeated in the extra_text field for better readability. This improves the visual impression of the datatree widget so that each (collapsed) row is exactly one line high.
|
Below is a comparison of the output before (left side) and after (right side) this PR when running pytest on the following code (click on image to enlarge): import pytest
@pytest.fixture(scope="function",
params=['none', 'setup', 'call'])
def resource(request):
param = request.param
print('setup')
if 'setup' in param:
raise Exception('setup')
yield param
print('teardown')
if 'teardown' in param:
raise Exception('teardown')
def test_with_error_in(resource):
print('call')
assert 'call' not in resource
@pytest.mark.xfail(reason='this is expected to fail')
class TestNoErrorXfail:
def setup_method(self):
print('setup')
def teardown_method(self):
print('teardown')
def test_xfail(self):
print('call')
assert False
def test_xpass(self):
print('call')
@pytest.mark.skip(reason="we don't want to test this now")
def test_skip(self):
print('call')
The following numbers (in the picture blue in cirles) show the improvements: |
|
I forgot to mention in the commit message that I deliberately meant self.longrepr.append(
report.wasxfail if report.wasxfail else 'wasxfail') |
|
Thanks, this looks good. I am wondering whether the output may be confusing if errors occur during setup or teardown. If you run pytest from the command line, the output has lines like "error at teardown of ". It is not as clearly indicated here. The user should know that a status of What do you think about that? Perhaps out of scope for this PR (as the PR does not make it worse)?
People are lazy. The spyder repository has xfail decorators without reason. I don't know whether the string "wasxfail" is better than an empty string, but let's leave it as you wrote it. |
Yes, maybe it's indeed better to prepend some reference to the message field as to where the error occurred in this case, like so:
May only concern is that the backend usually shouldn't contain any UI strings (like What do you think about that? EDIT: (*) I just saw that |
Yes, I do not particularly like it, but I'm happy to ignore it for the moment. If you want to add the "ERROR at setup/teardown" as you suggest, please go ahead. Otherwise, I'll merge it as is. The only part of the PR that I have questions about is the part that you copied from what I wrote myself (from what I remember, one problem is that the interface of pytest is not well documented). |
I'll add it, but maybe it'll be tomorrow only. |
|
BTW, is it OK to use f-strings? CI for the plugin tests with python 3.6, 3.7 and 3.8 only, so it should be OK. Spyder itself, however, seems to support 2.7 too, as per README.md. |
|
@StefRe for the time being it is best to not use f-strings (yet). |
If an error occurs in the setup phase of a test, then the test itself (call phase) will not be executed and the status remains '---'. If an error occurs in the teardown phase after a passed test, then the status will be 'passed'. In both cases, the test category will be FAIL (red). In order to make this more easily comprehensible we prepend 'ERROR at setup: ' or 'ERROR at teardown: ' to the message in these cases.
if no reason was given in the test. Pytest gives no message whatsoever in this case (as opposed to skipped test without reason message). As we color xfailed tests green and xpassed tests red it is necessary to give some explanatory message as to why the test has the status xfailed or xpassed to avoid confusion about the color.
Codecov Report
|
|
Thanks very much! |
Refactor pytest backend
Instead of writing events for each phase (setup, call, teardown) of a
test (and eventually displaying them in the datatree widget) we now
accumulate results over the individual test phases and only then write
the final result of the entire test upon test completion using the
pytest_runtest_logfinish hook.
This ensures the following:
without an error. This fixes Teardown function's logs not captured #127 .
phase) and the entire test as a whole. An error during teardown,
for instance, used to overwrite the (potentially passed) test result
of the call phase. Now we get a passed test with an error during
teardown. So in the datatree widget, the test status (first column)
is no longer 1:1 linked with the color of this line (see also 3)).
correct status
xfailedorxpassedand are colored green andred respectively (instead of status
skippedand gray color forboth of them). This fixes pytest statuses "expected-fail" and "unexpectedly passing" not yet reflected in Category #47.
important one, will be placed in the message column of the datatree
widget. Further messages will be appended to the extra_text field
(instead of overwriting messages from previous test phases). If the
first message spans over multiple lines then only its first line
will be displayed in the message column and the complete message
will be repeated in the extra_text field for better readability.
This improves the visual impression of the datatree widget so that
each (collapsed) row is exactly one line high.