regression in 2.7.0 - Githubissues

dereekb commented 2 years ago

I tried updating to 2.7.0 directly it caused some of my tests to fail, so some unintended behaviors may have changed between the two versions.

I can't dig too deep into the issue right now, but wanted to raise it just to let you know there is a difference, and I didn't see anything posted about intentional differences. Here's the failure on my CI and the test it was running:

failed tests: https://app.circleci.com/pipelines/github/dereekb/dbx-components/797/workflows/5a33c0bd-ee90-48ca-9cac-fa2a06a66d21/jobs/1917/tests#failed-test-1

test run: https://app.circleci.com/pipelines/github/dereekb/dbx-components/797/workflows/5a33c0bd-ee90-48ca-9cac-fa2a06a66d21/jobs/1917/parallel-runs/0/steps/0-106

  ● DateRRuleUtility › DateRRuleInstance › expand() › timezone shifting › (Denver to Los Angeles Time) › mo,we,th at 11AM-12PM (1PM-2PM CST) 3 times › with range › it should return only the first date if start and end date equals first date

    expect(received).toBe(expected) // Object.is equality

    Expected: 1
    Received: 0

      55 |
      56 |                 expect(results.between).toBeDefined();
    > 57 |                 expect(results.dates.length).toBe(expectedResults.length);
         |                                              ^
      58 |                 expect(results.dates[0].startsAt).toBeSameSecondAs(expectedExpandResults[0]);
      59 |               });
      60 |             });

Here's the tests being run. https://github.com/dereekb/dbx-components/blob/4b0699c411f04d08982947f981993a87edbe2b46/packages/date/src/lib/rrule/date.rrule.spec.ts#L58

I run these tests in several different timezones too. The first test that failed was using TZ=America/New_York, but I imagine it is failing for the other timezones as well.

It looks like it is not returning a date that was being returned previously. Another test is failing with some expected dates being an hour off, another one 4 days off.

I'd check that the rrule tests are capturing that test case in particular and see if you're getting your expected output.

Reporting an issue

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

[x] Verify that you've looked through existing issues for duplicates before creating a new one
[x] Code sample reproducing the issue. Be sure to include all input values you are using such as the exact RRule string and dates.
[x] Expected output
[x] Actual output
[x] The version of rrule you are using
[x] Your operating system
[x] Your local timezone (run $ date from the command line of the machine showing the bug)

rrule 2.7.0 macOS Monterey 12.2 Date: Thu Jun 9 18:06:30 CDT 2022

davidgoli commented 2 years ago

Hi @dereekb , thanks for letting us know, and thanks for the test case; we'll look into this.

davidgoli commented 2 years ago

@dereekb Can you please work up a minimal repro using only the rrule library? I will have limited time to do this myself in the next couple of weeks, and hope to troubleshoot sooner.

dereekb commented 2 years ago

I will, but probably not in the immediate. Not using the date modules that extensively right now either (and not looking forwards to debugging timezones again), but should be in a few weeks.

Mainly just wanted to ping that there was a regression with the returned results with just a version bump, and make a thread incase anyone else came across the behavior. A majority of my tests still pass, so it could just be an edgecase.

Edit:

Actually, this may be a me problem. The tests of mine pass when using TZ=America/Denver, and all the other timezones shift themselves off.

The dates coming out from the following test I tried in RRule looked correct and behaved the same between 2.6.8 and 2.7.0.

  it('parses a DSTART and interval', () => {
    const tzid = `America/Chicago`; // my current timezone
    const rules = `DTSTART;TZID=${tzid}:20181101T190000\nRRULE:FREQ=WEEKLY;BYDAY=MO,WE,TH;INTERVAL=1;COUNT=3\n`;

    const output = parseInput(rules, {});

    expect(output).to.deep.include({
      dtstart: new Date(Date.UTC(2018, 10, 1, 19, 0, 0)),
      tzid
    });

    const rruleset = rrulestr(rules) as RRuleSet;

    const dates = rruleset.all();

    const firstBaseDate = new Date(Date.UTC(2018, 10, 1, 19, 0, 0)); // what the parser will return from
    const expectedExpandResults = [firstBaseDate];

    console.log('Date: ', dates[0].toISOString(), firstBaseDate, firstBaseDate.toISOString(), expectedExpandResults);

    expect(dates[0].getTime()).to.equal(expectedExpandResults[0].getTime());
  });

2.7.0

Date:  2018-11-01T19:00:00.000Z 2018-11-01T19:00:00.000Z 2018-11-01T19:00:00.000Z [ 2018-11-01T19:00:00.000Z ]
    ✔ parses a DSTART and interval

2.6.8

Date:  2018-11-01T19:00:00.000Z 2018-11-01T19:00:00.000Z 2018-11-01T19:00:00.000Z [ 2018-11-01T19:00:00.000Z ]
    ✓ parses a DSTART and interval

dereekb commented 2 years ago

Ok, yea it was a me issue related to using another system for handling timezones.

I didn't have Luxon installed prior, so the tzid input that was previously being ignored (but was required because NOT passing it caused an error or different behavior when the parser saw a timezone or something like that) in 2.6.8 was now converting timezones.

It was technically a regression for those who passed a tzid while not having Luxon installed, but that was probably never supported behavior. Reason why is explained more in #441. I'll also be closing #492 since it was the extra workaround but is no longer needed.

All good now!

jkbrzt / rrule

regression in 2.7.0 #521

Reporting an issue