Skip to content

Add some tests for iso8601 parsing #3846

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −0
Open

Add some tests for iso8601 parsing #3846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cts/cli/regression.dates.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,14 @@ Duration: 3 months
Duration ends at: 2009-04-30 00:00:00Z
=#=#=#= End test: 2009-01-31 + 3 Months - OK (0) =#=#=#=
* Passed: iso8601 - 2009-01-31 + 3 Months
=#=#=#= Begin test: Test correct immediate plus-sign at timezone parsing =#=#=#=
Date: 1556242974
=#=#=#= End test: Test correct immediate plus-sign at timezone parsing - OK (0) =#=#=#=
* Passed: iso8601 - Test correct immediate plus-sign at timezone parsing
=#=#=#= Begin test: Like the previous, with exotic timezone to avoid false negative =#=#=#=
Date: 1556242974
=#=#=#= End test: Like the previous, with exotic timezone to avoid false negative - OK (0) =#=#=#=
* Passed: iso8601 - Like the previous, with exotic timezone to avoid false negative
Copy link
Contributor

Choose a reason for hiding this comment

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

@waltdisgrace Tests are failing because of this:

[2025-03-17T20:03:55.904Z]  =#=#=#= Begin test: Test correct immediate plus-sign at timezone parsing =#=#=#=
[2025-03-17T20:03:55.904Z] -Date: 1556246574
[2025-03-17T20:03:55.904Z] +Date: 1556242974

The difference between those two numbers is 3600 seconds, which is one hour, which is way too round a number to be a coincidence. When I run it here, I get the same number as CI does. How did you generate the new test output? Do you perhaps have your timezone set to something unusual?

Copy link
Contributor

Choose a reason for hiding this comment

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

We recently switched to daylight saving time. I would not be surprised if that's playing a role somehow. For example, if the system's timezone has an effect and if one of you has your system set to UTC and the other has your system set to local time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think system time zone is supposed to play a role here though? Maybe there's some sort of rounding behavior that's implementation-dependent and differs on your two machines?

Copy link
Contributor

@nrwahl2 nrwahl2 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Besides, shouldn't the resulting time be 2h20m later in the second test? (or earlier maybe)

Copy link
Contributor

@nrwahl2 nrwahl2 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There's a bug in our parsing. The time zone is parsed only if there's a space before the + or - sign. (Note that my machine is on PDT: UTC-0700.)

$ iso8601 -d '2019-04-25T21:42:54' -S
Date: 1556253774

$ iso8601 -d '2019-04-25T21:42:54+0200' -S
Date: 1556253774

$ iso8601 -d '2019-04-25T21:42:54 +0200' -S
Date: 1556221374

Copy link
Contributor

Choose a reason for hiding this comment

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

@waltdisgrace Yeah, looks like our parsing code is wrong. If you're curious, you can get a free version of the ISO8601 standard here:

https://www.loc.gov/standards/datetime/iso-tc154-wg5_n0038_iso_wd_8601-1_2016-02-16.pdf

Section 3.4.1 is pretty explicit that spaces aren't allowed in the time strings, and section 4.2.5.2 restates that again for the time offset portion. So, the fact that we do not parse the timezone if there's a space means we're not handling the standard correctly.

This might be more than you were looking to get into, but it would be worth taking a look and seeing if you could fix that bug. Of course, nothing is easy and we'll need to support our buggy behavior for now as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

the fact that we do not parse the timezone if there's a space means we're not handling the standard correctly.

The fact that we don't parse the timezone UNLESS there's a space means we're not handling the standard correctly.

  • Standard: Timezone is valid only WITH NO space.
  • Pacemaker: Timezone is valid only WITH a space, and is ignored WITH NO space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, yes. That's what I meant to say. One day I'll learn to stop typing comments and talking in slack and talking in irc at the same time.

=#=#=#= Begin test: 2009-03-31 - 1 Month =#=#=#=
Date: 2009-03-31 00:00:00Z
Duration: -1 months
Expand Down
4 changes: 4 additions & 0 deletions cts/cts-cli.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,10 @@ class DatesRegressionTest(RegressionTest):
"iso8601 -d '2009-01-31 00:00:00Z' -D P2M -E '2009-03-31 00:00:00Z'"),
Test("2009-01-31 + 3 Months",
"iso8601 -d '2009-01-31 00:00:00Z' -D P3M -E '2009-04-30 00:00:00Z'"),
Test("Test correct immediate plus-sign at timezone parsing",
"iso8601 -d '2019-04-25T21:42:54+0200' -S"),
Test("Like the previous, with exotic timezone to avoid false negative",
"iso8601 -d '2019-04-25T21:42:54+0420' -S"),
make_test_group("2009-03-31 - 1 Month",
"iso8601 {fmt} -d '2009-03-31 01:00:00 +01:00' -D P-1M -E '2009-02-28 00:00:00Z'",
[Test, ValidatingTest]),
Expand Down