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

Enable/add some datetime tests unrelated to Y2K38 #24442

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lanurmi
Copy link
Contributor

@lanurmi lanurmi commented Mar 28, 2024

TestTimeTicks() was actually run for very few values; enabled some more of those in the array.

In TestTimeFormat() I added some new dates, and found out that any date within a period of DST will fail the test by 1 hour, but only between the years [2031, 2037], and only on MSW and Linux, but not macOS. The timezone of the running environment, and the TZ environment variable, also affect this.

For example (on Linux):
./test DateTimeTestCase -> fail
TZ=CET ./test DateTimeTestCase -> fail
TZ=CEST ./test DateTimeTestCase -> no error

As I don't yet know what the reason is, or why 2030/2031 are different, this PR serves both as a PR and an issue with a minimal reproducible example of a problem.

TestTimeTicks() checks were skipped for testDates with gmticks == -1, i.e.
nearly all of them. (What is the point of the test if it is mostly skipped?)
Dates are, however, well below Y2K38.
@lanurmi
Copy link
Contributor Author

lanurmi commented Mar 28, 2024

And the tests did not even fail here with CI, although they do fail for me locally. 🤷‍♂️
(Windows 10, Fedora Rawhide; system timezone EET)

@vadz
Copy link
Contributor

vadz commented Apr 7, 2024

Sorry, I'm not sure if this one is supposed to be merged or if it's a draft/WIP?

@lanurmi
Copy link
Contributor Author

lanurmi commented Apr 7, 2024

The tests that were added are simple and completely valid, and tests passed in CI, so from that perspective it should be merged. But as mentioned, tests with the new date in 2031 fail for me locally (and I don't know why), meaning they could fail for others, too.

Then again, if there is an issue, it is there whether or not this is merged, but without merging the issue is untested, undocumented, and hidden.

@vadz
Copy link
Contributor

vadz commented Apr 8, 2024

FWIW I can confirm that this fails for me too locally, so I'm not going to merge it yet, as I want the tests to pass when I run them.

Might be related to #15370.

@vadz vadz added the work needed Too useful to close, but can't be applied in current state label Apr 8, 2024
@lanurmi
Copy link
Contributor Author

lanurmi commented Apr 8, 2024

Makes sense. 5ad2ca1 alone should be safe to cherry-pick, though.

@vadz
Copy link
Contributor

vadz commented Apr 8, 2024

I had a brief look at this and the problem is that the date gets formatted as 1931-07-29 15:28:00 (note the century) and MakeFromTimezone() doesn't apply DST to it.

IOW I think things would work if we used a format with 4 digits year instead of %x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work needed Too useful to close, but can't be applied in current state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants