Skip to content

Demonstrate that we don't need pytest-openfiles any more#14040

Closed
Cadair wants to merge 1 commit intoastropy:mainfrom
Cadair:retire_openfiles
Closed

Demonstrate that we don't need pytest-openfiles any more#14040
Cadair wants to merge 1 commit intoastropy:mainfrom
Cadair:retire_openfiles

Conversation

@Cadair
Copy link
Member

@Cadair Cadair commented Nov 24, 2022

I ran into a pytest-openfiles bug astropy/pytest-openfiles#32 and it seems like we should just stop using it because pytest throws a ResourceWarning now which our default warnings filter converts into an exception.

This PR adds some tests which leave open files hanging (and some which don't), my hypothesis is that all the CI jobs will fail and not just the one which uses pytest-openfiles, if this is the case we can remove openfiles because it is redundant.

Secondary hypothesis: More tests will fail on the pytest-openfiles build because of astropy/pytest-openfiles#32 that will fail on the other builds, but these fails are actually incorrect and shouldn't fail because the files are closed in the fixture teardown which happens after pytest-openfiles checks to see if the files are closed.

@Cadair Cadair added the testing label Nov 24, 2022
@Cadair Cadair changed the title Demonstrate we don't need pytest-openfiles any more Demonstrate that we don't need pytest-openfiles any more Nov 24, 2022
@astropy astropy deleted a comment from github-actions bot Nov 24, 2022
@Cadair
Copy link
Member Author

Cadair commented Nov 24, 2022

Well that proved both hypotheses. https://github.com/astropy/astropy/actions/runs/3540889860/jobs/5944492676#step:9:1750

@jdavies-st
Copy link
Contributor

This looks good!

I noticed that pytest-openfiles catches test_unclosed_fixture, whereas the defaults warnings filter does not. Any idea why?

@Cadair
Copy link
Member Author

Cadair commented Nov 24, 2022

Well pytest-openfiles would catch the fixture if it were closed or unclosed, so it's not a surprise it gets it, what is a surprise is that it doesn't raise a resource warning.

@pllim
Copy link
Member

pllim commented Nov 28, 2022

But we are also ignoring these. Does this mean we cannot ignore these no more?

astropy/setup.cfg

Lines 135 to 136 in b2b32a4

ignore:unclosed <socket:ResourceWarning
ignore:unclosed <ssl.SSLSocket:ResourceWarning

@pllim
Copy link
Member

pllim commented Nov 28, 2022

I would also be interested to see if #14041 would fail on these cases. Is there already such a PR somewhere?

@Cadair
Copy link
Member Author

Cadair commented Nov 29, 2022

Those ignores aren't excluding open file resource warnings, just ones for SSL and socket?

@pllim pllim closed this in #14041 Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants