jkbrzt / rrule

JavaScript library for working with recurrence rules for calendar dates as defined in the iCalendar RFC and more.
https://jkbrzt.github.io/rrule
Other
3.27k stars 507 forks source link

Tests fail when run from TZ=Pacific/Kiritimati #537

Closed dereekb closed 9 months ago

dereekb commented 2 years ago

Reporting an issue

Thank you for taking an interest in rrule! Please include the following in your report:

rrule 2.7.1 macOS Monterey 12.2 Sat Jul 16 07:59:29 +14 2022

Most of the tests will fail with the date being one day in the future when running the tests using the following: TZ=Pacific/Kiritimati npm run test. Mind this is not internally using timezones, but having a machine being in the +14 timezone Pacific/Kiritimati.

Example Output:

  271) rrulestr
       testStrNWeekDay [every year on the 1st Tuesday and last Thursday for 3 times] [DTSTART:19970902T090000Z
RRULE:FREQ=YEARLY;COUNT=3;BYDAY=+1TU,-1TH]:

       - 1/3
      + expected - actual

      -Thu Dec 25 1997 23:00:00 GMT+1400 (Line Islands Time)
      +Fri Dec 26 1997 23:00:00 GMT+1400 (Line Islands Time)

      at assertDatesEqual (test/lib/utils.ts:28:59)
      at Context.<anonymous> (test/lib/utils.ts:123:5)
      at processImmediate (node:internal/timers:466:21)

  272) rrulestr
       testStrNWeekDayLarge [every year on the 13th Tuesday and 13th last Thursday for 3 times] [DTSTART:19970902T090000Z
RRULE:FREQ=YEARLY;COUNT=3;BYDAY=+13TU,-13TH]:

       - 1/3
      + expected - actual

      -Thu Oct 02 1997 23:00:00 GMT+1400 (Line Islands Time)
      +Fri Oct 03 1997 23:00:00 GMT+1400 (Line Islands Time)

      at assertDatesEqual (test/lib/utils.ts:28:59)
      at Context.<anonymous> (test/lib/utils.ts:123:5)
      at processImmediate (node:internal/timers:466:21)
dereekb commented 2 years ago

I've picked out the following test to start debugging from:

 1) RRuleSet
       valueOf
         generates correcty zoned recurrences when a tzid is present:

I added some logging within the iter() function:

Screen Shot 2022-07-15 at 2 56 05 PM

The image on the left is in TZ=Pacific/Kiritimati, the middle is my timezone, and the right is in UTC.

The date comment I placed is after this line:

https://github.com/jakubroztocil/rrule/blob/8d32705712f731124a0e1b516d73259e8eac5b20/src/iter/index.ts#L69

So far I've traced it to fromOrdinal(), where it seems to be returning the next date in the future. It looks like it is adding the +14 hours to 2000-01-01T09:00:00.000Z (14 + 9) and going to the next day.

ii.yearordinal in the Pacific/Kiritimati test is returning 10958, whereas the other two return 10957.

This value is calculated/cached here:

https://github.com/jakubroztocil/rrule/blob/8d32705712f731124a0e1b516d73259e8eac5b20/src/iterinfo/yearinfo.ts#L34

A naive mitigation I tried was the following:

  const offsetInHours = new Date().getTimezoneOffset() / 60;
  let yearordinal = toOrdinal(firstyday)

  if (offsetInHours <= -14) {
    yearordinal -= 1;
  }

But it there were 80 or so tests still broken.

dereekb commented 2 years ago

https://github.com/jakubroztocil/rrule/blob/8d32705712f731124a0e1b516d73259e8eac5b20/src/dateutil.ts#L89 https://github.com/jakubroztocil/rrule/blob/8d32705712f731124a0e1b516d73259e8eac5b20/src/dateutil.ts#L90

The issue is on these two lines here. Once you remove the unnecessary offset subtraction the one passed (I tested the UTC timezone and the Pacific/Kiritimati timezone locally).

Both of these dates are already in UTC, so when the timezone offset is added they end up offset with the current timezone.

Even if these dates are in another timezone, subtracting the current timezone's offset doesn't seem like it would be accurate either. I'm guessing that the offset was large enough that it counted for an extra day.

Making sure there aren't any other broken tests and will submit a PR.

davidgoli commented 9 months ago

The fix has been merged, please reopen if this recurs