TST: Get rid of time test warnings#8754
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8754 +/- ##
==========================================
- Coverage 86.97% 86.91% -0.07%
==========================================
Files 399 399
Lines 59384 59384
Branches 1100 1100
==========================================
- Hits 51652 51612 -40
- Misses 7091 7131 +40
Partials 641 641
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
mhvk
left a comment
There was a problem hiding this comment.
Nice work, though I think we can reduce the impact, both by changing time scale to TAI where needed (see inline), and more specifically by ensuring that the IERS download doesn't happen unless it is really needed (for the tests that do not explicitly require it, at least, we should turn it off!). I think we may need @taldcroft for the latter...
| t.write(fn) | ||
| t2 = Table.read(fn, astropy_native=True) | ||
| with pytest.warns(VerifyWarning): | ||
| t2 = Table.read(fn, astropy_native=True) |
There was a problem hiding this comment.
While having the warning is a bug, I think we should still catch it for now until the bug is fixed. So that when it is fixed, this will cause the test to fail, and then we can remove the pytest.warns line?
| t.write(fn) | ||
| t2 = Table.read(fn, astropy_native=True) | ||
| with pytest.warns(VerifyWarning): | ||
| t2 = Table.read(fn, astropy_native=True) |
pllim
left a comment
There was a problem hiding this comment.
So, you would see that in some modules, I set the auto_download at function level, some at class level, and some at module level. My reasoning is that I should impact the tests as little as possible, hence the slightly different treatments.
| Time.FORMATS.update(func.FORMATS_ORIG) | ||
|
|
||
|
|
||
| class TestBasic(): |
There was a problem hiding this comment.
I think this is leftover from incomplete PY2 removal? Sorry, I can't help myself and had to clean it up. 😂
| t.write(fn) | ||
| t2 = Table.read(fn, astropy_native=True) | ||
|
|
||
| # Warning is a bug; https://github.com/astropy/astropy/issues/8773 |
There was a problem hiding this comment.
Added a comment here about #8773, if that helps.
astropy/time/tests/test_ut1.py
Outdated
|
|
||
|
|
||
| class TestTimeUT1(): | ||
| @pytest.mark.remote_data |
There was a problem hiding this comment.
Here, I did the opposite but marking the whole test class as remote_data. This is becase its test_utc_to_utc1 was already marked as remote_data, so I am guessing the other two not being marked as remote_data was an oversight? Or is there a reason why the other two methods in this test class need local IERS table, but not the first one?
There was a problem hiding this comment.
Here, all the tests should work with the built-in table, so I'd go for your do-not-download setting.
There was a problem hiding this comment.
Actually, the test that is already marked remote_data fails when using local file because extrapolation is not allowed. I'll think of something...
| '2008-12-31T17:27:17.193' '2010-01-01T00:00:00.000']> | ||
|
|
||
| >>> t.sidereal_time('apparent', 'greenwich') # doctest: +FLOAT_CMP | ||
| >>> t.sidereal_time('apparent', 'greenwich') # doctest: +FLOAT_CMP +REMOTE_DATA |
There was a problem hiding this comment.
I thought about setting iers.conf.auto_download = False in the doc here too, but decided against it. I think it is cleaner this way unless you strongly oppose this approach.
There was a problem hiding this comment.
Agreed; docs should be kept clean.
astropy/time/tests/test_ut1.py
Outdated
|
|
||
|
|
||
| class TestTimeUT1(): | ||
| @pytest.mark.remote_data |
There was a problem hiding this comment.
Here, all the tests should work with the built-in table, so I'd go for your do-not-download setting.
| '2008-12-31T17:27:17.193' '2010-01-01T00:00:00.000']> | ||
|
|
||
| >>> t.sidereal_time('apparent', 'greenwich') # doctest: +FLOAT_CMP | ||
| >>> t.sidereal_time('apparent', 'greenwich') # doctest: +FLOAT_CMP +REMOTE_DATA |
There was a problem hiding this comment.
Agreed; docs should be kept clean.
|
@mhvk, I think I have addressed all your comments. @taldcroft , are you okay with catching that bug warning for now? |
|
@pllim - could you rebase to make sure it's still all green? Let's get this in to have fewer warnings all around. |
|
@bsipocz , it is rebased. |
|
OK. I suggest to merge when all CI is happy. Then #8784 can remove the catching of the warning. |
bsipocz
left a comment
There was a problem hiding this comment.
Approving for now, there is a clear way for the only one remaining issue of how to be resolved.
|
@pllim - running this locally (with the changes to setup.cfg), I still see 5 failures. Can you comment on them? |
|
@bsipocz , those look so familiar! Where have I seen them before! Can you please post your pytest header? |
|
Sure: |
|
What are your |
|
For the "file format not recognized" one, there is an issue at scientific-python/pytest-doctestplus#59 . For the |
|
asdf 2.3.3 |
|
@bsipocz , you might need to upgrade to Overall, all the warnings you reported can be explained and fixing them is out of scope for this PR. |
tried to, it has no effect, still 5 failures.
that is perfectly reasonable. This PR clears up a significant amount of stuff, and we still have a few to go until we can turn off ignoring the warnings. |
As part of work for #7928 . The warnings were captured by removing the line in
setup.cfgto suppress pytest warnings and then add this in the same section:And then run tests with
python setup.py -P time. Most of the changes are to handleErfaWarningand tests accessing IERS tables (which require download) but not marked asremote_data.