Skip to content

Conversation

@zhiyuanliang-ms
Copy link
Member

Why this PR?

Added testcases to check whether the cache of time window filter cached the correct closest active recurring time window start.

#266 (comment)

Copy link

@ivywei0125 ivywei0125 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jimmyca15
Copy link
Member

The test doesn't appear to be validating what it intends to. Ensuring something is in the cache does not check whether the filter calculates the window again or not.

@zhiyuanliang-ms
Copy link
Member Author

@jimmyca15 In the latest commit, I added an internal property for the timewindow filter to track how many times recurring time window is re-calculated. What do you think?

@zhiyuanliang-ms
Copy link
Member Author

This PR goes after PR #452

@jimmyca15
Copy link
Member

@jimmyca15 In the latest commit, I added an internal property for the timewindow filter to track how many times recurring time window is re-calculated. What do you think?

@zhiyuanliang-ms too invasive. Can we pass a custom implementation of IMemoryCache and check whenever memorycache.Set is called?

@zhiyuanliang-ms
Copy link
Member Author

Can we pass a custom implementation of IMemoryCache and check whenever memorycache.Set is called?

@jimmyca15 Updated in 5da2d7d

@zhiyuanliang-ms
Copy link
Member Author

@rossgrambo Can you approve this PR?

mockedTimeProvider.UtcNow = mockedTimeProvider.UtcNow.AddHours(1);
Assert.True(await mockedTimeWindowFilter.EvaluateAsync(context));
}
mockedTimeProvider.UtcNow = DateTimeOffset.Parse("2024-2-2T23:00:00+08:00");
Copy link
Member

Choose a reason for hiding this comment

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

It might make these tests more readable if we switched this out for "StartTimeStamp.PlusDays(1)" or something similar. Not sure if it would lead to issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it easier to see the date directly, I used to open a calendar and check whether a date hits a recurrence. With"StartTimeStamp.PlusDays(1)", I need to do some extra calculation in my mind.

@zhiyuanliang-ms zhiyuanliang-ms merged commit 76969fa into main Dec 17, 2024
4 checks passed
@zhiyuanliang-ms zhiyuanliang-ms deleted the zhiyuanliang/update-timewindow-filter-cache-test branch November 9, 2025 12:24
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.

5 participants