improve logging test and increase coverage on server.py
#1743
Conversation
Kludex
force-pushed
the
fix/improve-cov-server
branch
from
f039148
to
d7c0fbb
Compare
Kludex
force-pushed
the
fix/improve-cov-server
branch
from
52db18f
to
1b4f770
Compare
…icorn into fix/improve-cov-server
| @@ -120,7 +120,7 @@ def _share_socket(sock: socket.SocketType) -> socket.SocketType: | |||
| self.servers.append(server) | |||
| listeners = sockets | |||
|
|
|||
| elif config.fd is not None: | |||
| elif config.fd is not None: # pragma: py-win32 | |||
There was a problem hiding this comment.
Add # pragma: py-win32 because we use socket.AF_UNIX in all instances.
There was a problem hiding this comment.
Ah, you mean in the line after... Got it. Yep!
iudeen
changed the title
server.py
…icorn into fix/improve-cov-server
tests/middleware/test_logging.py
Outdated
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| @pytest.mark.parametrize("use_colors", [True, False]) |
There was a problem hiding this comment.
The goal of improving coverage is not to blindly improve it...
It doesn't look like we have a big motivation for creating this test, based on the name it has.
Maybe we can create a test for: "test_port_zero()` or something like that...
Please create another PR for this. 👀
There was a problem hiding this comment.
There were no tests for servers that have port 0. That's the reason I had added it. I initially added it in test_main, later decided to check the logs to ensure a port is assigned.
I can move this to another PR and probably rename it.
There was a problem hiding this comment.
Done, I have removed other tests. Will create another PR for those.
|
No not at all, I'll do it tomorrow! |
Apply suggestions from code review Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
This PR helped me appreciate writing tests and also understand uvicorn even better! 😀