Conversation
|
Does this mean we could basically stop maintaining pytest-openfiles? (after advertising on astropy-dev that it is no longer needed) |
|
I think so. |
|
I worry that removing @Cadair will removing |
|
We should test that hypothesis. We can take this PR and reintroduce the handler leak that it is supposed to catch. |
|
I would expect that to lead to a resource warning, but pushes to my test PR to check are welcome! |
|
Note the caveats on filtering ResourceWarning alone: It seems that filtering for ResourceWarning allows more latitude in constructing pytest fixtures that don't close files until teardown, but also it seems that it doesn't enforce doing so, or catch instances when they are not closed. Agree it would be nice to test famous open files cases we have in this repo and see if ResourceWarning filter would catch it. |
|
I am not that bothered about when in the pytest cycle they get raised, to me having them be thrown late makes sense because you don't know if something else (i.e. fixture teardown) is going to shut them. Having it fail a specific test is less of a concern to me as having the build fail. |
|
@WilliamJamieson and @jdavies-st , did @Cadair sufficiently address your concerns or is some follow-up required? Thanks! |
I moved/duplicated the test that astropy flagged into ASDF awhile ago. I am fine with this change. |
jdavies-st
left a comment
There was a problem hiding this comment.
This looks good to me. It seems that converting ResourceWarning to an error will catch runtime cases, and that this will give more flexibility in building pytest fixtures that provide already open files.
It does allow one to be sloppy with the fixtures and not close them, but it seems that pytest itself must do proper teardown of the fixtures before the python session is ended, or else they would show up as ResourceWarnings, but they do not.
So I'm fine with this.
section but update the contents.
df9564b to
d083b17
Compare
pllim
left a comment
There was a problem hiding this comment.
I added back a section about testing for open files but mention the new way it is done. LGTM as long as CI passes. Thanks!
|
devdeps failure is not related. Thanks, all! |
|
Now let's discuss this: |
As demonstrated in #14040 we no longer need to use
pytest-openfilesto fail the test build if files are left open. This PR purges any mention of it from basically everywhere. I split the test runner changes into a different commit because we might not want to break that if it's still in use downstream somewhere?Close #14040