Skip to content

test: Fix UTC assuming test #3722

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 31, 2024
Merged

test: Fix UTC assuming test #3722

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ def _normalize_distribution_name(name):
), # UTC time
(
"2021-01-01T00:00:00.000000",
datetime(2021, 1, 1, tzinfo=timezone.utc),
), # No TZ -- assume UTC
datetime(2021, 1, 1).astimezone(timezone.utc),
), # No TZ -- assume local but convert to UTC
Comment on lines 70 to +73
Copy link
Contributor

@asottile-sentry asottile-sentry Oct 31, 2024

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
Member Author

@BYK BYK Oct 31, 2024

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
Contributor

@asottile-sentry asottile-sentry Oct 31, 2024

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
Member Author

@BYK BYK Oct 31, 2024

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.

(
"2021-01-01T00:00:00Z",
datetime(2021, 1, 1, tzinfo=timezone.utc),
Expand Down
Loading