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.

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

Conversation

waltdisgrace
Copy link
Contributor

closes T188

=#=#=#= Begin test: Like the previous, with exotic timezone to avoid false negative =#=#=#=
Date: 1556246574
=#=#=#= 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

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

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.

@clumens
Copy link
Contributor

clumens commented Mar 19, 2025

@waltdisgrace Rebase on main and regenerate the regression test output and you should be good. Merging @nrwahl2's PR for crm_resource is what's causing you problems here.

@clumens clumens added the review: in progress PRs that are currently being reviewed label Mar 21, 2025
@waltdisgrace
Copy link
Contributor Author

retest this please

@clumens clumens added the needs attention PRs that someone needs to look at label Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs attention PRs that someone needs to look at review: in progress PRs that are currently being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants