Add regression tests for 12-hour AM/PM parse and format by rootkiller6788 · Pull Request #371 · google/cctz

rootkiller6788 · 2026-08-20T06:53:41Z

The 12-hour (%I/%l with %p) conversion is handled subtly: %I maps 12 to 0 and a following %p shifts the afternoon hours. The boundary values "12 AM" (midnight) and "12 PM" (noon) are the cases most prone to cross-platform bugs (newlib's strptime handled "12 AM" incorrectly until recently).

These tests pin the correct behavior on every supported platform:

parse("%I %p", "12 AM") -> 00:00, parse("%I %p", "12 PM") -> 12:00
format("%I %p") renders midnight as "12 AM" and noon as "12 PM"
the glibc-only %r specifier is checked for the same boundary

No production code changes; this only adds regression coverage. Verified against a local build of the library sources.

devbww · 2026-08-20T16:40:27Z

+  EXPECT_EQ("12 AM", cctz::format("%I %p", tp, tz));
+  tp = convert(civil_second(1970, 1, 1, 12, 0, 0), tz);
+  EXPECT_EQ("12 PM", cctz::format("%I %p", tp, tz));
+}


These would be better sitting with the other "%p" expectations in Format.LocaleSpecific (they are locale specific, after all). So, instead, ...

@@ -217,6 +217,10 @@ TEST(Format, LocaleSpecific) { TestFormatSpecifier(tp, tz, "%x", "01/01/70"); TestFormatSpecifier(tp, tz, "%X", "00:00:00"); + // %I renders midnight and noon as 12, with %p disambiguating them. + TestFormatSpecifier(tp, tz, "%I %p", "12 AM"); + TestFormatSpecifier(tp + chrono::hours(12), tz, "%I %p", "12 PM"); + #if defined(__linux__) // SU/C99/TZ extensions TestFormatSpecifier(tp, tz, "%h", "Jan"); // Same as %b

devbww · 2026-08-20T16:41:30Z

+TEST(Parse, TwelveHour) {
+  const time_zone tz = utc_time_zone();
+  time_point<chrono::nanoseconds> tp;
+
+  // %I maps 12 to 0 and a following %p shifts the afternoon, so "12 AM" is
+  // midnight and "12 PM" is noon. These boundary values are the 12-hour cases
+  // most prone to cross-platform bugs (see, e.g., newlib's handling of %I).
+  EXPECT_TRUE(parse("%I %p", "12 AM", tz, &tp));
+  EXPECT_EQ(0, convert(tp, tz).hour());
+  EXPECT_TRUE(parse("%I %p", "12 PM", tz, &tp));
+  EXPECT_EQ(12, convert(tp, tz).hour());
+  EXPECT_TRUE(parse("%I %p", "01 AM", tz, &tp));
+  EXPECT_EQ(1, convert(tp, tz).hour());
+  EXPECT_TRUE(parse("%I %p", "11 PM", tz, &tp));
+  EXPECT_EQ(23, convert(tp, tz).hour());
+
+#if defined(__linux__)
+  // %r is the locale's full 12-hour time; it must land on the correct side of
+  // the AM/PM boundary too.
+  EXPECT_TRUE(parse("%r", "12:00:00 AM", tz, &tp));
+  EXPECT_EQ(0, convert(tp, tz).hour());
+  EXPECT_TRUE(parse("%r", "12:00:00 PM", tz, &tp));
+  EXPECT_EQ(12, convert(tp, tz).hour());
+#endif
+}


Similarly, these would also be better as part of Parse.LocaleSpecific, so, instead, ...

@@ -1080,6 +1094,22 @@ TEST(Parse, LocaleSpecific) { EXPECT_TRUE(parse("%I %p", "5 PM", tz, &tp)); EXPECT_EQ(17, convert(tp, tz).hour()); + // %I maps 12 to 0 and a following %p shifts to the afternoon, + // so "12 AM" is midnight and "12 PM" is noon. + tp = reset; + EXPECT_TRUE(parse("%I %p", "12 AM", tz, &tp)); + EXPECT_EQ(0, convert(tp, tz).hour()); + tp = reset; + EXPECT_TRUE(parse("%I %p", "12 PM", tz, &tp)); + EXPECT_EQ(12, convert(tp, tz).hour()); + + tp = reset; + EXPECT_TRUE(parse("%I %p", "01 AM", tz, &tp)); + EXPECT_EQ(1, convert(tp, tz).hour()); + tp = reset; + EXPECT_TRUE(parse("%I %p", "11 PM", tz, &tp)); + EXPECT_EQ(23, convert(tp, tz).hour()); + tp = reset; EXPECT_TRUE(parse("%x", "02/03/04", tz, &tp)); if (convert(tp, tz).month() == 2) {

and

@@ -1115,6 +1145,18 @@ TEST(Parse, LocaleSpecific) { EXPECT_EQ(44, convert(tp, tz).minute()); EXPECT_EQ(55, convert(tp, tz).second()); + tp = reset; + EXPECT_TRUE(parse("%r", "12:00:00 AM", tz, &tp)); + EXPECT_EQ(0, convert(tp, tz).hour()); + EXPECT_EQ(0, convert(tp, tz).minute()); + EXPECT_EQ(0, convert(tp, tz).second()); + + tp = reset; + EXPECT_TRUE(parse("%r", "12:00:00 PM", tz, &tp)); + EXPECT_EQ(12, convert(tp, tz).hour()); + EXPECT_EQ(0, convert(tp, tz).minute()); + EXPECT_EQ(0, convert(tp, tz).second()); + #if defined(__GLIBC__) tp = reset; EXPECT_TRUE(parse("%Ec", "Tue Nov 19 05:06:07 2013", tz, &tp));

devbww · 2026-08-20T16:48:07Z

In the PR description:

the cases most prone to cross-platform bugs (newlib's strptime handled "12 AM" incorrectly until recently).

I'm not sure what "cross-platform" is supposed to impart, so let's remove it.
Let's also remove mention of other libraries' issues.

So that just leaves, "the cases most prone to bugs."

Drop the 'cross-platform' qualifier and the reference to newlib's handling of %I, as requested in review, leaving just 'the 12-hour cases most prone to bugs.'

When parsing a 12-hour time with "%I %p", the parser re-parses the matched %p text against a known hour ("1" + the AM/PM token) to learn whether the platform's strptime() shifts the hour into the afternoon. Some strptime() implementations, notably the std::get_time()-based fallback used on Windows, cannot parse "%I%p" together and leave the hour unchanged, so PM times were treated as AM. When that check does not indicate afternoon, fall back to inspecting the matched %p text directly: a leading 'p' (case-insensitively) denotes PM. This makes the 12-hour AM/PM parse regression tests pass on Windows.

rootkiller6788 · 2026-08-22T10:02:57Z

Thank you for the review.

I've simplified the wording as requested: the "cross-platform" qualifier and the reference to newlib's handling are removed, leaving just "the 12-hour cases most prone to bugs." The matching comment in src/time_zone_format_test.cc was updated accordingly (commit 24f0ed5), and the PR description now uses the same wording.

While finalizing I also found that the %I %p PM assertions fail on Windows, where cctz's parse() uses the std::get_time()-based strptime() fallback. That fallback cannot parse "%I%p" together, so the parser's AM/PM probe never saw the expected 13-hour shift and treated PM as AM (this also affects the existing parse("%I %p", "5 PM") case in Parse.LocaleSpecific). I fixed the probe in src/time_zone_format.cc (commit 0b670f6): when the reparse is inconclusive, the parser now inspects the matched %p text directly and treats a leading p/P as afternoon. All of the 12-hour assertions now pass on Windows (verified with a local mingw build), and the existing %I %p / %r cases remain green.

I also merged the current master into the branch to keep it up to date (commit 21fd392).

Could you please take another look?

devbww · 2026-08-26T04:39:46Z

While finalizing I also found that the %I %p PM assertions fail on Windows, where cctz's parse() uses the std::get_time()-based strptime() fallback. That fallback cannot parse "%I%p" together, so the parser's AM/PM probe never saw the expected 13-hour shift and treated PM as AM (this also affects the existing parse("%I %p", "5 PM") case in Parse.LocaleSpecific). I fixed the probe in src/time_zone_format.cc (commit 0b670f6): when the reparse is inconclusive, the parser now inspects the matched %p text directly and treats a leading p/P as afternoon.

Please revert this. We're not in the business of working around bugs in a platform's std::get_time() implementation. And, even if we were, a leading /\s*[pP]/ would not be a robust match for a locale's equivalent of AM or PM: in locales that use a 24-hour clock, for example, %p matches an empty string, while many others use localized characters.

Could you please take another look?

Don't forget to move the new expectations into Format.LocaleSpecific and Parse.LocaleSpecific, as suggested above.

rootkiller6788 · 2026-08-26T15:27:17Z

Got it. I will revert the custom %p probe and move the test cases into Format.LocaleSpecific and Parse.LocaleSpecific. Please take another look once updated.

…allback" This reverts commit 0b670f6.

The %I/%p and %r boundary checks are locale-specific, so they belong with the existing %p expectations in Format.LocaleSpecific and Parse.LocaleSpecific rather than standing alone in TwelveHour tests.

rootkiller6788 · 2026-08-26T16:19:30Z

Thank you for the follow-up. Both points are addressed:

Reverted the %p probe (faf8236) — src/time_zone_format.cc is back to the upstream logic, with the AM/PM determination again based solely on the %I%p reparse. No workaround for the std::get_time() fallback remains.
Moved the new expectations into Format.LocaleSpecific and Parse.LocaleSpecific (57c6241) at the locations you suggested — the standalone TwelveHour tests are removed, and the %r boundary cases sit with the existing %r case under the __linux__ guard.

I verified the assertions against a local build of the library: the pre-existing parse("%I %p", "5 PM") case and the new 12 AM / 12 PM / 01 AM / 11 PM cases all pass, and format("%I %p") renders "12 AM" / "12 PM".

Could you please take another look?

devbww

Looks good to me. Thanks.

Passing off to a Google owner now; perhaps @derekmauro or @pkumar0508.

devbww · 2026-09-09T17:10:19Z

Looks good to me. Thanks.

Passing off to a Google owner now; perhaps @derekmauro or @pkumar0508.

@derekmauro: These new expectations appear to pass, so I am fine with merging them. Thanks.

Add regression tests for 12-hour AM/PM parse and format

f5acba5

devbww suggested changes Aug 20, 2026

View reviewed changes

rootkiller6788 added 3 commits August 21, 2026 23:31

Reword 12-hour parse test comment per review

24f0ed5

Drop the 'cross-platform' qualifier and the reference to newlib's handling of %I, as requested in review, leaving just 'the 12-hour cases most prone to bugs.'

Merge remote-tracking branch 'origin/master' into test-twelve-hour

21fd392

rootkiller6788 added 2 commits August 26, 2026 23:29

Revert "Fix AM/PM detection with the std::get_time()-based strptime f…

faf8236

…allback" This reverts commit 0b670f6.

Fold 12-hour expectations into the LocaleSpecific tests

57c6241

The %I/%p and %r boundary checks are locale-specific, so they belong with the existing %p expectations in Format.LocaleSpecific and Parse.LocaleSpecific rather than standing alone in TwelveHour tests.

rootkiller6788 marked this pull request as ready for review August 26, 2026 16:19

devbww approved these changes Aug 26, 2026

View reviewed changes

Merge branch 'master' into test-twelve-hour

8a8f0b5

derekmauro approved these changes Sep 10, 2026

View reviewed changes

derekmauro merged commit 731e7de into google:master Sep 10, 2026
14 checks passed

Navigation Menu

Uh oh!

Add regression tests for 12-hour AM/PM parse and format - #371

Add regression tests for 12-hour AM/PM parse and format#371
derekmauro merged 7 commits into
google:masterfrom
rootkiller6788:test-twelve-hour

rootkiller6788 commented Aug 20, 2026

Uh oh!

devbww Aug 20, 2026

Uh oh!

devbww Aug 20, 2026

Uh oh!

devbww commented Aug 20, 2026

Uh oh!

rootkiller6788 commented Aug 22, 2026

Uh oh!

devbww commented Aug 26, 2026

Uh oh!

rootkiller6788 commented Aug 26, 2026

Uh oh!

rootkiller6788 commented Aug 26, 2026

Uh oh!

devbww left a comment

Uh oh!

devbww commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rootkiller6788 commented Aug 20, 2026

Uh oh!

devbww Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

devbww Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

devbww commented Aug 20, 2026

Uh oh!

rootkiller6788 commented Aug 22, 2026

Uh oh!

devbww commented Aug 26, 2026

Uh oh!

rootkiller6788 commented Aug 26, 2026

Uh oh!

rootkiller6788 commented Aug 26, 2026

Uh oh!

devbww left a comment

Choose a reason for hiding this comment

Uh oh!

devbww commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants