Skip to content

Commit

Permalink
Improve recurrence handling, remove restrictions (#627)
Browse files Browse the repository at this point in the history 
* Modified Constants.cs:
- Deprecated `RecurrenceRestrictionType` and `RecurrenceEvaluationModeType` enums.

Updated RecurrencePattern.cs:
- Deprecated `RestrictionType` and `EvaluationMode` properties.
- Set default for `RestrictionType`.

Enhanced RecurrenceUtil:
- Added new using directives.
- Introduced `DefaultTimeout`.
- Updated `GetOccurrences` with timeout logic.

Refined RecurrencePatternEvaluator:
- Removed `_maxIncrementCount` with connected logic in favor of `RecurrenceUtil.DefaultTimeout`
- Improved XML documentation comments.

Updated Calendar.cs:
- Simplified `Load` method.
- Deprecated certain properties and methods associated with `RestrictionType` and `EvaluationMode`
- Removed obsolete `Evaluate` method.

Enhanced RecurrenceTests.cs with more robust test cases:
- Replaced `Assert.That` with `Assert.Multiple`.
- Changed assertion for occurrences count.
- Renamed test methods for clarity.
- Added timeout settings and updated expected exceptions.
- Adjusted test data and added comments.

Corrected typos and improved formatting for better readability.

Fixes #622

* Don't convert dt to  `SystsemLocal` in `Calendar.GetOccurrences(IDateTime dt)`

* Remove timeout handling and adjust related recurrence tests

- Removed `SetupBeforeEachTest` and timeout settings from tests.
- Re-introduced `_maxIncrementCount` in `RecurrencePatternEvaluator`.
- Modified `GetDates` to use `_maxIncrementCount` for loop control.
- Removed timeout handling from `RecurrenceUtil` class.

* Refactor test names and update assertion message

- Updated test method names to reflect checking for at least a defined number of occurrences.
- Removed redundant summary comments to test methods for clarity.
- Remove remaining mentions of "Timeout"

* Change tests to check for exact number of occurrences

* Secondly_DefinedNumberOfOccurrences_ShouldSucceed(), Minutely_DefinedNumberOfOccurrences_ShouldSucceed(), Hourly_DefinedNumberOfOccurrences_ShouldSucceed() assert exact number of occurences.

* Updated test methods to dynamically generate expected occurrences using loops

* Implemented changes from the code review
  • Loading branch information
@axunonb
axunonb authored Nov 4, 2024
1 parent 52b34e2 commit 533aeeb
Show file tree
Hide file tree
Showing 9 changed files with 206 additions and 279 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/coverage.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
- name: Build
run: dotnet build --no-restore --configuration Release -p:nowarn=1591
- name: Test
run: dotnet test --no-build --configuration Release --verbosity quiet /p:AltCover=true /p:AltCoverXmlReport="coverage.xml" /p:AltCoverStrongNameKey="../IcalNetStrongnameKey.snk" /p:AltCoverAssemblyExcludeFilter="Ical.Net.Tests|NUnit3.TestAdapter|AltCover" /p:AltCoverAttributeFilter="ExcludeFromCodeCoverage" /p:AltCoverLineCover="false"
run: dotnet test --no-build --configuration Release --verbosity normal /p:AltCover=true /p:AltCoverXmlReport="coverage.xml" /p:AltCoverStrongNameKey="../IcalNetStrongnameKey.snk" /p:AltCoverAssemblyExcludeFilter="Ical.Net.Tests|NUnit3.TestAdapter|AltCover" /p:AltCoverAttributeFilter="ExcludeFromCodeCoverage" /p:AltCoverLineCover="false"

- name: Store coverage report as artifact
uses: actions/upload-artifact@v4
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,4 @@ jobs:
- name: Build
run: dotnet build --no-restore --configuration Release -p:nowarn=1591
- name: Test
run: dotnet test --no-build --configuration Release --verbosity quiet
run: dotnet test --no-build --configuration Release --verbosity normal
208 changes: 71 additions & 137 deletions Ical.Net.Tests/RecurrenceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,33 +30,41 @@ int eventIndex
)
{
var evt = cal.Events.Skip(eventIndex).First();
var rule = evt.RecurrenceRules.FirstOrDefault();
#pragma warning disable 0618
if (rule != null) rule.RestrictionType = RecurrenceRestrictionType.NoRestriction;
#pragma warning restore 0618
fromDate.AssociatedObject = cal;
toDate.AssociatedObject = cal;

var occurrences = evt.GetOccurrences(fromDate, toDate)
.OrderBy(o => o.Period.StartTime)
.ToList();

Assert.That(
occurrences,
Has.Count.EqualTo(dateTimes.Length),
"There should be exactly " + dateTimes.Length + " occurrences; there were " + occurrences.Count);

if (evt.RecurrenceRules.Count > 0)
Assert.Multiple(() =>
{
Assert.That(evt.RecurrenceRules, Has.Count.EqualTo(1));
}
Assert.That(
occurrences,
Has.Count.EqualTo(dateTimes.Length),
"There should have been " + dateTimes.Length + " occurrences; there were " + occurrences.Count);

for (var i = 0; i < dateTimes.Length; i++)
{
// Associate each incoming date/time with the calendar.
dateTimes[i].AssociatedObject = cal;
if (evt.RecurrenceRules.Count > 0)
{
Assert.That(evt.RecurrenceRules, Has.Count.EqualTo(1));
}

for (var i = 0; i < dateTimes.Length; i++)
{
// Associate each incoming date/time with the calendar.
dateTimes[i].AssociatedObject = cal;

var dt = dateTimes[i];
Assert.That(occurrences[i].Period.StartTime, Is.EqualTo(dt), "Event should occur on " + dt);
if (timeZones != null)
Assert.That(dt.TimeZoneName, Is.EqualTo(timeZones[i]), "Event " + dt + " should occur in the " + timeZones[i] + " timezone");
}
var dt = dateTimes[i];
Assert.That(occurrences[i].Period.StartTime, Is.EqualTo(dt), "Event should occur on " + dt);
if (timeZones != null)
Assert.That(dt.TimeZoneName, Is.EqualTo(timeZones[i]),
"Event " + dt + " should occur in the " + timeZones[i] + " timezone");
}
});
}

