Skip to content

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

Closed
Cadair wants to merge 1 commit into
astropy:mainfrom
Cadair:retire_openfiles
Closed

Cadair wants to merge 1 commit into
astropy:mainfrom
Cadair:retire_openfiles

Conversation

@Cadair

@Cadair Cadair commented Nov 24, 2022

Copy link
Copy Markdown
Member

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

Cadair commented Nov 24, 2022

Copy link
Copy Markdown
Member Author

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

@jdavies-st

Copy link
Copy Markdown
Contributor

This looks good!

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

@Cadair

Cadair commented Nov 24, 2022

Copy link
Copy Markdown
Member Author

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

pllim commented Nov 28, 2022

Copy link
Copy Markdown
Member

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

pllim commented Nov 28, 2022

Copy link
Copy Markdown
Member

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

@Cadair

Cadair commented Nov 29, 2022

Copy link
Copy Markdown
Member Author

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