Skip to content

Reveal local-time DST ambiguity where it occurs #1697

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

Jump to bottom
Merged
merged 3 commits into from
Nov 23, 2024
Merged

Reveal local-time DST ambiguity where it occurs #1697

merged 3 commits into from
Nov 23, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Nov 23, 2024 •
edited
Loading

This more effectively observes #1696 by expanding gix-date::date time::parse::relative::various to have multiple sub-cases. That issue describes a procedure to modify the test code to bring back the failure. This supersedes that procedure. This is to say that, with the changes in this PR, a system where the bug can be observed at any time of the year should always observe it.

Two very important things this does not do:

  • This neither fixes nor in any way mitigates #1696.

  • No failure occurs, and thus #1696 remains unobserved, when

    1. The system is set (or overridden via TZ) to UTC. Thus, this still does not cause a failure on CI. Or
    2. The system is set to local time, but in a time zone that does not make daylight savings related clock adjustments. I believe most countries actually do not observe daylight savings clock adjustments, though many do.

To make the results easy to understand and to show full results in the known failure cases, this does not place the existing code inside an outer loop, but instead populates arrays corresponding to the cases, so that assertions can be made comparing the arrays. pretty_assertions::assert_eq! is used, in the same manner as elsewhere in the test suite, to allow such output to be easily readable.

In addition to testing more cases, for each "X weeks ago" case, a corresponding "Y minutes ago" case is also included, where $Y = 10080 X$. Failures occur only in "X weeks ago" cases (that include exactly one daylight saving clock adjustment).

Even accounting for the weeks/minutes doubling up of sub-cases, and for variations in daylight savings rules creating the need to check more intervals than one might intuitively expect to need to check, I think this this has more sub-cases than needed. ac17b62 includes a description of why I did it this way and how it might be improved.

I have verified on GNU/Linux, macOS, and Windows that these tests fail in the expected way when run in North American Eastern Time (America/New York), but pass when TZ=UTC is set. That failure does not occur on CI is further demonstration that when UTC is configured there is not failure (which we should expect, since UTC has no DST-related adjustments). Because CI does not observe the failures, relevant failure output is included in each commit message.

@EliahKagan
Let time::parse::relative::various fail more often
04c82ca 
This tests four inputs representing relative times, instead of one,
in order to:

1. Reveal GitoxideLabs#1696 during more of the year when the tests are run on
   machines set to local time in time zones whose rules include
   daylight savings clock adjustments.

2. Illuminate the nature and limited extent of GitoxideLabs#1696 by trying both
   "weeks ago" and "minutes ago" input.

   (It looks like the "minutes ago" input always passes the test,
   while the "weeks ago" input can fail the test if the interval
   includes a DST adjustment.)

3. Cover a wider range of inputs more generally, which is probably
   a good idea even where GitoxideLabs#1696 is not involved.

Although these change intend to, and appear to succeed at,
triggering more failures due to that but on affected systems set to
local time, they are not expected to produce any new failures on
CI, since all platforms' GitHub-hosted GHA runners are set to use
UTC.

With these changes, the failure, when it occurs, looks like:

    --- STDERR:              gix-date::date time::parse::relative::various ---
    thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:209:9:
    assertion `left == right` failed: relative times differ
      left: [2024-11-08T21:10:19Z, 2024-11-08T21:10:19Z, 2024-07-05T21:10:19Z, 2024-07-05T21:10:19Z]
     right: [2024-11-08T21:10:19Z, 2024-11-08T21:10:19Z, 2024-07-05T20:10:19Z, 2024-07-05T21:10:19Z]
@EliahKagan
Add even longer duration to test; run cargo fmt
5d51bd1 
This duration, 40 weeks, serves to demonstrate that the actual
versus expected value disagreement of 1 hour is due to DST
adjustments. Even where DST is observed, adjustments would
ordinarily have been done twice in 40 weeks. So this case should
not fail, even when the 20 weeks case does.

However, the test remains imperfect, even assuming it is run with
the time zone set to local time and in a place where DST adustments
are made, because there will sometimes never have been exactly one
DST adjustment in any of the last 2, 20, or 40 weeks.

Better temporal coverage should probably be put in place, but
something like the 40 weeks case might still make sense to keep in,
so that we are more likely to catch a possible variant of GitoxideLabs#1696
where the time zone rules had at one time had daylight savings but
were changed to no longer have it (or vice versa). It's possible
that GitoxideLabs#1696 might be fixed by a change to the tested code, or to the
test, while still leaving such a variant in place.

