Skip to content

Conversation

@minichma
Copy link
Collaborator

@minichma minichma commented Oct 19, 2024

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.

@minichma
Copy link
Collaborator Author

@axunonb what do you think?

@axunonb
Copy link
Collaborator

axunonb commented Oct 20, 2024

If it's possible to use existing wheels, reinventing is a waste of time.
So yes, let's move on.

Licensing

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.
This typically means that any modifications to the LGPL-licensed code must be made available under the same LGPL license, and we must provide a way to relink the application with a modified version of the LGPL library.

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".
This will reduce the license complexity close to zero. The files included in this PR are sufficient re licensing.

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.

@minichma
Copy link
Collaborator Author

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?

@minichma minichma force-pushed the xp/test_libical_test_recur branch from e4fce9f to 7cf1a19 Compare October 20, 2024 19:21
@minichma
Copy link
Collaborator Author

@winterz We consider reusing test definitions from the libical project in Ical.Net by copying the icalrecur_test.out file. :-) Do you think license comments introduced in this PR are sufficient? Do you have any thoughts on compatibility between libical's LGPL/MPL and this project's MIT license?

@winterz
Copy link

winterz commented Oct 21, 2024

@winterz We consider reusing test definitions from the libical project in Ical.Net by copying the icalrecur_test.out file. :-) Do you think license comments introduced in this PR are sufficient? Do you have any thoughts on compatibility between libical's LGPL/MPL and this project's MIT license?

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".

@minichma
Copy link
Collaborator Author

@winterz Thank you!

@minichma minichma force-pushed the xp/test_libical_test_recur branch from 7cf1a19 to a0fb39e Compare October 24, 2024 09:48
@minichma minichma changed the title Experimental: Run recurrence tests from the libical project Test: Run recurrence test cases from the libical project and introduce generic file-based test cases. Oct 24, 2024
@minichma minichma force-pushed the xp/test_libical_test_recur branch 2 times, most recently from 3e49328 to 8ed141d Compare October 24, 2024 09:54
@minichma minichma marked this pull request as ready for review October 24, 2024 09:54
@minichma minichma requested a review from axunonb October 24, 2024 09:54
@minichma minichma force-pushed the xp/test_libical_test_recur branch from 8ed141d to b5602d4 Compare October 24, 2024 09:56
@minichma
Copy link
Collaborator Author

@axunonb Please have a closer look especially at the licensing text.

@minichma minichma force-pushed the xp/test_libical_test_recur branch from b5602d4 to 69c6beb Compare October 24, 2024 13:53
axunonb
axunonb previously approved these changes Oct 25, 2024
Copy link
Collaborator

@axunonb axunonb left a 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 block
// 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

@sonarqubecloud
Copy link

@minichma minichma requested a review from axunonb October 25, 2024 18:33
@minichma
Copy link
Collaborator Author

@axunonb Thanks for the review. Please approve once again!

A test with false as arg would be good, if applicable. Must not necessarily go into this PR

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 FREQ=[HOURLY|MINUTELY|SECONDLY];BYYEARDAY=..., because BYYEARDAY is limiting only in the case of these frequencies.

Copy link
Collaborator

@axunonb axunonb left a 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.

@minichma
Copy link
Collaborator Author

Thanks for the kind words. Its really you who deserves the kudus. Great to see, you're taking care of this important project.

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.

3 participants