-
Notifications
You must be signed in to change notification settings - Fork 247
Test: Run recurrence test cases from the libical project and introduce generic file-based test cases. #617
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
Conversation
|
@axunonb what do you think? |
|
If it's possible to use existing wheels, reinventing is a waste of time.
Using LGPL-licensed Code in MIT Projects: brings - in general - some complexity. If we include LGPL-licensed code in ICal.Net that is licensed under the MIT license, we must comply with the LGPL's requirements. At the same time we'd like to use what they're publishing "as is", and also only for the test project. We can add a note to contributors like "Do not modify code in the contrib folder". Just one more thing: Tt would be a matter of courtesy to inform libical project owners that we are using parts of their code for testing, and ask for their formal okay. |
|
The project is published under a dual license. We can choose between the LGPL and the MPL. Maybe the MPL is more suitable for us. What do you think? |
e4fce9f to
7cf1a19
Compare
|
@winterz We consider reusing test definitions from the libical project in Ical.Net by copying the |
Yes, you can add icalrecur_test.out to ical.net project. mixing MPL with MIT is ok. you need to mention somewhere that this file is MPL licensed and copyrighted by "the libical developers". |
|
@winterz Thank you! |
7cf1a19 to
a0fb39e
Compare
3e49328 to
8ed141d
Compare
8ed141d to
b5602d4
Compare
|
@axunonb Please have a closer look especially at the licensing text. |
b5602d4 to
69c6beb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that this
| dates = GetYearDayVariants(dates, pattern, expandBehaviors[2]); |
is the only reference to
| private List<DateTime> GetYearDayVariants(List<DateTime> dates, RecurrencePattern pattern, bool? expand) |
and the
bool? expand arg is always true in unit tests meaning this blockical.net/Ical.Net/Evaluation/RecurrencePatternEvaluator.cs
Lines 493 to 514 in 2146ea0
| // Limit behavior | |
| for (var i = dates.Count - 1; i >= 0; i--) | |
| { | |
| var date = dates[i]; | |
| for (var j = 0; j < pattern.ByYearDay.Count; j++) | |
| { | |
| var yearDay = pattern.ByYearDay[j]; | |
| var newDate = yearDay > 0 | |
| ? date.AddDays(-date.DayOfYear + yearDay) | |
| : date.AddDays(-date.DayOfYear + 1).AddYears(1).AddDays(yearDay); | |
| if (newDate.Date == date.Date) | |
| { | |
| goto Next; | |
| } | |
| } | |
| dates.RemoveAt(i); | |
| Next: | |
| ; | |
| } |
never gets called. A test with
false as arg would be good, if applicable.Must not necessarily go into this PR
…project. 14/70 failing.
69c6beb to
b975b2b
Compare
|
|
@axunonb Thanks for the review. Please approve once again!
Good finding! Agree that this PR should just take care about adding the infrastructure and the existing libical test set, not go deep with additional tests. Coincident (more or less): Exactly this test case will probably be added to the libical test cases too, see libical/libical#791. I'm currently also doing some reviews and testing there, which leads to a number of additional test cases. Will try to add them here as well over time. Unfortunately my time is quite limited (investing way more than I should already), so I don't have an ETA yet. The test case would probably have to be of the form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot the time and dedication. It's a lot of work and amazing what you're doing!
Regarding ETA: we can't catch up 3 years in 2 weeks time - I'm ware.
|
Thanks for the kind words. Its really you who deserves the kudus. Great to see, you're taking care of this important project. |



This PR is for discussion, not to be merged (at least for now).
The libical project maintains a list of test cases for their recurrence evaluation code. The PR adds a copy of the test case definitions and runs them on Ical.Net. 8/70 test cases fail, but I didn't have the time to dig deeper for now. However, the failing tests are probably worth an investigation.
Licensing: If it turns out that the test cases make sense and we think this should be merged, we'd have to check the licensing. The file used from the libical project is available under a dual license, either the MPL v2.0 or the LGPL v2.1. I assume the MPL v2 should be compatible with this project, but at this time that's just a guess.