Failure, when it occurs, now looks like:

    --- STDERR:              gix-date::date time::parse::relative::various ---
    thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:211:9:
    assertion `left == right` failed: relative times differ
      left: [2024-11-08T21:14:45Z, 2024-11-08T21:14:45Z, 2024-07-05T21:14:45Z, 2024-07-05T21:14:45Z, 2024-02-16T21:14:45Z, 2024-02-16T21:14:45Z]
     right: [2024-11-08T21:14:45Z, 2024-11-08T21:14:45Z, 2024-07-05T20:14:45Z, 2024-07-05T21:14:45Z, 2024-02-16T21:14:45Z, 2024-02-16T21:14:45Z]
@EliahKagan
Test 12-week increments from 2 to 50
ac17b62 
The idea here is to make it so that:

1. If we are using local time, in a time zone with DST adjustments,
   then no matter when we run the test, at least one "X weeks ago"
   case will have exactly one DST adjustment between then and now.
   (Thus, if our local time can observe GitoxideLabs#1696, it will.)

2. It is easy to verify that this is so.

The rules are somewhat variable, and adjustments cannot be
approximated as happening every half-year:

- In multiple time zones with DST-related adjustments, an
  adjustment and its reversal can be separated by only four months
  (November to March).

- Countries may institute rules that may be hard for people not
  familiar with them to anticipate. For example, Morocco used to
  have DST-related adjustments; when it did, if the (lunar) month
  of Ramadan occurred during the usual (Gregorian-based) DST
  period, standard time (rather than DST) was in effect during
  Ramadan. Thus, in some years, there were four DST-related clock
  adjustments.

  It is hard to predict (or, at least, I do not know how to
  predict) if, or what kind of, new DST adjustments may be put in
  place somewhere.

With that said, the current test probably has more intervals than
necessary. Furthermore, if it turns out to be feasible, it may be
better to make this a true unit test by using a set time instad of
`now` to take "X weeks ago" times relative to, and by somehow
specifying or mocking out the time zone, so as to test the tricky
cases in exactly the same way no matter where, when, or in what
local configuration the test is run.

Failure, when it occurs, now looks like:

    --- STDERR:              gix-date::date time::parse::relative::various ---
    thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:216:9:
    assertion failed: `(left == right)`: relative times differ

    Diff < left / right > :
     [
         2024-11-09T04:22:21Z,
    <    2024-08-17T04:22:21Z,
    <    2024-05-25T04:22:21Z,
    >    2024-08-17T03:22:21Z,
    >    2024-05-25T03:22:21Z,
         2024-03-02T04:22:21Z,
         2023-12-09T04:22:21Z,
         2024-11-09T04:22:21Z,
         2024-08-17T04:22:21Z,
         2024-05-25T04:22:21Z,
         2024-03-02T04:22:21Z,
         2023-12-09T04:22:21Z,
     ]

Equivalent cases with weeks and minutes were interleaved before,
but now all the "weeks" cases are listed before all the "minutes"
cases, because that produces more readable output when
`pretty_assertions::assert_eq!` is used.
@EliahKagan EliahKagan mentioned this pull request Nov 23, 2024
Closed
Byron
Byron approved these changes Nov 23, 2024
View reviewed changes
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving the observability of the test!

I will attempt to share more in the related ticket.

@Byron Byron merged commit 438ee47 into GitoxideLabs:main Nov 23, 2024
20 checks passed
@EliahKagan EliahKagan deleted the run-ci/duration branch November 23, 2024 08:37
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Nov 25, 2024
@EliahKagan
feat: Parse relative months and years
3082d40 
This extends `gix_date::parse::relative::span` to recognize times
with "months" and "years", as in "3 months ago" and "2 years ago".

The actual change here is very simple, because it is directly
facilitated by GitoxideLabs#1474.

The units "seconds", "minutes", "hours", "days", and "weeks"
continue to be recognized (as before). Relative times like
"1 year, 2 months ago" remain unrecognized.

For background, see 43b6c06 (GitoxideLabs#498), c5c6bf6,
GitoxideLabs#1474 (comment),
and
GitoxideLabs#1696 (comment).

Note that this change does not fix issue GitoxideLabs#1696, which continues to
apply to the intepretation of "days" and "weeks", and now also
applies in the same way to the interpretation of "months" and
"years". (It continues not to apply to the interpretation of
"seconds", "minutes", and "hours".)

The tests are updated to cover "months" and "years", as well as to
exercise a wider range of relative times, including showing which
units (e.g. "days") are affected by GitoxideLabs#1696. A few sub-cases, of
those adjusted or added here, test strings from a related test in
Git, to allow comparison. But most are not related to cases there.

As before, the tests pass on CI, or when the time zone is otherwise
UTC, or with local time zones that never have DST adjustments.

The sub-cases are reordered to be monotone increasing in the
magnitude of the relative intervals tested (and thus monotone
decreasing in the associated absolute timestamps), and the original
input is kept zipped into the `actual` and `expected` arrays being
compared. This way, if the assertion fails (such as due to GitoxideLabs#1696),
it is clear and readable which sub-cases failed and exactly how, as
well as what all the sub-cases are and what each sub-case tests.

Reordering the sub-cases and preserving the inputs they parse in
this way avoids the disadvantages of GitoxideLabs#1697 while keeping, and
expanding on, its benefits.

Failure, where it occurs, now looks like:

    --- STDERR:              gix-date::date time::parse::relative::various ---
    thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:252:9:
    assertion failed: `(left == right)`: relative times differ

    Diff < left / right > :
     [
         (
             "5 seconds ago",
             2024-11-24T23:51:49Z,
         ),
         (
             "5 minutes ago",
             2024-11-24T23:46:54Z,
         ),
         (
             "5 hours ago",
             2024-11-24T18:51:54Z,
         ),
         (
             "5 days ago",
             2024-11-19T23:51:54Z,
         ),
         (
             "3 weeks ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "21 days ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "504 hours ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "30240 minutes ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "2 months ago",
    <        2024-09-24T23:51:54Z,
    >        2024-09-24T22:51:54Z,
         ),
         (
             "1460 hours ago",
             2024-09-25T03:51:54Z,
         ),
         (
             "87600 minutes ago",
             2024-09-25T03:51:54Z,
         ),
         (
             "14 weeks ago",
    <        2024-08-18T23:51:54Z,
    >        2024-08-18T22:51:54Z,
         ),
         (
             "98 days ago",
    <        2024-08-18T23:51:54Z,
    >        2024-08-18T22:51:54Z,
         ),
         (
             "2352 hours ago",
             2024-08-18T23:51:54Z,
         ),
         (
             "141120 minutes ago",
             2024-08-18T23:51:54Z,
         ),
         (
             "5 months ago",
    <        2024-06-24T23:51:54Z,
    >        2024-06-24T22:51:54Z,
         ),
         (
             "3650 hours ago",
             2024-06-25T21:51:54Z,
         ),
         (
             "219000 minutes ago",
             2024-06-25T21:51:54Z,
         ),
         (
             "26 weeks ago",
    <        2024-05-26T23:51:54Z,
    >        2024-05-26T22:51:54Z,
         ),
         (
             "182 days ago",
    <        2024-05-26T23:51:54Z,
    >        2024-05-26T22:51:54Z,
         ),
         (
             "4368 hours ago",
             2024-05-26T23:51:54Z,
         ),
         (
             "262080 minutes ago",
             2024-05-26T23:51:54Z,
         ),
         (
             "8 months ago",
    <        2024-03-24T23:51:54Z,
    >        2024-03-24T22:51:54Z,
         ),
         (
             "5840 hours ago",
             2024-03-26T15:51:54Z,
         ),
         (
             "350400 minutes ago",
             2024-03-26T15:51:54Z,
         ),
         (
             "38 weeks ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "266 days ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "6384 hours ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "383040 minutes ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "11 months ago",
             2023-12-24T23:51:54Z,
         ),
         (
             "8030 hours ago",
             2023-12-26T09:51:54Z,
         ),
         (
             "481800 minutes ago",
             2023-12-26T09:51:54Z,
         ),
         (
             "14 months ago",
    <        2023-09-24T23:51:54Z,
    >        2023-09-24T22:51:54Z,
         ),
         (
             "21 months ago",
             2023-02-24T23:51:54Z,
         ),
         (
             "2 years ago",
             2022-11-24T23:51:54Z,
         ),
         (
             "20 years ago",
             2004-11-24T23:51:54Z,
         ),
         (
             "630720000 seconds ago",
             2004-11-29T23:51:54Z,
         ),
     ]
@EliahKagan EliahKagan mentioned this pull request Nov 25, 2024
Merged
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Nov 25, 2024
@EliahKagan
fix: Parse relative months and years
c95135b 
This extends `gix_date::parse::relative::span` to recognize times
with "months" and "years", as in "3 months ago" and "2 years ago".
Those units are supported by Git.

The actual change here to the parsing code is very simple, because
it is directly facilitated by GitoxideLabs#1474.

The units "seconds", "minutes", "hours", "days", and "weeks"
continue to be recognized (as before). Relative times like
"1 year, 2 months ago" remain unrecognized.

For background, see 43b6c06 (GitoxideLabs#498), c5c6bf6,
GitoxideLabs#1474 (comment),
and
GitoxideLabs#1696 (comment).

Note that this specific change does not itself fix issue GitoxideLabs#1696,
which applies to the intepretation of "days" and "weeks", and now
also applies in the same way to the interpretation of "months" and
"years". (It continues not to apply to the interpretation of
"seconds", "minutes", and "hours".)

The tests are updated to cover "months" and "years", as well as to
exercise a wider range of relative times, including showing which
units (e.g. "days") are affected by GitoxideLabs#1696. A few sub-cases, of
those adjusted or added here, test strings from a related test in
Git, to allow comparison. But most are not related to cases there.

As before, the tests pass on CI, or when the time zone is otherwise
UTC, or with local time zones that never have DST adjustments.

The sub-cases are reordered to be monotone increasing in the
magnitude of the relative intervals tested (and thus monotone
decreasing in the associated absolute timestamps), and the original
input is kept zipped into the `actual` and `expected` arrays being
compared. This way, if the assertion fails (such as due to GitoxideLabs#1696),
it is clear and readable which sub-cases failed and exactly how, as
well as what all the sub-cases are and what each sub-case tests.

Reordering the sub-cases and preserving the inputs they parse in
this way avoids the disadvantages of GitoxideLabs#1697 while keeping, and
expanding on, its benefits.

Failure, where it occurs, now looks like:

    --- STDERR:              gix-date::date time::parse::relative::various ---
    thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:252:9:
    assertion failed: `(left == right)`: relative times differ

    Diff < left / right > :
     [
         (
             "5 seconds ago",
             2024-11-24T23:51:49Z,
         ),
         (
             "5 minutes ago",
             2024-11-24T23:46:54Z,
         ),
         (
             "5 hours ago",
             2024-11-24T18:51:54Z,
         ),
         (
             "5 days ago",
             2024-11-19T23:51:54Z,
         ),
         (
             "3 weeks ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "21 days ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "504 hours ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "30240 minutes ago",
             2024-11-03T23:51:54Z,
         ),
         (
             "2 months ago",
    <        2024-09-24T23:51:54Z,
    >        2024-09-24T22:51:54Z,
         ),
         (
             "1460 hours ago",
             2024-09-25T03:51:54Z,
         ),
         (
             "87600 minutes ago",
             2024-09-25T03:51:54Z,
         ),
         (
             "14 weeks ago",
    <        2024-08-18T23:51:54Z,
    >        2024-08-18T22:51:54Z,
         ),
         (
             "98 days ago",
    <        2024-08-18T23:51:54Z,
    >        2024-08-18T22:51:54Z,
         ),
         (
             "2352 hours ago",
             2024-08-18T23:51:54Z,
         ),
         (
             "141120 minutes ago",
             2024-08-18T23:51:54Z,
         ),
         (
             "5 months ago",
    <        2024-06-24T23:51:54Z,
    >        2024-06-24T22:51:54Z,
         ),
         (
             "3650 hours ago",
             2024-06-25T21:51:54Z,
         ),
         (
             "219000 minutes ago",
             2024-06-25T21:51:54Z,
         ),
         (
             "26 weeks ago",
    <        2024-05-26T23:51:54Z,
    >        2024-05-26T22:51:54Z,
         ),
         (
             "182 days ago",
    <        2024-05-26T23:51:54Z,
    >        2024-05-26T22:51:54Z,
         ),
         (
             "4368 hours ago",
             2024-05-26T23:51:54Z,
         ),
         (
             "262080 minutes ago",
             2024-05-26T23:51:54Z,
         ),
         (
             "8 months ago",
    <        2024-03-24T23:51:54Z,
    >        2024-03-24T22:51:54Z,
         ),
         (
             "5840 hours ago",
             2024-03-26T15:51:54Z,
         ),
         (
             "350400 minutes ago",
             2024-03-26T15:51:54Z,
         ),
         (
             "38 weeks ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "266 days ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "6384 hours ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "383040 minutes ago",
             2024-03-03T23:51:54Z,
         ),
         (
             "11 months ago",
             2023-12-24T23:51:54Z,
         ),
         (
             "8030 hours ago",
             2023-12-26T09:51:54Z,
         ),
         (
             "481800 minutes ago",
             2023-12-26T09:51:54Z,
         ),
         (
             "14 months ago",
    <        2023-09-24T23:51:54Z,
    >        2023-09-24T22:51:54Z,
         ),
         (
             "21 months ago",
             2023-02-24T23:51:54Z,
         ),
         (
             "2 years ago",
             2022-11-24T23:51:54Z,
         ),
         (
             "20 years ago",
             2004-11-24T23:51:54Z,
         ),
         (
             "630720000 seconds ago",
             2004-11-29T23:51:54Z,
         ),
     ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Byron Byron Byron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EliahKagan @Byron