Skip to content

Conversation

@leplatrem
Copy link
Contributor

@leplatrem leplatrem commented Sep 26, 2024

Without this, the _now() returns a datetime without timezone, and breaks in the real world: mozilla-services/telescope#1493 and mozilla/remote-settings#667

The tests don't catch this because they mock _now() :/

@leplatrem leplatrem requested a review from a team as a code owner September 26, 2024 09:13
@leplatrem leplatrem added the bug Something isn't working label Sep 26, 2024
Copy link
Contributor

@grahamalama grahamalama left a comment

Choose a reason for hiding this comment

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

The tests don't catch this because they mock _now() :/

Not sure if this needs to be a "fast merge" (e.g. if something is broken), but I wonder if we should modify our tests or _now() in some way so that this kind of thing won't happen again. We seemed to treat a symptom and not a cause.

@leplatrem
Copy link
Contributor Author

Not sure if this needs to be a "fast merge" (e.g. if something is broken)

Only current 1.0 version is broken, no running service

_now() in some way so that this kind of thing won't happen again. We seemed to treat a symptom and not a cause.

I know exactly what you mean.
I thought of doing like this in the tests but didn't find it very elegant either:

from autograph_utils import _now

def test_now_is_timezone_aware():
    assert _now() > datetime.datetime(2024, 9, 26, tzinfo=timezone.utc)

But I could also add a test that check for expiration without using the fixed_now fixture.

@leplatrem leplatrem merged commit 9560d20 into main Sep 26, 2024
@leplatrem leplatrem deleted the remove-deprecated-utcnow branch September 26, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants