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

Fix tests with Europe/Amsterdam pre-1970 time on tzdata version 2022b. #939

Merged
merged 1 commit into from Aug 24, 2022

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Aug 24, 2022

The Time Zone Database (tzdata) changed the pre-1970 timestamps in some zones
including Europe/Amsterdam on tzdata version 2022b or later.
See eggert/tz@35fa37f.

The tzdata RPM package maintainer on Fedora project suggested changing the Ruby
test, because the change is intentional.
See https://bugzilla.redhat.com/show_bug.cgi?id=2118259#c1.

We use post-1970 time test data to simplify the test.


This PR fixes #936. I tested the command in the 2 cases below.

tzdata: version 2022b, Ruby: the latest master branch

<mock-chroot> sh-5.1$ cat /etc/fedora-release 
Fedora release 38 (Rawhide)

<mock-chroot> sh-5.1$ rpm -q tzdata
tzdata-2022b-1.fc38.noarch

<mock-chroot> sh-5.1$ dest/bin/ruby -v
ruby 3.2.0dev (2022-08-23T09:01:35Z master 46c3a93982) [x86_64-linux]

<mock-chroot> sh-5.1$ dest/bin/ruby -e "p Time.send(:local, 1970, 5, 16).to_a"
[0, 0, 0, 16, 5, 1970, 6, 136, false, "CET"]

tzdata: version 2022a, Ruby: 3.1.1

$ cat /etc/fedora-release 
Fedora release 36 (Thirty Six)

$ rpm -q tzdata
tzdata-2022a-2.fc36.noarch

$ ruby -v
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]

$ ruby -e "p Time.send(:local, 1970, 5, 16).to_a"
[0, 0, 0, 16, 5, 1970, 6, 136, false, "CET"]

Note for the return value of the Time.send(:local, 1970, 5, 16).to_a.
https://ruby-doc.org/core-3.1.2/Time.html#method-i-to_a

The Time Zone Database (tzdata) changed the pre-1970 timestamps in some zones
including Europe/Amsterdam on tzdata version 2022b or later.
See <eggert/tz@35fa37f>.

The tzdata RPM package maintainer on Fedora project suggested changing the Ruby
test, because the change is intentional.
See <https://bugzilla.redhat.com/show_bug.cgi?id=2118259#c1>.

We use post-1970 time test data to simplify the test.
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you!

@eregon eregon merged commit daf11e3 into ruby:master Aug 24, 2022
@junaruga junaruga deleted the wip/tzdata-pre-1970 branch August 24, 2022 12:19
@junaruga
Copy link
Member Author

Thanks for your checking. Please sync this commit to the ruby/ruby too.

jbronn added a commit to radiant-maxar/geoint-deps that referenced this pull request Sep 9, 2022
jbronn added a commit to radiant-maxar/geoint-deps that referenced this pull request Sep 14, 2022
jbronn added a commit to radiant-maxar/geoint-deps that referenced this pull request Sep 14, 2022
* Upgrade `step-cli` to 0.22.0 and `step-ca` to 0.22.1.

* Fix tzdata tests that broke with 2022b, refs ruby/spec#939.
@nobu
Copy link
Member

nobu commented Sep 16, 2022

What did this test intend?
Since I couldn't get the intention, just commented out.

Previously, the time seemed the edge case when the time zone rule changed, however the same day in 1970 seems not.

@junaruga
Copy link
Member Author

I assume that this test just wants to test Time#local with the Amsterdam timezone. The year doesn't matter.

@nobu
Copy link
Member

nobu commented Sep 22, 2022

Thank you, I see.
Just I've overthought it.

eregon pushed a commit to ruby/ruby that referenced this pull request Sep 27, 2022
This was disabled by b7577b4, while properly fixed upstream by:

ruby/spec#939
@eregon
Copy link
Member

eregon commented Sep 27, 2022

ruby/ruby#6393 merged

@eregon
Copy link
Member

eregon commented Sep 27, 2022

@nobu Please use quarantine! do for a problematic spec, or comment using # .
Commenting with =begin causes no merge conflict and so could easily disable specs silently and unintentionally, even if the spec was fixed like here.

@nobu
Copy link
Member

nobu commented Sep 30, 2022

@eregon I din't know quarantine!, thanks.

@nobu
Copy link
Member

nobu commented Sep 30, 2022

Still I'm curious why we should test a CET day, which isn't an edge-case.
Furthermore, aren't the tests in this file tests for tzdata, not for the Time class?

@mame
Copy link
Member

mame commented Sep 30, 2022

@junaruga @eregon I don't think this change makes sense.

According to the original commit of the spec, the intention of the spec was to test the moment when the time zone was changed.

According to this article, the timezone in Amsterdam was actually changed from NET to CEST on 16th May 1940.

There are no timezone changes in Amsterdam since 1970 (except for daylight saving time). So using post-1970 does not make sense for this spec. Instead, how about just removing the spec if it is difficult to test the intention due to tzdata?

@eregon
Copy link
Member

eregon commented Sep 30, 2022

Ah, I interpreted timezone changes as changing TZ which with_timezone does.
Yeah it looks like all 3 examples from that initial commit are gone/have changed, seems tzdata doesn't care about old timezones like that anymore.

I think we should keep the example and just fix the description.

nagachika pushed a commit to ruby/ruby that referenced this pull request Oct 1, 2022
@nobu
Copy link
Member

nobu commented Oct 1, 2022

What do you mean by “keep”?

@junaruga
Copy link
Member Author

junaruga commented Oct 2, 2022

According to this article, the timezone in Amsterdam was actually changed from NET to CEST on 16th May 1940.

I see. Thanks for the catching up. Now on the tzdata >= 2022b, the timezone in Amsterdam is WEST on 16th May 1940.

Instead, how about just removing the spec if it is difficult to test the intention due to tzdata?

I am ok with your idea.

@eregon
Copy link
Member

eregon commented Oct 6, 2022

What do you mean by “keep”?

4281f46

@nobu
Copy link
Member

nobu commented Oct 10, 2022

What do you mean by “keep”?

4281f46

Does keeping it has any meaning?
I think it should be removed too.

@eregon
Copy link
Member

eregon commented Oct 10, 2022

Does keeping it has any meaning?

Yes, it seems the only test in that file for TZ=Europe/Amsterdam => the Time zone is "CET", i.e., how the TZ value is resolved to something different than just $TZ.

@nobu
Copy link
Member

nobu commented Oct 10, 2022

Why is Amsterdam special to test now?
The $TZ change isn’t tested with America/New_York?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Failures with tzdata-2022b
4 participants