private void EventOccurrenceTest(
Expand Down Expand Up @@ -1867,134 +1875,55 @@ public void Bug1741093()
);
}

/// <summary>
/// Ensures that, by default, SECONDLY recurrence rules are not allowed.
/// </summary>
[Test, Category("Recurrence")]
public void Secondly1()
{
Assert.That(() =>
{
var iCal = Calendar.Load(IcsFiles.Secondly1);
_ = iCal.GetOccurrences(new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid), new CalDateTime(2007, 7, 21, 8, 0, 0, _tzid));
}, Throws.Exception.TypeOf<ArgumentException>(), "Evaluation engine should have failed.");
}

/// <summary>
/// Ensures that the proper behavior occurs when the evaluation
/// mode is set to adjust automatically for SECONDLY evaluation
/// </summary>
[Test, Category("Recurrence")]
public void Secondly1_1()
public void Secondly_DefinedNumberOfOccurrences_ShouldSucceed()
{
var iCal = Calendar.Load(IcsFiles.Secondly1);
iCal.RecurrenceEvaluationMode = RecurrenceEvaluationModeType.AdjustAutomatically;

EventOccurrenceTest(
iCal,
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 10, 1, _tzid), // End period is exclusive, not inclusive.
new[]
{
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 1, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 2, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 3, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 4, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 5, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 6, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 7, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 8, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 9, 0, _tzid),
new CalDateTime(2007, 6, 21, 8, 10, 0, _tzid)
},
null
);
}
var start = new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid);
var end = new CalDateTime(2007, 6, 21, 8, 1, 0, _tzid); // End period is exclusive, not inclusive.

/// <summary>
/// Ensures that if configured, MINUTELY recurrence rules are not allowed.
/// </summary>
[Test, Category("Recurrence")]
public void Minutely1()
{
Assert.That(() =>
var dateTimes = new List<IDateTime>();
for (var dt = start; dt.LessThan(end); dt = (CalDateTime) dt.AddSeconds(1))
{
var iCal = Calendar.Load(IcsFiles.Minutely1);
iCal.RecurrenceRestriction = RecurrenceRestrictionType.RestrictMinutely;
var occurrences = iCal.GetOccurrences(
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 7, 21, 8, 0, 0, _tzid));
}, Throws.Exception.TypeOf<ArgumentException>());
dateTimes.Add(new CalDateTime(dt));
}

EventOccurrenceTest(iCal, start, end, dateTimes.ToArray(), null);
}

/// <summary>
/// Ensures that the proper behavior occurs when the evaluation
/// mode is set to adjust automatically for MINUTELY evaluation
/// </summary>
[Test, Category("Recurrence")]
public void Minutely1_1()
public void Minutely_DefinedNumberOfOccurrences_ShouldSucceed()
{
var iCal = Calendar.Load(IcsFiles.Minutely1);
iCal.RecurrenceRestriction = RecurrenceRestrictionType.RestrictMinutely;
iCal.RecurrenceEvaluationMode = RecurrenceEvaluationModeType.AdjustAutomatically;

EventOccurrenceTest(
iCal,
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 12, 0, 1, _tzid), // End period is exclusive, not inclusive.
new[]
{
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 9, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 10, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 11, 0, 0, _tzid),
new CalDateTime(2007, 6, 21, 12, 0, 0, _tzid)
},
null
);
}
var start = new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid);
var end = new CalDateTime(2007, 6, 21, 12, 0, 1, _tzid); // End period is exclusive, not inclusive.

/// <summary>
/// Ensures that if configured, HOURLY recurrence rules are not allowed.
/// </summary>
[Test, Category("Recurrence")/*, ExpectedException(typeof(EvaluationEngineException))*/]
public void Hourly1()
{
Assert.That(() =>
var dateTimes = new List<IDateTime>();
for (var dt = start; dt.LessThan(end); dt = (CalDateTime)dt.AddMinutes(1))
{
var iCal = Calendar.Load(IcsFiles.Hourly1);
iCal.RecurrenceRestriction = RecurrenceRestrictionType.RestrictHourly;
_ = iCal.GetOccurrences(new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid), new CalDateTime(2007, 7, 21, 8, 0, 0, _tzid));
dateTimes.Add(new CalDateTime(dt));
}

}, Throws.Exception.TypeOf<ArgumentException>());
EventOccurrenceTest(iCal, start, end, dateTimes.ToArray(), null);
}

/// <summary>
/// Ensures that the proper behavior occurs when the evaluation
/// mode is set to adjust automatically for HOURLY evaluation
/// </summary>
[Test, Category("Recurrence")]
public void Hourly1_1()
public void Hourly_DefinedNumberOfOccurrences_ShouldSucceed()
{
var iCal = Calendar.Load(IcsFiles.Hourly1);
iCal.RecurrenceRestriction = RecurrenceRestrictionType.RestrictHourly;
iCal.RecurrenceEvaluationMode = RecurrenceEvaluationModeType.AdjustAutomatically;

EventOccurrenceTest(
iCal,
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 25, 8, 0, 1, _tzid), // End period is exclusive, not inclusive.
new[]
{
new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 22, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 23, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 24, 8, 0, 0, _tzid),
new CalDateTime(2007, 6, 25, 8, 0, 0, _tzid)
},
null
);
var start = new CalDateTime(2007, 6, 21, 8, 0, 0, _tzid);
var end = new CalDateTime(2007, 6, 25, 8, 0, 1, _tzid); // End period is exclusive, not inclusive.

var dateTimes = new List<IDateTime>();
for (var dt = start; dt.LessThan(end); dt = (CalDateTime)dt.AddHours(1))
{
dateTimes.Add(new CalDateTime(dt));
}

EventOccurrenceTest(iCal, start, end, dateTimes.ToArray(), null);
}

/// <summary>
Expand Down Expand Up @@ -2038,7 +1967,7 @@ public void YearlyInterval1()
}

/// <summary>
/// Ensures that "off-day" calcuation works correctly
/// Ensures that "off-day" calculation works correctly
/// </summary>
[Test, Category("Recurrence")]
public void DailyInterval1()
Expand Down Expand Up @@ -2841,10 +2770,10 @@ public void Evaluate1(string freq, int secsPerInterval, bool hasTime)

// This case (DTSTART of type DATE and FREQ=MINUTELY) is undefined in RFC 5545.
// ical.net handles the case by pretending DTSTART has the time set to midnight.
evt.RecurrenceRules.Add(new RecurrencePattern($"FREQ={freq};INTERVAL=10;COUNT=5")
{
RestrictionType = RecurrenceRestrictionType.NoRestriction,
});
evt.RecurrenceRules.Add(new RecurrencePattern($"FREQ={freq};INTERVAL=10;COUNT=5"));
#pragma warning disable 0618
evt.RecurrenceRules[0].RestrictionType = RecurrenceRestrictionType.NoRestriction;
#pragma warning restore 0618

var occurrences = evt.GetOccurrences(CalDateTime.Today.AddDays(-1), CalDateTime.Today.AddDays(100))
.OrderBy(x => x)
Expand All @@ -2868,8 +2797,10 @@ public void RecurrencePattern1()
{
// NOTE: evaluators are not generally meant to be used directly like this.
// However, this does make a good test to ensure they behave as they should.
RecurrencePattern pattern = new RecurrencePattern("FREQ=SECONDLY;INTERVAL=10");
var pattern = new RecurrencePattern("FREQ=SECONDLY;INTERVAL=10");
#pragma warning disable 0618
pattern.RestrictionType = RecurrenceRestrictionType.NoRestriction;
#pragma warning restore 0618

var us = new CultureInfo("en-US");

Expand Down Expand Up @@ -3177,10 +3108,14 @@ public void OccurrenceMustBeCompletelyContainedWithinSearchRange()
Assert.That(occurrences.Last().StartTime.Equals(lastExpected), Is.True);
}

[Test, Ignore("Turn on in v3")]
/// <summary>
/// Evaluate relevancy and validity of the request.
/// Find a solution for issue #120 or close forever
/// </summary>
[Test, Ignore("Turn on in v3", Until = "2024-12-31")]
public void EventsWithShareUidsShouldGenerateASingleRecurrenceSet()
{
//https://github.com/rianjs/ical.net/issues/120
//https://github.com/rianjs/ical.net/issues/120 dated Sep 5, 2016
const string ical =
@"BEGIN:VCALENDAR
PRODID:-//Google Inc//Google Calendar 70.9054//EN
Expand Down Expand Up @@ -3757,7 +3692,9 @@ private static IEnumerable<RecurrenceTestCase> TestLibicalTestCasesSource

[TestCaseSource(nameof(TestLibicalTestCasesSource))]
public void TestLibicalTestCases(RecurrenceTestCase testCase)
=> ExecuteRecurrenceTestCase(testCase);
{
ExecuteRecurrenceTestCase(testCase);
}

private static IEnumerable<RecurrenceTestCase> TestFileBasedRecurrenceTestCaseSource
=> ParseTestCaseFile(IcsFiles.RecurrrenceTestCases);
Expand All @@ -3775,10 +3712,7 @@ public void ExecuteRecurrenceTestCase(RecurrenceTestCase testCase)

// Start at midnight, UTC time
evt.Start = testCase.DtStart;
evt.RecurrenceRules.Add(new RecurrencePattern(testCase.RRule)
{
RestrictionType = RecurrenceRestrictionType.NoRestriction,
});
evt.RecurrenceRules.Add(new RecurrencePattern(testCase.RRule));

var occurrences = evt.GetOccurrences(testCase.StartAt?.Value ?? DateTime.MinValue, DateTime.MaxValue)
.OrderBy(x => x)
Expand Down
Loading

0 comments on commit 533aeeb

Please sign in to comment.