Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: 9 failures in test_angles.py in devinfra job #16016

Closed
pllim opened this issue Feb 8, 2024 · 6 comments
Closed

TST: 9 failures in test_angles.py in devinfra job #16016

pllim opened this issue Feb 8, 2024 · 6 comments

Comments

@pllim
Copy link
Member

pllim commented Feb 8, 2024

Probably something changed again in pytest-dev . Just started seeing this today. I restarted a previously passing job on main and it fails now: https://github.com/astropy/astropy/actions/runs/7780284592/job/21375925091

All failures are similar in nature as this:

>       with pytest.warns(IllegalMinuteWarning):
astropy/coordinates/tests/test_angles.py:495: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
wrn = <warnings.WarningMessage object at 0x7fc1da718650>
    @staticmethod
    def _validate_message(wrn: Any) -> None:
        if not isinstance(msg := wrn.message.args[0], str):
>           raise TypeError(
                f"Warning message must be str, got {msg!r} (type {type(msg).__name__})"
            )
E           TypeError: Warning message must be str, got 60 (type int)
_pytest/recwarn.py:340: TypeError
FAILED coordinates/tests/test_angles.py::test_negative_sixty_hm - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_plus_sixty_hm - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_negative_fifty_nine_sixty_dms - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_plus_fifty_nine_sixty_dms - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_negative_sixty_dms - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_plus_sixty_dms - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_angle_to_is_angle - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_angle_to_quantity - TypeError: Warning message must be str, got 60 (type int)
FAILED coordinates/tests/test_angles.py::test_angle_string - TypeError: Warning message must be str, got 60 (type int)

Caused by:

Because we have:

class IllegalMinuteWarning(AstropyWarning):
"""
Raised when a minute value is 60.
Parameters
----------
minute : int, float
"""
def __init__(self, minute, alternativeactionstr=None):
self.minute = minute
self.alternativeactionstr = alternativeactionstr

Reported to:

@neutrinoceros
Copy link
Contributor

I was able to reproduce this locally, and I think there's actually a simple fix on our side:

diff --git a/astropy/coordinates/angles/formats.py b/astropy/coordinates/angles/formats.py
index d772e066b..bcece11dd 100644
--- a/astropy/coordinates/angles/formats.py
+++ b/astropy/coordinates/angles/formats.py
@@ -324,7 +324,7 @@ def _check_hour_range(hrs):
     Checks that the given value is in the range (-24, 24).
     """
     if np.any(np.abs(hrs) == 24.0):
-        warn(IllegalHourWarning(hrs, "Treating as 24 hr"))
+        warn(IllegalHourWarning(str(hrs), "Treating as 24 hr"))
     elif np.any(hrs < -24.0) or np.any(hrs > 24.0):
         raise IllegalHourError(hrs)
 
@@ -335,7 +335,7 @@ def _check_minute_range(m):
     is equal to 60, then a warning is raised.
     """
     if np.any(m == 60.0):
-        warn(IllegalMinuteWarning(m, "Treating as 0 min, +1 hr/deg"))
+        warn(IllegalMinuteWarning(str(m), "Treating as 0 min, +1 hr/deg"))
     elif np.any(m < -60.0) or np.any(m > 60.0):
         # "Error: minutes not in range [-60,60) ({0}).".format(min))
         raise IllegalMinuteError(m)
@@ -347,7 +347,7 @@ def _check_second_range(sec):
     is equal to 60, then a warning is raised.
     """
     if np.any(sec == 60.0):
-        warn(IllegalSecondWarning(sec, "Treating as 0 sec, +1 min"))
+        warn(IllegalSecondWarning(str(sec), "Treating as 0 sec, +1 min"))
     elif sec is None:
         pass
     elif np.any(sec < -60.0) or np.any(sec > 60.0):

Maybe there's something I missed that makes this patch undesirable, but just in case I'll push a PR now so it's up and ready to merge when you read this !

@dhomeier
Copy link
Contributor

dhomeier commented Feb 9, 2024

Would have seemed the obvious solution to me, just wondering if one of our warning classes might intercept numbers and convert to str instead – but it's probably not a good idea getting used to have our Warnings accept messages that pytest no longer will.

@pllim
Copy link
Member Author

pllim commented Feb 9, 2024

The weird thing is these warnings are grandchildren of Python built-in Warning and Python itself isn't complaining, but somehow pytest 8.1 is trying to enforce something that Python proper does not care about. Looks like @eerovaher is trying to patch pytest, so let's wait and see. 🤞

@dhomeier
Copy link
Contributor

dhomeier commented Feb 9, 2024

pytest has started to validate the type of the warning message in pytest-dev/pytest#11804, seems quite obvious (and legitimate) to me.

@eerovaher
Copy link
Member

The problems were indeed caused by a bug in the development version of pytest, which has now been fixed, so there's nothing for us to do here.

@pllim
Copy link
Member Author

pllim commented Feb 16, 2024

Coincidentally, weekly cron is running for a recent merge commit into v6.0.x. Let's see if devinfra will be green now. 🤞

https://github.com/astropy/astropy/actions/runs/7936108222

If I don't comment again, means everything is good. If it still fails with the same stuff, I will re-open this issue. If it fails with a different stuff, I will open a new issue. FYI and thanks!

xref

UPDATE: devinfra passed! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants