Skip to content

Small fix for recurring time window testcase #522

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.

Sign up for GitHub

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

Merged
zhiyuanliang-ms merged 8 commits into from
Jan 22, 2025
Merged

Small fix for recurring time window testcase #522

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5519811
add comments & correct testcase
zhiyuanliang-ms Dec 9, 2024
93c9800
update
zhiyuanliang-ms Dec 9, 2024
bd9e937
update
zhiyuanliang-ms Dec 9, 2024
c477006
update
zhiyuanliang-ms Dec 9, 2024
6ed724c
update
zhiyuanliang-ms Dec 10, 2024
5a2f87f
update
zhiyuanliang-ms Dec 17, 2024
26f0c78
Merge branch 'main' of https://github.com/microsoft/FeatureManagement…
zhiyuanliang-ms Dec 17, 2024
c49f145
Merge branch 'main' into zhiyuanliang/fix-recurring-timewindow
zhiyuanliang-ms Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ private static int CalculateWeeklyDayOffset(DayOfWeek day1, DayOfWeek day2)
/// </summary>
private static List<DayOfWeek> SortDaysOfWeek(IEnumerable<DayOfWeek> daysOfWeek, DayOfWeek firstDayOfWeek)
{
List<DayOfWeek> result = daysOfWeek.ToList();
List<DayOfWeek> result = daysOfWeek.Distinct().ToList(); // dedup

result.Sort((x, y) =>
CalculateWeeklyDayOffset(x, firstDayOfWeek)
Expand Down
33 changes: 11 additions & 22 deletions src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,49 +341,38 @@ private static bool TryValidateNumberOfOccurrences(TimeWindowFilterSettings sett
private static bool IsDurationCompliantWithDaysOfWeek(TimeSpan duration, int interval, IEnumerable<DayOfWeek> daysOfWeek, DayOfWeek firstDayOfWeek)
{
Debug.Assert(interval > 0);
Debug.Assert(daysOfWeek.Count() > 0);

if (daysOfWeek.Count() == 1)
{
return true;
}

DateTime firstDayOfThisWeek = DateTime.Today.AddDays(
DaysPerWeek - CalculateWeeklyDayOffset(DateTime.Today.DayOfWeek, firstDayOfWeek));

List<DayOfWeek> sortedDaysOfWeek = SortDaysOfWeek(daysOfWeek, firstDayOfWeek);

DateTime prev = DateTime.MinValue;
DayOfWeek firstDay = sortedDaysOfWeek.First(); // the closest occurrence day to the first day of week

DayOfWeek prev = firstDay;

TimeSpan minGap = TimeSpan.FromDays(DaysPerWeek);

foreach (DayOfWeek dayOfWeek in sortedDaysOfWeek)
foreach (DayOfWeek dayOfWeek in sortedDaysOfWeek.Skip(1))

@rossgrambo rossgrambo Dec 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like this Skip syntax- it seems less readable, could you add a comment why we skip the first?

I'd prefer we don't do a skip and explicitly check if prev == firstDay or if prev != null instead.

@zhiyuanliang-ms zhiyuanliang-ms Dec 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about this? Using index to skip the first day, instead of .Skip(1)

  List<DayOfWeek> sortedDaysOfWeek = SortDaysOfWeek(daysOfWeek, firstDayOfWeek);

  DayOfWeek firstDay = sortedDaysOfWeek.First(); // the closest occurrence day to the first day of week

  DayOfWeek prev = firstDay;

  TimeSpan minGap = TimeSpan.FromDays(DaysPerWeek);

  for (int i = 1; i < sortedDaysOfWeek.Count(); i++)
  {
      DayOfWeek dayOfWeek = sortedDaysOfWeek[i];

      TimeSpan gap = TimeSpan.FromDays(CalculateWeeklyDayOffset(dayOfWeek, prev));

      if (gap < minGap)
      {
          minGap = gap;
      }

      prev = dayOfWeek;
  }

{
DateTime date = firstDayOfThisWeek.AddDays(
CalculateWeeklyDayOffset(dayOfWeek, firstDayOfWeek));
TimeSpan gap = TimeSpan.FromDays(CalculateWeeklyDayOffset(dayOfWeek, prev));

if (prev != DateTime.MinValue)
if (gap < minGap)
{
TimeSpan gap = date - prev;

if (gap < minGap)
{
minGap = gap;
}
minGap = gap;
}

prev = date;
prev = dayOfWeek;
}

//
// It may across weeks. Check the next week if the interval is one week.
if (interval == 1)
{
DateTime firstDayOfNextWeek = firstDayOfThisWeek.AddDays(DaysPerWeek);

DateTime firstOccurrenceInNextWeek = firstDayOfNextWeek.AddDays(
CalculateWeeklyDayOffset(sortedDaysOfWeek.First(), firstDayOfWeek));

TimeSpan gap = firstOccurrenceInNextWeek - prev;
TimeSpan gap = TimeSpan.FromDays(CalculateWeeklyDayOffset(firstDay, prev));

if (gap < minGap)
{
Expand Down Expand Up @@ -413,7 +402,7 @@ private static int CalculateWeeklyDayOffset(DayOfWeek day1, DayOfWeek day2)
/// </summary>
private static List<DayOfWeek> SortDaysOfWeek(IEnumerable<DayOfWeek> daysOfWeek, DayOfWeek firstDayOfWeek)
{
List<DayOfWeek> result = daysOfWeek.ToList();
List<DayOfWeek> result = daysOfWeek.Distinct().ToList(); // dedup

@rossgrambo rossgrambo Dec 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we take a Set instead so we don't need to Distinct it?

@zhiyuanliang-ms zhiyuanliang-ms Dec 17, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I personally like the Distinct one, it is more readable. If we use a set here, we will either need to write two lines of code (1 line for new set and 1 line for ToList) or we will write List<DayOfWeek> result = new HashSet<DayOfWeek>(daysOfWeek).ToList(); I feel this single line ugly. I believe the background implementation of Distinct is based on hash set. Do you have any other concern about using Distinct


result.Sort((x, y) =>
CalculateWeeklyDayOffset(x, firstDayOfWeek)
Expand Down
49 changes: 46 additions & 3 deletions tests/Tests.FeatureManagement/RecurrenceEvaluation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ public void InvalidTimeWindowAcrossWeeksTest()
{
Type = RecurrencePatternType.Weekly,
Interval = 1,
FirstDayOfWeek = DayOfWeek.Sunday,
DaysOfWeek = new List<DayOfWeek>() { DayOfWeek.Tuesday, DayOfWeek.Saturday } // The time window duration should be shorter than 3 days because the gap between Saturday in the previous week and Tuesday in this week is 3 days.
},
Range = new RecurrenceRange()
Expand All @@ -299,7 +300,7 @@ public void InvalidTimeWindowAcrossWeeksTest()

//
// The settings is valid. No exception should be thrown.
RecurrenceEvaluator.IsMatch(DateTimeOffset.UtcNow, settings);
Assert.True(RecurrenceValidator.TryValidateSettings(settings, out string paramName, out string errorMessage));

settings = new TimeWindowFilterSettings()
{
Expand All @@ -320,7 +321,49 @@ public void InvalidTimeWindowAcrossWeeksTest()

//
// The settings is valid. No exception should be thrown.
RecurrenceEvaluator.IsMatch(DateTimeOffset.UtcNow, settings);
Assert.True(RecurrenceValidator.TryValidateSettings(settings, out paramName, out errorMessage));

settings = new TimeWindowFilterSettings()
{
Start = DateTimeOffset.Parse("2024-1-15T00:00:00+08:00"), // Monday
End = DateTimeOffset.Parse("2024-1-17T00:00:00+08:00"), // Time window duration is 2 days.
Recurrence = new Recurrence()
{
Pattern = new RecurrencePattern()
{
Type = RecurrencePatternType.Weekly,
Interval = 1,
FirstDayOfWeek = DayOfWeek.Sunday,
DaysOfWeek = new List<DayOfWeek>() { DayOfWeek.Monday, DayOfWeek.Saturday }
},
Range = new RecurrenceRange()
}
};

//
// The settings is valid. No exception should be thrown.
Assert.True(RecurrenceValidator.TryValidateSettings(settings, out paramName, out errorMessage));

settings = new TimeWindowFilterSettings()
{
Start = DateTimeOffset.Parse("2024-1-15T00:00:00+08:00"), // Monday
End = DateTimeOffset.Parse("2024-1-17T00:00:01+08:00"), // Time window duration is more than 2 days.
Recurrence = new Recurrence()
{
Pattern = new RecurrencePattern()
{
Type = RecurrencePatternType.Weekly,
Interval = 1,
FirstDayOfWeek = DayOfWeek.Sunday,
DaysOfWeek = new List<DayOfWeek>() { DayOfWeek.Monday, DayOfWeek.Saturday }
},
Range = new RecurrenceRange()
}
};

Assert.False(RecurrenceValidator.TryValidateSettings(settings, out paramName, out errorMessage));
Assert.Equal(ParamName.End, paramName);
Assert.Equal(ErrorMessage.TimeWindowDurationOutOfRange, errorMessage);

settings = new TimeWindowFilterSettings()
{
Expand All @@ -339,7 +382,7 @@ public void InvalidTimeWindowAcrossWeeksTest()
}
};

Assert.False(RecurrenceValidator.TryValidateSettings(settings, out string paramName, out string errorMessage));
Assert.False(RecurrenceValidator.TryValidateSettings(settings, out paramName, out errorMessage));
Assert.Equal(ParamName.End, paramName);
Assert.Equal(ErrorMessage.TimeWindowDurationOutOfRange, errorMessage);
}
Expand Down