Set timed_out attrib on reports for logging customization #90
Conversation
|
Should I 🍒-pick 3df1250, too? |
pytest_timeout.py
Outdated
| if hasattr(call.excinfo, "value"): | ||
| msg = getattr(call.excinfo.value, "msg", None) | ||
| if isinstance(msg, str) and msg.startswith("Timeout >"): | ||
| timed_out = True |
There was a problem hiding this comment.
I don't really like this method of detecting whether a timeout occurred. We have access to item here, and we have access to the same item in timeout_setup(). It seems like we could set an attribute, say item.pytest_timeout__timed_out, to keep track of this which I think would be more robust.
There was a problem hiding this comment.
Ah yes, that would be probably better! That way originated in my attempt to implement this feature outside of pytest-timeout. Now I really should use that power of internals. I'll have a look at it, thanks for the feedback!
There was a problem hiding this comment.
Done. The tests still pass, although I don't currently have a test for the customization feature. I probably should add one that checks the reliability of this new, more efficient technique using the old method that you highlighted above.
| times out. You might want to restrict this to the *call* phase because | ||
| the above code would print three symbols (lines) per test; one for each of | ||
| the three test phases *setup*, *call* and *teardown* (use ``report.when``). | ||
|
|
I'm not really sure what that looks like, could you post example output somewhere? I think it would be best to not re-use this PR for that though, up to you if you re-use your existing issue or create a new issue or create a new PR. |
In case the test timed out, set `report.timed_out = True`. If the test ran without a timeout, set `report.timed_out = False`. In any other case `report.timeout_out` should be set to `None` or be undefined. That makes it easier to customize the output in case of timeouts. This commit also adds a small section to the README including a small example.
ep12
force-pushed
the
simplify_logging_customisation
branch
from
4db168c to
266f7a2
Compare
This commit adds a simple test to verify that the `report.timed_out` attribute is set correctly.
ep12
force-pushed
the
simplify_logging_customisation
branch
from
99aeda5 to
971817e
Compare
|
I added a small test for this feature. Hopefully this isn't missing anything... |
test_pytest_timeout.py
Outdated
| timed_out = False | ||
| if hasattr(call.excinfo, "value"): | ||
| msg = getattr(call.excinfo.value, "msg", None) | ||
| if isinstance(msg, str) and msg.startswith("Timeout >"): | ||
| timed_out = True |
There was a problem hiding this comment.
What would you think of using item.name to know whether the test should have timed out or not? Or even go further and just write two tests, or rather parametrise this test.
There was a problem hiding this comment.
I am not sure if the name thing would work: test_suppresses_timeout_when_debugger_is_entered contains the word timeout but the test asserts that no timeout occurs. test_ini_method does not contain the string timeout but the test expects a timeout. There is no correlation between the names and the expected outcome. Also, using item.name might break the test when another feature pr adds another test. Seeing an unrelated test fail might become an unnecessary nightmare for the developer of the other feature. The only possible way would be to create a decorator @expect(timeout: Optional[bool]) and mark all tests. I am not sure if that is ideal. The call.excinfo.value method should be reliable and detect a timeout if it has occurred I think. Ideally, we could shift this conftest into a wrapper such that all tests are run with this test. That would probably also require a separate decorator for all existing tests though. What'd be your preference?
I'll experiment with it later this week and maybe I can come up with something more general...
There was a problem hiding this comment.
regarding names, your test only contains two test names: test_times_out and test_does_not_time_out. The rest of the tests in this file are isolated from your test.
But I think it's even better to parametrise this test and make a different conftest.py based on the parameter so it knows whether to expect a timeout or not
There was a problem hiding this comment.
Okay, I'll try to push it into as many tests as I can. However, my schedule has changed a little so I had to postpone working on this to next week...
There was a problem hiding this comment.
I managed to get a wide-ranging implementation of the test for this pr working. It is very experimental let's say if not completely opaque and ugly.
It uses a pytest hook to create a conftest.py file before every test in the pytest-timeout suite is run. The thing that annoys me most is that I am not sure if this can be easily identified as the source of tests failing. Hence I marked that last commit as temporary; either it shall be discarded or possibly changed merged into the previous one.
Here is the commit message:
TEMPORARY commit: implement test for #90
This commit implements a test for #90 (logging customisation) that is
wide-ranging as it (ab)uses all other tests in the suite to also check
that the feature implemented in the above-mentioned pull request works.
As the implications are currently not completely clear, this commit is
marked as temporary and will either be discarded or merged into a
previous commit in the pr before it is merged.
The most important open question is:
If a change breaks #90, will this be visible to the developers?
The tests in the suite run other tests using the `tempdir` facility and
this hacky implementation just adds a `conftest.py` to each of the
temporary directories. Will the message from the assert message ever
reach the output of the pytest-timeout test suite? Can that be somehow
enforced from within the child-tests or their pytest runner?
There was a problem hiding this comment.
@flub: If you find the time, I'd be glad to hear your opinion about that idea.
There was a problem hiding this comment.
hi! again, thanks for your patience.
I don't think this is the right direction though, we shouldn't be changing the entire test suite, just adding a few tests for the new functionality. I think your original test was fine as structure and overall approach, i thought the only tweak needed was to split it into two tests so you didn't have to try and detect whether the test was was going to time out or not. Since both tests are rather similar i suggested parametrisation as a shortcut, but that's not strictly required if you're not sure how to use that.
This commit implements a test for pytest-dev#90 (logging customisation) that is wide-ranging as it (ab)uses all other tests in the suite to also check that the feature implemented in the above-mentioned pull request works. As the implications are currently not completely clear, this commit is marked as temporary and will either be discarded or merged into a previous commit in the pr before it is merged. The most important open question is: If a change breaks pytest-dev#90, will this be visible to the developers? The tests in the suite run other tests using the `tempdir` facility and this hacky implementation just adds a `conftest.py` to each of the temporary directories. Will the message from the assert message ever reach the output of the pytest-timeout test suite? Can that be somehow enforced from within the child-tests or their pytest runner?
This commit implements a test for pytest-dev#90 (logging customisation) that is wide-ranging as it (ab)uses all other tests in the suite to also check that the feature implemented in the above-mentioned pull request works. As the implications are currently not completely clear, this commit is marked as temporary and will either be discarded or merged into a previous commit in the pr before it is merged. The most important open question is: If a change breaks pytest-dev#90, will this be visible to the developers? The tests in the suite run other tests using the `tempdir` facility and this hacky implementation just adds a `conftest.py` to each of the temporary directories. Will the message from the assert message ever reach the output of the pytest-timeout test suite? Can that be somehow enforced from within the child-tests or their pytest runner?
ep12
force-pushed
the
simplify_logging_customisation
branch
from
db2083f to
63aef2e
Compare
This commit implements a test for pytest-dev#90 (logging customisation) that is wide-ranging as it (ab)uses all other tests in the suite to also check that the feature implemented in the above-mentioned pull request works. As the implications are currently not completely clear, this commit is marked as temporary and will either be discarded or merged into a previous commit in the pr before it is merged. The most important open question is: If a change breaks pytest-dev#90, will this be visible to the developers? The tests in the suite run other tests using the `tempdir` facility and this hacky implementation just adds a `conftest.py` to each of the temporary directories. Will the message from the assert message ever reach the output of the pytest-timeout test suite? Can that be somehow enforced from within the child-tests or their pytest runner?
ep12
force-pushed
the
simplify_logging_customisation
branch
from
63aef2e to
e09d0f8
Compare
|
closing as no one seems to be working on this anymore. |
STATUS: draft...
In case the test timed out, set
report.timed_out = True. That makes it easier to customize the output in case of timeouts.This commit also adds a small section to the README including a small example.
Motivated by #87.