fix: flaky cache test #19140
fix: flaky cache test #19140
Conversation
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Two small nits, but generally looks good to me!
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub fn new_with_provider( |
There was a problem hiding this comment.
Does this need to be pub? It doesn't really matter since it won't be available in other crates anyway, just a nit.
There was a problem hiding this comment.
Could we also have fn with_time_provider(mut self, provider: Arc<dyn TimeProvider>) -> Self instead?
There was a problem hiding this comment.
Thanks for the review!
I agree it doesn't need to be fully public. I will update it to pub(crate).
My concern with fn with_time_provider() is that it technically creates a live instance using the SystemTimeProvider first, and then mutates it. I prefer enforcing at the type/constructor level that this specific test instance is instantiated with the Mock immediately. This guarantees the object is never in a state where it interacts with the real system clock, avoiding any potential side effects during setup.
There was a problem hiding this comment.
I understand the point, either seems fine to me, I’d opt for the less verbose / repetitive version here, I don’t think the interaction with the system clock is problematic.
There was a problem hiding this comment.
Fair point! I agree that reducing code duplication is better here. I will change it to use the with_time_provider as suggested.
|
FYI @BlakeOrth |
|
@adriangb all suggested changes are made . Thanks |
There was a problem hiding this comment.
@xonx4l what's going on here? This seems unrelated to this PR. Is there a rebase / merge issue?
There was a problem hiding this comment.
I think you should rebase on main and take mains version of this file to resolve conflicts
There was a problem hiding this comment.
I had some local environment issues where my formatter accidentally modified const_evaluator.rs. So i pushed a fix that reverts those changes and restores the file to match main
BlakeOrth
left a comment
BlakeOrth
left a comment
There was a problem hiding this comment.
Thank you for fixing up this flaky test! I just have one small piece of feedback related to the doc comments, but it's more of a nit-pick.
| /// Adds a new key-value pair to cache, meaning LRU entries might be evicted if required. | ||
| /// If the key is already in the cache, the previous entry is returned. | ||
| /// If the size of the entry is greater than the `memory_limit`, the value is not inserted. | ||
| /// Adds a new key-value pair to cache. | ||
| /// Takes `now` explicitly to determine expiration. |
There was a problem hiding this comment.
It looks like a decent amount of the doc comment updates here have lost some communication on the expected output of the method they're documenting. Unless I'm missing something, taking now as a parameter hasn't functionally changed the behavior of this method. I think including the purpose of now is a good addition, but I'd expect these doc comments to better communicate the purpose of now.
Perhaps they could read like the following:
/// Adds a new key-value pair to cache, meaning LRU entries might be evicted if required.
/// If the key is already in the cache, the previous entry is returned.
/// If the size of the entry is greater than the `memory_limit`, the value is not inserted.
/// The `now` parameter is used to determine the entry's expiration.
There was a problem hiding this comment.
Yeah the suggested comment looks good to me . Will update that. Thanks
4 participants
Which issue does this PR close?
cache::list_files_cache::tests::test_cache_with_ttl_and_lru( seems flaky) #19114.Rationale for this change
The test test_cache_with_ttl_and_lru was flaky and failing intermittently in CI. It relied on std::thread::sleep and Instant::now(), which caused race conditions when the test environment was slow or under load. This PR makes the test correct by removing reliance on the system clock and thread sleeping.
What changes are included in this PR?
-> Introduced a TimeProvider trait to abstract time retrieval.
-> Refactored DefaultListFilesCache to use provider.
-> Added DefaultListFilesCache::new_with_provider for testing purposes.
-> Updated test_cache_with_ttl_and_lru to use a MockTimeProvider.
Are these changes tested?
Yes. I verified the fix by running the specific test locally without failure.
Are there any user-facing changes?
No.