Strict pytest #929
Strict pytest #929
Conversation
|
ok another one that passes locally and fails in CI ! will take a look later |
Adds some trace logs
Adds some trace logs
Added trace logger in ws test that fails
| xfail_strict=True | ||
| filterwarnings= | ||
| # Turn warnings that aren't filtered into exceptions | ||
| error | ||
|
|
There was a problem hiding this comment.
it turns exceptions into errors
| @@ -123,9 +123,10 @@ def set_protocol(self, protocol): | |||
|
|
|||
There was a problem hiding this comment.
the whole diff may seem huge, but it is simple: the MockLoop takes a parameter event_loop, which is the pytest-asyncio fixture, we get nice cleanup with it.
So in order to use that, each test is decorated with @pytest.mark.asyncio
And the get_connected_protocol function is turned into a context manager so that we can also clean it on every use
|
I added some comments on the diffs to help understand it, LMK if that is enough, will explain with pleasure if something is not clear |
tests/protocols/test_http.py
Outdated
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Do we really need this marker, or can we have the mock setup the event loop by itself? Might help simplify things. I also find it surprising to use this marker on a sync test function (usually it is used to invoke async test functions).
There was a problem hiding this comment.
ok I thought that it was necessary because I'm using their (pytest-asyncio) event_loop fixture, but it is not, so I removed it.
To be complete you can use the marker on a sync test, pytest_asyncio will wrap it, but that's effectively unnecessary in our case
| @@ -151,6 +151,8 @@ def connection_lost(self, exc): | |||
| self.cycle.message_event.set() | |||
| if self.flow is not None: | |||
| self.flow.resume_writing() | |||
| if exc is None: | |||
| self.transport.close() | |||
There was a problem hiding this comment.
Could you explain these two lines?
The questions I have are:
- I assume this was prompted by warnings raising errors in certain situations. Did it happen always or only in specific conditions?
- Maybe related, why only close in case there was no exception? Does closing in case of exception cause issues?
There was a problem hiding this comment.
ok if you remove those 2 lines that close the transport inside the connection_lost method you get errors in the test_ssl
E Traceback (most recent call last):
E File "/home/lotso/.asdf/installs/python/3.9.1/lib/python3.9/asyncio/sslproto.py", line 320, in __del__
E _warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
E ResourceWarning: unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x7f3d548c8460>
../venv/lib/python3.9/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning
it's only true for httptools_impl.py you dont have the error should you remove the exact same lines in h11_impl.py or the 2 ws protocols.
reason it works without for those last 3 is simply (I think) the ssl tests defaults to the httptools implementation, so I decided to put it in 4 places but it's a bet.
the ResourceWarning error happens on every test, it's very reproducible.
the reason I opted for the exc condition is debatable, I'm not sure about it : I saw in the BaseProtocol the following docstring:
def connection_lost(self, exc):
"""Called when the connection is lost or closed.
The argument is an exception object or None (the latter
meaning a regular EOF is received or the connection was
aborted or closed).
"""
while debugging it I had a None parameter, so I decided to close the transport only in that case, but I'll be lying if I said I'm 100% sure it's the correct move
There was a problem hiding this comment.
I'd be also interested in knowing if that change (closing the transport to fix the test_ssl ressource warning) would help on the weird issue #1021
|
I think the deprecation warning that would have been caught by this PR when we unpinned |
| @@ -92,7 +94,7 @@ async def process_request(self, path, headers): | |||
| """ | |||
| path_portion, _, query_string = path.partition("?") | |||
|
|
|||
| websockets.handshake.check_request(headers) | |||
| websockets.legacy.handshake.check_request(headers) | |||
There was a problem hiding this comment.
This should fail on websockets==0.8, right? @euri10
There was a problem hiding this comment.
yes it does fail, I'm wondering if we should just pin websockets higher ?
There was a problem hiding this comment.
Either that, or doing something like:
if websockets.__version__ < "0.9":
handshake = websockets.handshake
else:
handshake = websockets.legacy.handshakeMaybe also adding a warning? Anyway, I'm fine with both.
There was a problem hiding this comment.
ok now I have given it more thoughts and considering that:
- we do a minor bump soon
- a cve has been issued on 8.* and is solved in 9.1
we should just keep this PR the way it currently is and up wesockets min reqs to 9.1
thoughts ?
There was a problem hiding this comment.
I didn't know about 2.. So yeah, let's do this: we should just keep this PR the way it currently is and up wesockets min reqs to 9.1.
* Magically turn warnings into errors ! * Close that socket * Close those loops * Is it because of lifespan was not explicitely set to off ? * Closing loop explicitely, maybe consider switching to a context manager ? * trigger GitHub actions * Close the transport when connection is lost * Close the transport when connection is lost works locally Adds some trace logs * Close the transport when connection is lost works locally Adds some trace logs * Fixed bug in message_logger.py, doesnt return Added trace logger in ws test that fails * trace log on failing wds test in ci, cant get it locally * Hard to know which test doesnt close and make the CI fail * Test order in protocols * Rewrite protocols http with context manager * Lint * rename order counts and still a bug * WOOT ? * Minimized diff, I moved tests around * Minimized diff, ws test should stay the same * Minimized diff, httptools debug stuff ermoved * Removing trace logs from diff * Reduce diff again, we take care of that bug in #967 * Removed pytest.mark.asyncio * Fix lifespan close loop post merge master * Fix websockets 9 deprecation warning
* Magically turn warnings into errors ! * Close that socket * Close those loops * Is it because of lifespan was not explicitely set to off ? * Closing loop explicitely, maybe consider switching to a context manager ? * trigger GitHub actions * Close the transport when connection is lost * Close the transport when connection is lost works locally Adds some trace logs * Close the transport when connection is lost works locally Adds some trace logs * Fixed bug in message_logger.py, doesnt return Added trace logger in ws test that fails * trace log on failing wds test in ci, cant get it locally * Hard to know which test doesnt close and make the CI fail * Test order in protocols * Rewrite protocols http with context manager * Lint * rename order counts and still a bug * WOOT ? * Minimized diff, I moved tests around * Minimized diff, ws test should stay the same * Minimized diff, httptools debug stuff ermoved * Removing trace logs from diff * Reduce diff again, we take care of that bug in encode#967 * Removed pytest.mark.asyncio * Fix lifespan close loop post merge master * Fix websockets 9 deprecation warning
The refactored test suite is able to cope with a stricter pytest (see previous draft attempt before the refactor that was way way more difficult to handle ....)
This new pytest config sets the bar high as it turns warnings into exceptions and since the pytest 6.2 release, on python versions > 3.8 you get warnings on unclosed resources (https://docs.pytest.org/en/stable/usage.html#unraisable). We open / close stuff a lot so that's possible in tests that we forget to close things, this pytest config is unforgiving in that respect !
I'm not married to that PR, it's just a proposal, stricter is not always good, in that case I think it could add value on noticing things we would skip otherwise, happy to hear your thoughts