Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter pytest #910

Closed
wants to merge 18 commits into from
Closed

Stricter pytest #910

wants to merge 18 commits into from

Conversation

euri10
Copy link
Member

@euri10 euri10 commented Dec 23, 2020

It seems to me we could benefit from a stricter pytest, especially the pytest.PytestUnhandledThreadExceptionWarning since we use threads a lot.

Moreover, test exceptions raised in a separate thread dont seem to be handled well with pytest, we have an example of this in the ws test suite, see #760 for instance.
This is also one of the reason our test suite behave strangely as sometimes the whole suite passes fine but seperate tests triggered on their own can fail.

With this pytest configuration we can see 2 issues, fixed after:

  1. the test_config left an unclosed socket when the bind_socket() method was called
  2. and the whole ws test suite failed with those stricter testing, as the context manager to run the ws server was not closed

this is the error we would get with the stricter config, without the 2 fixes.

>               warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
E               pytest.PytestUnraisableExceptionWarning: Exception ignored in: <socket.socket fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0>
E               
E               Traceback (most recent call last):
E                 File "/home/lotso/PycharmProjects/uvicorn/venv/lib/python3.8/site-packages/_pytest/python.py", line 183, in pytest_pyfunc_call
E                   result = testfunction(**testargs)
E               ResourceWarning: unclosed <socket.socket fd=17, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 8000)>

../venv/lib/python3.8/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning

@euri10 euri10 marked this pull request as ready for review December 23, 2020 10:37
@euri10
Copy link
Member Author

euri10 commented Dec 23, 2020

interestingly the ci fails on windows....on other tests !
have no idea as to why.... will investigate
if you get any idea I'm all ears, dont have a win machine at hand now

@euri10 euri10 marked this pull request as draft December 24, 2020 13:11
@euri10
Copy link
Member Author

euri10 commented Dec 24, 2020

will continue on this when I'll have a windows machine, those errors are hard to debug because it's failing on weird places and to be honest the pytest messages are not helping

@euri10
Copy link
Member Author

euri10 commented Dec 28, 2020

haha closing this, the test suite changed and the new approach with context manager leads to way less issues it seems, will resurrect when/if #918 lands

@euri10 euri10 closed this Dec 28, 2020
@euri10 euri10 deleted the stricter_pytest branch December 28, 2020 19:09
@euri10 euri10 mentioned this pull request Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant