DM-12061: Eliminate test warnings in test_methods.py #279
Conversation
There was a problem hiding this comment.
This looks fine given that we want to minimize code changes. I'm okay with python2 silently skipping. Have you tested that this test works fine from the command line when deprecation warnings aren't enabled? I worry it needs a warnings.simplefilter('always') line somewhere.
|
Good question. The unittest documentation says of |
| @@ -39,7 +39,7 @@ | |||
|
|
|||
| from builtins import object | |||
| import numpy as np | |||
| import pyfits | |||
| import astropy.io | |||
There was a problem hiding this comment.
Many people do this as import astropy.io.fits as pyfits to minimize code impact but that is probably more confusing in the long term since it leaves the pyfits string in the code.
There was a problem hiding this comment.
Agreed. I considered from astropy.io import fits but felt it was too ambiguous, and in at least one case fits was already used as a variable.
There was a problem hiding this comment.
Looking at other AFW code this should be plain import astropy.io.fits (we don't want to import all the other astropy IO plugins).
Use `unittest.assertWarns` if available to check that deprecated asserts raise `DeprecationWarning`. If the version of Python is too old for unittest to have `assertWarngs` then use a null context manager instead (and pytest will issue warnings).
r-owen
force-pushed
the
tickets/DM-12061
branch
from
be706a7
to
2a096ba
Compare
|
In case my comment got lost when I attached it to a line; I think we need |
|
Sorry, I missed that. I'll fix in on DM-10765 |
|
I also found and fixed another instance in test_wcsFitsTable.py on that same ticket. |
Use
unittest.assertWarnsif available to check that deprecatedasserts raise
DeprecationWarning. If the version of Pythonis too old for unittest to have
assertWarngsthen use anull context manager instead (and pytest will issue warnings).