test: Fix UTC assuming test by BYK · Pull Request #3722 · getsentry/sentry-python · GitHub
Skip to content

test: Fix UTC assuming test - #3722

Merged
BYK merged 1 commit into
masterfrom
byk/test/fix-utc-issue
Oct 31, 2024
Merged

BYK merged 1 commit into
masterfrom
byk/test/fix-utc-issue

Conversation

@BYK

@BYK BYK commented Oct 31, 2024

Copy link
Copy Markdown
Member

Fixes #3720.

@codecov

codecov Bot commented Oct 31, 2024

Copy link
Copy Markdown

@BYK
BYK enabled auto-merge (squash) October 31, 2024 15:19
@BYK
BYK requested a review from asottile-sentry October 31, 2024 15:19
Comment thread tests/test_utils.py
Comment on lines 70 to +73

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

given the comment I actually think the implementation is incorrect and should be something like:

try:
   dt = datetime.fromisoformat(s)
except ...:
    ...
else:
   if dt.tzinfo is None:
       return dt.replace(tzinfo=UTC)
   else:
       return dt.astimezone(UTC)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wrote that comment 😅 And after reading the ISO 8601 spec, I think I was mistaken:

https://en.wikipedia.org/wiki/ISO_8601#Local_time_(unqualified)

If no UTC relation information is given with a time representation, the time is assumed to be in local time.

Now there's the obvious warning this being unsafe in the next sentence but I'm split between being spec-compliant or "doing the right thing" which may confuse people if they rely on the spec.

What's your take @asottile-sentry?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suspect it won't matter as most should run their servers in UTC anyway -- so I guess we can go with the current implementation

it seems to only be used to sort breadcrumbs in the event payload (is that even necessary? surely relay or the frontend should sort those right? do we even need this function and the caller?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

surely relay or the frontend should sort those right?

Agree with this but maybe we're trying to save resources? It also seems like the side effect of converting a string to a python date might be something we are after here. That said that python date will then be converted back into an ISO 8601 timestamp so 🤷🏻

Will ask ingest folks.

@asottile-sentry asottile-sentry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we can just delete this code?

@BYK
BYK merged commit 5c5d98a into master Oct 31, 2024
@BYK
BYK deleted the byk/test/fix-utc-issue branch October 31, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test assumes system timezone is UTC

2 participants