TST: extend check for leaked files #39047
Conversation
|
Hello @twoertwein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-03-31 11:44:04 UTC |
| @@ -64,6 +64,7 @@ xfail_strict = True | |||
| filterwarnings = | |||
| error:Sparse:FutureWarning | |||
| error:The SparseArray:FutureWarning | |||
| always::ResourceWarning | |||
There was a problem hiding this comment.
default::ResourceWarning might create less spam and probably catch the same mistakes. If I interpret default correctly, it will only create one warning instead of repeating it over and over (one test should fail, but not all test that use the failed function will fail). I might be wrong on that.
There was a problem hiding this comment.
python doesn't show ResourceWarnings by default (irronically -W default is not the default). Instead of having always::ResourceWarning here (or default:...), it would also be possible to have warnings.simplefilter("always", category=ResourceWarning) within the context manager or in conftest.py.
There was a problem hiding this comment.
ok, but then how do we know this is working?
There was a problem hiding this comment.
I locally tested whether it errors on open(...) without a call to close. I will look into making a test that is expected to fail to make sure that the line is not accidentally removed in the future.
| @@ -64,6 +64,7 @@ xfail_strict = True | |||
| filterwarnings = | |||
| error:Sparse:FutureWarning | |||
| error:The SparseArray:FutureWarning | |||
| always::ResourceWarning | |||
|
I added a test to make sure that The test to check this leaks a file ending with "_resoruce_test" [1]. For the test runs that fail it seems that this exact file's edit: this phenomenon might probably also explain some ssl |
|
|
||
| conns2 = proc.connections() | ||
| assert conns2 == conns, (conns2, conns) | ||
| def __init__(self, ignore_connections: bool = False): |
There was a problem hiding this comment.
why would we ignore connections?
There was a problem hiding this comment.
I disabled them when extending the test to pandas/tests/io since we probably catch some ssl connections from boto3. I will double check whether that is needed. Tests that were previously covered by check_file_leaks have their connections checked.
Do you have an idea what might be causing #39047 (comment) ?
There was a problem hiding this comment.
Do you have an idea what might be causing #39047 (comment) ?
I've never had any luck in tracking down the unclosed ssl socket warnings. the connections part of check_file_leaks was supposed to identify them, but apparently doesnt
There was a problem hiding this comment.
I have a feeling that associating ResoruceWarnings with different tests might be an issue with pytest-xdist. I found a few issues about warnings on their github but nothing that would directly explain what I see in this PR.
There was a problem hiding this comment.
could try some CI runs with xdist disabled?
There was a problem hiding this comment.
what is the bes way to disable it? I tried to add -n 0 to addopts in setup.cfg but that has no effect as pytest seems to be called with -n auto
|
@twoertwein how's this coming? |
I assume pytest-xdist might cause warnings to be associated with the other tests but I haven't yet tested disabling pytest-xdist on the CI. |
|
@twoertwein do you have a plan in mind for how to get this finished? |
It still fails when running pytest with |
Sorry to hear that, and that i dont have any helpful suggestions. If you're not planning on working on this in the near future, let's close this to clear the queue. |
Use
ResourceWarnings as indicators of leaked files. I left thepsutil-based check for leaked files.I enabled the check for leaked files for all io tests.
boto3 (#17058) is probably causing all of the unclosed ssl connections. This PR ignores ResourceWarnings that contain the string
'ssl'. The fixture applied to all io tests further ignores all connections found bypsutilsfor the same reason. Connections are checked for code that was previously covered bycheck_file_leaks.