Re-enable tests for Sys.RT.Caching#118508
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request re-enables previously disabled tests for the System.Runtime.Caching library by removing restrictive test annotations and improving test reliability. The changes focus on expanding test coverage across platforms and adding new functionality testing.
Key changes include:
- Removal of
[ActiveIssue],[PlatformSpecific], and[SkipOnPlatform]annotations from multiple tests - Addition of a new
[OuterLoop]test to verify cache expiration behavior with file dependencies - Refactoring of test helper methods to improve test isolation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| MemoryCacheTest.cs | Removes test skip annotations from 7 different test methods, enabling broader platform execution |
| HostFileChangeMonitorTest.cs | Removes test skip annotations, adds new cache expiration test, and refactors helper method for better test isolation |
| if (!mc.Contains("key")) | ||
| break; | ||
| Thread.Sleep(20); | ||
| } |
There was a problem hiding this comment.
When there are a lot of tests running in parallel (which xunit attempts to achieve), there can be a lot of thread contention and this might not return within 20ms. I would create a Stopwatch before the loop and use a while loop checking if the required amount of time has elapsed yet. This would be immune to CPU contention causing the test to take significantly longer due to the iteration count.
There was a problem hiding this comment.
I thought about that. Decided to just keep it simple though. The exact timing isn't important. The cache item should be removed relatively quickly - just not instantly, which is why we check on it in a loop. I'd be shocked if it ever took longer than 1s, but I wanted to be flexible enough so the test isn't noisy. 5s felt like an eternity for this, but in reality, the condition for breaking the loop should be met much, much sooner... and what that exact time is isn't important.
Do you think the potential for running so much longer than 5s that it becomes a problem in the test harness is a realistic problem?
There was a problem hiding this comment.
Ooh! Also, I switched from Thread.Sleep() to Task.Delay() and re-created the PR instead of mucking around with some re-basing/conflict resolution... and proceeded to use the old branch again. :/
I'm going to close this PR and assign the new one for the correct branch to you. It's all the same changes except with one more commit updating to Task.Delay().
This pull request updates the
System.Runtime.Cachingtest suite to improve coverage and maintainability. The main changes include removing obsolete or restrictive test annotations, refactoring test helpers for better isolation, and adding a new test to verify cache item expiration behavior when file dependencies change.Test coverage and reliability improvements:
[OuterLoop]testReasonable_DelayinHostFileChangeMonitorTest.csto verify that cache items expire promptly when a monitored file changes.SetupMonitoringhelper to accept a unique ID, ensuring test file isolation and reducing interference between tests. [1] [2]Test annotation and platform restriction cleanup:
[ActiveIssue],[PlatformSpecific], and[SkipOnPlatform]annotations from several tests in bothHostFileChangeMonitorTest.csandMemoryCacheTest.cs, enabling broader test execution across platforms and configurations. [1] [2] [3] [4] [5] [6] [7]General code and import maintenance:
HostFileChangeMonitorTest.csto remove unnecessary imports and improve code clarity.