Test output of default logging configuration #887
Conversation
|
Hmm. Looks like there's a weird interaction between the existing test cases and this new one. Only the access log line is captured when running all the tests in a row, rather than |
|
iirc I had to resort on using capsys instead of caplog when I sent the trace logging test but cant remember if I found the reason, and yes there might be an interaction from previious tests |
| assert "ASGI 'lifespan' protocol appears unsupported" in messages.pop(0) | ||
| assert "Application startup complete" in messages.pop(0) | ||
| assert "Uvicorn running on http://127.0.0.1:8000" in messages.pop(0) | ||
| assert '"GET / HTTP/1.1" 204' in messages.pop(0) |
There was a problem hiding this comment.
I'm just surprised here that it outputs 204 and not 204 No Content
There was a problem hiding this comment.
I think it's because caplog uses its own formatter, and we use AccessFormatter for access logs,
I cant find a clean way to get it through caplog without messing up with pytest
|
Even if using |
| "uvicorn": {"handlers": ["default"], "level": "INFO"}, | ||
| "uvicorn.error": {"level": "INFO"}, |
There was a problem hiding this comment.
@euri10 There were two problems here:
- The test config overrides the
rootlogger. Doesn't actually make the test fail if left as-is, but we had a PR to fix it in the default logging config: Don't override the root logger #674. - It sets
propagate: Falseonuvicorn.error, which is why when the new test runs after the others,uvicorn.errorwon't be captured. It could be captured without changing the test config — I'd have to do the same trick than foruvicorn.access— but that sounds a bit absurd.
I think given that the test trace config aims at focusing on uvicorn.asgi in particular, keeping the rest in line with LOGGING_CONFIG sounds about right.
florimondmanca
force-pushed
the
fm/test-default-logging
branch
from
e8e6905 to
9bd30c0
Compare
|
arg sorry @florimondmanca I didn't mean to push this here but to a new branch, hope this is not causing troubles, worst case we can revert my commit on this. |
|
@florimondmanca what do you think of this: https://raw.githubusercontent.com/euri10/uvicorn/test_logging/tests/middleware/test_logging.py i fu the last two commits on your PR, thought I was doing it on another branch, the above link is just the same as the present PR, just adapted for the new test suite, |
|
closed in favor of #964 |
Alternative to #885
Adds a test that verifies the expected output from our default logging configuration on a basic startup-request-shutdown sequence.