Skip to content

TST: Partially fix io.fits test warnings#7998

Merged
saimn merged 2 commits intoastropy:masterfrom
pllim:fix-warn-io-fits
Oct 29, 2018
Merged

TST: Partially fix io.fits test warnings#7998
saimn merged 2 commits intoastropy:masterfrom
pllim:fix-warn-io-fits

Conversation

@pllim
Copy link
Member

@pllim pllim commented Oct 27, 2018

This PR gets rid of some of the warnings seen when removing addopts = -p no:warnings in setup.cfg and then running python setup.py test -P io.fits --remote-data.

There are still a plethora of ResourceWarning because I am not sure how to handle them and I also tried but ran out of energy:
zlog.txt

Maybe we can just put this in setup.cfg and be done with those pesky ResourceWarning 😱

filterwarnings =
    ignore::ResourceWarning

Also see #7928

@astropy-bot
Copy link

astropy-bot bot commented Oct 27, 2018

Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @pllim , I have a few hours to kill at the airport so it was perfect to keep me busy for some time ;).

It looks good, I just wonder if we should do this for doctest. These warnings are useful for the users, to explain what happens, so hiding them in the docs is like giving a wrong message. I don't have a better alternative for reducing the number of warnings when running tests though, so not sure which is best.

tbhdu.dump(datafile, cdfile, hfile)
tbhdu.dump(datafile, cdfile, hfile, overwrite=True)
with pytest.warns(AstropyUserWarning,
match='Overwriting existing file'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning could be removed imo, there is no reason to raise a warning if overwrite is True. (I can address this later)

@pllim
Copy link
Member Author

pllim commented Oct 27, 2018

if we should do this for doctest

I agree that we shouldn't but I am also not sure how to ask pytest to not report warnings from doctest. @drdavella , any idea? Is it even possible?

@bsipocz
Copy link
Member

bsipocz commented Oct 27, 2018

I agree that we shouldn't but I am also not sure how to ask pytest to not report warnings from doctest. @drdavella , any idea? Is it even possible?

well, but it should in general, it should just figure out which ones are expected ;)

@bsipocz bsipocz added this to the v2.0.10 milestone Oct 27, 2018
@pllim pllim changed the title TST: Fix io.fits test warnings TST: Partially fix io.fits test warnings Oct 29, 2018
@pllim
Copy link
Member Author

pllim commented Oct 29, 2018

Okay, I applied @saimn 's review (thanks, Simon!). Maybe we should merge this one for now and address the rest later, before conflicts start happening? (Assuming tests pass.)

@saimn
Copy link
Contributor

saimn commented Oct 29, 2018

Yes, good idea @pllim, and thanks for addressing this! Hopefully we can find another solution for doctests, maybe with a way to capture warnings with doctest-plus ?

@saimn saimn merged commit 064dac1 into astropy:master Oct 29, 2018
@pllim pllim deleted the fix-warn-io-fits branch October 30, 2018 02:31
bsipocz pushed a commit that referenced this pull request Oct 30, 2018
TST: Partially fix io.fits test warnings
bsipocz pushed a commit that referenced this pull request Oct 30, 2018
TST: Partially fix io.fits test warnings
bsipocz pushed a commit that referenced this pull request Nov 9, 2018
TST: Partially fix io.fits test warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants