Skip to content
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

Add tests for recurrence generators #722

Merged
merged 2 commits into from
Feb 13, 2024
Merged

Add tests for recurrence generators #722

merged 2 commits into from
Feb 13, 2024

Conversation

ish-bindra
Copy link
Contributor

The following limitations to using lunartick for ical rules were discovered:

  1. It does not support interval with rules that are monthly and have an interval > 1 (e.g. FREQ=MONTHLY;BYMONTHDAY=17;INTERVAL=2)
  2. It does not support fortnightly rules (e.g. FREQ=WEEKLY;BYDAY=TH;INTERVAL=2)
  3. It does not support BYMONTHDAY=-1 (https://github.com/ordermentum/lunartick/blob/develop/src/iterator.js#L110 shifts the date by an extra month)
  4. It does not support multiple values for BYDAY (e.g. FREQ=WEEKLY;BYDAY=MO,WE,FR)
  5. It does not support BYSETPOS parameter (e.g. FREQ=DAILY;BYHOUR=8,18;BYMINUTE=30,0;BYSETPOS=1,4)
  6. It does not chronologically sort BY rules (e.g. Values will be different for FREQ=DAILY;BYHOUR=8,18;BYMINUTE=0,30 and FREQ=DAILY;BYHOUR=8,18;BYMINUTE=30,0 - Note the BYMINUTE prop)
  7. It does not support -ve BYDAY values (e.g. FREQ=MONTHLY;BYDAY=-2FR)

This PR adds some verification tests related to recurrence generator libraries

  • Rules without DTSTART
  • Rules with DTSTART
  • DST thresholds
  • Verify above limitations

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

changeset-bot bot commented Feb 13, 2024

⚠️ No Changeset found

Latest commit: f7bc48a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -43,7 +43,8 @@
"rrule-rust": "^1.2.0",
"sequelize": "^6.6.2",
"typed-emitter": "^1.3.1",
"uuid": "^8.3.2"
"uuid": "^8.3.2",
"lunartick-deprecated": "npm:@ordermentum/[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

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

dev dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is interim while we transition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to dev dep.
Yeah, can remove once the solution is prod tested.

@sugendran
Copy link
Contributor

Looks reasonable to me. When you clean up lunartick, what's the leave behind unit tests that confirm rrule is set up and working as expected?

@ish-bindra
Copy link
Contributor Author

Looks reasonable to me. When you clean up lunartick, what's the leave behind unit tests that confirm rrule is set up and working as expected?

That would be the helpers test file https://github.com/ordermentum/steveo/blob/main/packages/scheduler-sequelize/test/helpers_test.ts#L21

@ish-bindra ish-bindra merged commit 4882616 into main Feb 13, 2024
4 checks passed
@ish-bindra ish-bindra deleted the rrule-lunartick branch February 13, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants