hy144328 commented on PR #39782: URL: https://github.com/apache/superset/pull/39782#issuecomment-4453706659
> This fix makes the test environment-independent, but it does so by changing the nature of the test input — the datetimes are no longer naive. The production code path being tested (dttm.timestamp() on a naive datetime) is what actually runs in the real application, because the callers throughout the Superset codebase pass naive datetimes. So the test is now no longer testing the real code path — it's testing a scenario that doesn't happen in practice (a timezone-aware input), and the underlying bug in the implementation (where a naive datetime produces a timezone-dependent epoch) remains live and unfixed. > > The best aproach would be to combine elements of both... > > 1. Fix the implementation to handle naive datetimes deterministically (using timezone.utc, not datetime.UTC), with a proper astimezone() guard for already-aware datetimes. > > 2. Keep the test inputs naive (as they reflect real usage), so the test continues to cover the actual production code path. @rusackas Thank you for the clarification. I was reluctant to change the code because, as the one who originally raised #39781 , I observed this bug when running tests locally but not in production. This said, most servers probably run on UTC so this bug will stay unnoticed for a long time. As suggested, I have reverted my own changes to the unit test, and applied @charliesheh 's ideas from #40034 . Would you please give this PR another review? Apologies for the extra loop. This would be my first contribution to this project. Happy to squash into a single commit if the history is not needed anymore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
