From 04c82cacfe9ee1a5a775c47c19361a0a5f0343a3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Nov 2024 16:26:04 -0500 Subject: [PATCH 1/3] Let `time::parse::relative::various` fail more often This tests four inputs representing relative times, instead of one, in order to: 1. Reveal #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 #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 #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] --- gix-date/tests/time/parse.rs | 44 ++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/gix-date/tests/time/parse.rs b/gix-date/tests/time/parse.rs index 545fbb7fc19..bb598335d5d 100644 --- a/gix-date/tests/time/parse.rs +++ b/gix-date/tests/time/parse.rs @@ -179,24 +179,34 @@ mod relative { #[test] fn various() { let now = SystemTime::now(); - let two_weeks_ago = gix_date::parse("2 weeks ago", Some(now)).unwrap(); - assert_eq!(Sign::Plus, two_weeks_ago.sign); - assert_eq!(0, two_weeks_ago.offset); - let expected = Zoned::try_from(now) - .unwrap() - // account for the loss of precision when creating `Time` with seconds - .round( - jiff::ZonedRound::new() - .smallest(jiff::Unit::Second) - .mode(jiff::RoundMode::Trunc), - ) - .unwrap() - .saturating_sub(2.weeks()); - assert_eq!( - jiff::Timestamp::from_second(two_weeks_ago.seconds).unwrap(), - expected.timestamp(), - "relative times differ" + + let cases = [ + ("2 weeks ago", 2.weeks()), + ("20160 minutes ago", 20_160.minutes()), // 2 weeks + ("20 weeks ago", 20.weeks()), + ("201600 minutes ago", 201_600.minutes()), // 20 weeks + ]; + + let times = cases.map(|(input, _)| gix_date::parse(input, Some(now)).unwrap()); + + assert_eq!(times.map(|_| Sign::Plus), times.map(|time| time.sign)); + assert_eq!(times.map(|_| 0), times.map(|time| time.offset)); + + let expected = cases.map(|(_, span)| + Zoned::try_from(now) + .unwrap() + // account for the loss of precision when creating `Time` with seconds + .round( + jiff::ZonedRound::new() + .smallest(jiff::Unit::Second) + .mode(jiff::RoundMode::Trunc), + ) + .unwrap() + .saturating_sub(span) + .timestamp() ); + let actual = times.map(|time| jiff::Timestamp::from_second(time.seconds).unwrap()); + assert_eq!(actual, expected, "relative times differ"); } } From 5d51bd10f67bf4099ca832a5ca13de4e87fe0681 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Nov 2024 16:33:58 -0500 Subject: [PATCH 2/3] Add even longer duration to test; run `cargo fmt` 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 #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 #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] --- gix-date/tests/time/parse.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gix-date/tests/time/parse.rs b/gix-date/tests/time/parse.rs index bb598335d5d..69a273f5235 100644 --- a/gix-date/tests/time/parse.rs +++ b/gix-date/tests/time/parse.rs @@ -185,6 +185,8 @@ mod relative { ("20160 minutes ago", 20_160.minutes()), // 2 weeks ("20 weeks ago", 20.weeks()), ("201600 minutes ago", 201_600.minutes()), // 20 weeks + ("40 weeks ago", 40.weeks()), + ("403200 minutes ago", 403_200.minutes()), // 40 weeks ]; let times = cases.map(|(input, _)| gix_date::parse(input, Some(now)).unwrap()); @@ -192,7 +194,7 @@ mod relative { assert_eq!(times.map(|_| Sign::Plus), times.map(|time| time.sign)); assert_eq!(times.map(|_| 0), times.map(|time| time.offset)); - let expected = cases.map(|(_, span)| + let expected = cases.map(|(_, span)| { Zoned::try_from(now) .unwrap() // account for the loss of precision when creating `Time` with seconds @@ -204,7 +206,7 @@ mod relative { .unwrap() .saturating_sub(span) .timestamp() - ); + }); let actual = times.map(|time| jiff::Timestamp::from_second(time.seconds).unwrap()); assert_eq!(actual, expected, "relative times differ"); } From ac17b62a5c9d63606e9c161c1533cdbebf2de977 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Nov 2024 22:53:06 -0500 Subject: [PATCH 3/3] Test 12-week increments from 2 to 50 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 #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. --- Cargo.lock | 1 + gix-date/Cargo.toml | 3 ++- gix-date/tests/time/parse.rs | 15 ++++++++++----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dcfc0729ce5..94f8ed1885a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1690,6 +1690,7 @@ dependencies = [ "itoa", "jiff", "once_cell", + "pretty_assertions", "serde", "thiserror 2.0.3", ] diff --git a/gix-date/Cargo.toml b/gix-date/Cargo.toml index 2d911059938..94c98dbcf85 100644 --- a/gix-date/Cargo.toml +++ b/gix-date/Cargo.toml @@ -28,9 +28,10 @@ thiserror = "2.0.0" document-features = { version = "0.2.0", optional = true } [dev-dependencies] +gix-hash = { path = "../gix-hash" } gix-testtools = { path = "../tests/tools" } once_cell = "1.12.0" -gix-hash = { path = "../gix-hash" } +pretty_assertions = "1.4.1" [package.metadata.docs.rs] all-features = true diff --git a/gix-date/tests/time/parse.rs b/gix-date/tests/time/parse.rs index 69a273f5235..bee1ac4c856 100644 --- a/gix-date/tests/time/parse.rs +++ b/gix-date/tests/time/parse.rs @@ -156,6 +156,7 @@ mod relative { use gix_date::time::Sign; use jiff::{ToSpan, Zoned}; + use pretty_assertions::assert_eq; #[test] fn large_offsets() { @@ -182,11 +183,15 @@ mod relative { let cases = [ ("2 weeks ago", 2.weeks()), - ("20160 minutes ago", 20_160.minutes()), // 2 weeks - ("20 weeks ago", 20.weeks()), - ("201600 minutes ago", 201_600.minutes()), // 20 weeks - ("40 weeks ago", 40.weeks()), - ("403200 minutes ago", 403_200.minutes()), // 40 weeks + ("14 weeks ago", 14.weeks()), + ("26 weeks ago", 26.weeks()), + ("38 weeks ago", 38.weeks()), + ("50 weeks ago", 50.weeks()), + ("20160 minutes ago", 20_160.minutes()), // 2 weeks + ("141120 minutes ago", 141_120.minutes()), // 14 weeks + ("262080 minutes ago", 262_080.minutes()), // 26 weeks + ("383040 minutes ago", 383_040.minutes()), // 38 weeks + ("504000 minutes ago", 504_000.minutes()), // 50 weeks ]; let times = cases.map(|(input, _)| gix_date::parse(input, Some(now)).unwrap());