MNT: Stop using our own catch_warnings and raises by pllim · Pull Request #10504 · astropy/astropy

pllim · 2020-06-17T21:34:52Z

Description

This pull request is to replace our own catch_warnings with standard pytest stuff. This addresses most of #6761 but not going to deprecate anything.

Also see https://docs.pytest.org/en/stable/warnings.html . pytest was at 5.4.1 when this PR was opened.

As for deprecation warning, local testing indicates that it will bubble through and be caught by filterwarnings = error in setup.cfg even without our own catch_warnings.

Similarly, our own raises can be replaced with pytest.raises.

TODO

p.s. Oh dear, I didn't realize I was going to touch so many lines. Initial grep-fu failed me and I underestimated the impact. If this sits too long and conflicts start to creep in, this PR might need to be broken down into smaller pieces that can be merged faster. Or someone can come do a mega-review quick and get this in sooner than later. 😬

mhvk

Yes, nice! Nitpicks only.

pllim · 2020-06-19T02:58:31Z

-                if message_to_find in str(w.message):
-                    messages_found[i] = True
-    assert all(messages_found)
+        messages_found = ["(some) times are outside of range covered by IERS "


@mhvk , I tried to simplify the logic here but you might want to double check.

pllim · 2020-06-19T02:59:32Z

-        altaz_auto = sc.transform_to(altaz)
-
-    assert len(w) == 0
+    altaz_auto = sc.transform_to(altaz)


Note: Now that we turn all warnings into errors, except for the ones we explicitly ignore, check for zero warning is no longer necessary.

pllim · 2020-06-19T03:02:14Z

    values = np.array([1.01e200, np.inf, np.inf, 0.0, -np.inf])
-    with catch_warnings(AstropyWarning) as w:
+    # NOTE: Warning behavior varies for the parameters being passed in.
+    with pytest.warns(None) as w:


This test file is a tricky one. nullcontext doesn't work and too strict a warns catching doesn't work either; must be due to weird interactions between pyx and pure Python. 🤷

At least this way, I think I didn't change the original intent of the tests.

pllim · 2020-06-19T03:05:29Z

    assert isinstance(t2['l'].unit, u.UnrecognizedUnit)
    assert str(t2['l'].unit) == 'bandpass_sol_lum'
    if table_cls is QTable:
-        assert len(w) == 1


For whatever reason, catch_warnings did not see the ResourceWarning but pytest.warns did. 🤷

pllim · 2020-06-30T21:41:41Z

I was wondering mostly as an easy way for us to know that we are regressing

Yes, it is a risk we have to take until we're ready to issue that deprecation warning. I removed mention of it from dev docs, so hopefully that would reduce the risk but it is not foolproof. :(

pllim · 2020-06-30T21:49:21Z

not quite clear what the status is

Update: I undid my damage to wcs tests. Should be good for review now.

pllim · 2020-07-01T00:15:27Z

    @pytest.mark.skipif(not HAS_BZ2, reason='Python built without bz2 module')
    def test_detect_bzipped(self):
        """Test detection of a bzip2 file when the extension is not .bz2."""
-        with ignore_warnings():


Note: I don't know why warnings were ignored blindly and turns out they are not necessary anyway.

pllim · 2020-07-01T00:19:12Z

-            with pytest.raises(OSError):
-                fits.open(filename, ignore_missing_end=True)
+        with pytest.raises(OSError):
+            fits.open(filename, ignore_missing_end=False)


Note: The diff is a bit misleading. All I did was un-indent after removing with catch_warnings(ResourceWarning) as ws because that catch is now unnecessary.

mhvk

Looks all good, especially the cases where you found ignoring warnings was not even necessary! I also think the pytest.warns(None) as w trick is actually quite nice, maybe even better than the null context (obviously, no need to adjust!), and nice to have it mentioned in the test guide. Thanks so much for doing this huge cleanup!!

Only item left would seem to get an answer to https://github.com/astropy/astropy/pull/10504/files#r446478186, plus a few really small items if you have the energy. None feel necessary, so I am approving - would definitely be good to merge sooner rather than later!

mhvk · 2020-07-01T15:26:24Z

        diffobj = HeaderDiff(ha, hb)
        diffobj.report(fileobj=outpath)
-        with catch_warnings(AstropyDeprecationWarning) as warning_lines:
+        with pytest.warns(AstropyDeprecationWarning, match=r'"clobber" was '


Probably OK to remove now, but a separate issue!

If you mean removing clobber from API, I cannot find an issue for it though it was a bit controversial when the deprecation happen. It was truly deprecated in #6203 (v2.0). @saimn , do you think it is safe to finally remove now? Or is this one of those we just keep around deprecated forever. This is FITS, after all.

I would vote to keep it forever ;)

mhvk · 2020-07-01T15:35:14Z

    t1 = Table()
    t1.add_column(Column(name='a', data=[1, 2, 3]))
-    with pytest.raises(ValueError) as exc:
+    with pytest.warns(UserWarning, match="table path was not set via the path= argument"):


Yes, surely this was just a copy & paste paste error

mhvk · 2020-07-01T15:39:48Z

 def test_oversize_unicode():
    config = {'verify': 'exception'}
-    with catch_warnings(exceptions.W46) as w:
+    with pytest.raises(exceptions.W47):


Weird, how did catch_warnings manage to pass?

mhvk · 2020-07-01T15:42:24Z


        assert_quantity_allclose(b_fit.temperature, 2840.7438339457754 * u.K)
-        assert_quantity_allclose(b_fit.bolometric_flux, 6.821837075583734e-08 * u.erg / u.cm**2 / u.s)
+        assert_quantity_allclose(b_fit.bolometric_flux, 6.821837075583734e-08 * u.erg / u.cm**2 / u.s)  # noqa


Another weird noqa one...

Line too long, but maybe I should just fix it...

mhvk · 2020-07-01T15:51:20Z

-            assert w[0].category is table.TableReplaceWarning
-            assert 'test_table' in w[0].filename
+        # Make sure the warning points back to the user code line
+        assert w[0].lineno == frameinfo.lineno + 2


I think it is simply because you now inserted the with statement in between. This perhaps is one case where it makes sense to get the frame inside it, but no need unless you are editing anyway.

mhvk · 2020-07-01T15:59:53Z

-                with pytest.raises(ValueError) as err:
-                    dat.ut1_utc(Time(60000, format='mjd').jd)
-            assert 'interpolating from IERS_Auto using predictive values' in str(err.value)
+            with pytest.warns(iers.IERSStaleWarning, match='IERS_Auto predictive values are older') as warns, pytest.raises(ValueError, match='interpolating from IERS_Auto using predictive values'):  # noqa


Would be nice to break the line (if you are doing another round...)

pllim · 2020-07-01T21:04:11Z

@mhvk , I think I applied all the code changes you asked for and hopefully answer all your questions/concerns. Please let me know if I missed anything. Thank you so much for doing the mega-review!

pllim · 2020-07-01T21:27:55Z

As for pytest.warns(None) vs nullcontext, these two example below should be equivalent:

with pytest.warns(None) as w:
    some_code()
assert len(w) == 0

with nullcontext():
    some_code()

Therefore, I think pytest.warns(None) should be used when you want to blindly catch any warnings and nullcontext is for when you don't want any warnings at all.

mhvk · 2020-07-01T21:29:18Z

Yes, I think this can (and should!) go in, once tests have passed.

And the nullcontext vs None makes sense.

pllim · 2020-07-02T02:39:07Z

Given the approval, I'll go ahead and merge then before I need to rebase (again). Codecov failure can be ignored. Thank you so much for the review!

…ng in WCSAxes tests to not be sensitive to unrelated warnings

pllim added testing Refactoring no-changelog-entry-needed labels Jun 17, 2020

pllim added this to the v4.2 milestone Jun 17, 2020

astropy-bot Bot added config coordinates labels Jun 17, 2020

pllim added the erfa label Jun 17, 2020

pllim mentioned this pull request Jun 17, 2020

Deprecate catch_warnings and raises in favor of pytest machinery? #6761

Closed

mhvk reviewed Jun 17, 2020

View reviewed changes

pllim force-pushed the no-catch-warnings branch from ee84d0e to c693cf1 Compare June 19, 2020 02:53

pllim added Docs io.ascii labels Jun 19, 2020

pllim commented Jun 19, 2020

View reviewed changes

Comment thread astropy/coordinates/tests/test_iau_fullstack.py

pllim commented Jun 19, 2020

View reviewed changes

Comment thread astropy/io/ascii/tests/test_c_reader.py

pllim commented Jun 19, 2020

View reviewed changes

Comment thread astropy/io/ascii/tests/test_c_reader.py

pllim commented Jun 19, 2020

View reviewed changes

Comment thread docs/development/testguide.rst Outdated

pllim added units utils utils.iers visualization.wcsaxes wcs.wcsapi wcs labels Jun 19, 2020

This comment has been minimized.

Sign in to view

pllim force-pushed the no-catch-warnings branch 2 times, most recently from 9ab3457 to d62b324 Compare June 25, 2020 21:35

pllim changed the title ~~MNT: Stop using our own catch_warnings~~ MNT: Stop using our own catch_warnings and raises Jun 25, 2020

pllim added 3 commits June 30, 2020 12:42

MNT: Remove outdated imports

ad61c7d

DOC: Also add a note about raises

fa2dd7b

Fix typo and CI error

6b11398

MNT: Remove catch_warning and raises from wcs tests

f467c4a

pllim force-pushed the no-catch-warnings branch from 23cc99a to f467c4a Compare June 30, 2020 21:47

pllim added the Ready-for-final-review label Jun 30, 2020

pllim commented Jul 1, 2020

View reviewed changes

Comment thread setup.cfg

mhvk approved these changes Jul 1, 2020

View reviewed changes

pllim added 3 commits July 1, 2020 16:32

Fix test_oversize_unicode in io.votable

8301175

Delete duplicate test_write_nopath in test_hdf5

0c348c6

Apply @mhvk comments for modeling, table, utils tests

73b94a4

pllim commented Jul 1, 2020

View reviewed changes

Comment thread astropy/io/votable/tests/converter_test.py

mhvk added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Jul 1, 2020

pllim merged commit 1f75aa1 into astropy:master Jul 2, 2020

pllim deleted the no-catch-warnings branch July 2, 2020 02:39

This was referenced Jul 3, 2020

Missing dependencies for astropy docs build astropy/sphinx-astropy#21

Open

fitscheck should not be using astropy.tests.helper.catch_warnings #10528

Closed

Lock-free cache implementation #10437

Merged

astrofrog added a commit to astrofrog/astropy that referenced this pull request Aug 3, 2020

Partial backport of changes in astropy#10504 to update warning handli…

c8cf2b1

…ng in WCSAxes tests to not be sensitive to unrelated warnings

astrofrog mentioned this pull request Aug 3, 2020

Fix mpldev build in v4.1.x branch #10621

Merged

dhomeier mentioned this pull request Nov 13, 2020

TST: Handle PY_SSIZE_T_CLEAN warnings #11019

Merged

pllim mentioned this pull request Apr 29, 2021

TST: Use pytest.raises instead of astropy.tests.helper.raises #11670

Merged

Conversation

pllim commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

pllim commented Jun 30, 2020

Uh oh!

pllim commented Jun 30, 2020

Uh oh!

pllim Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pllim commented Jul 1, 2020

Uh oh!

pllim commented Jul 1, 2020

Uh oh!

mhvk commented Jul 1, 2020

Uh oh!

pllim commented Jul 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pllim commented Jun 17, 2020 •

edited

Loading

pllim Jul 1, 2020 •

edited

Loading