-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
CachedConfigCollection is written to disk on every page load #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sensible - we're setting the hash any time we get or fetch the full config from the cache, and then on destruct we only set the cache in the event we've changed config and not already updated the cache.
Just one question, which is about tests - in the previous attempt at fixing this, there was an update to the tests - which does have some calls expecting the set()
method for the cache to be called some number of times.
It seems a little concerning that those tests are passing without change here - can you please just have a quick look to check if there's a way we can validate this change with unit testing?
@GuySartorelli yes that makes sense, it the current test, some changes are made, and therefore, it's still written twice to prove that what i did does something, a unit test should be added where no changes are made and no extra write happens. |
Sweet. I think that's the only thing holding this PR up now. Do you feel comfortable writing that test? |
@GuySartorelli ok, added the test. It's not very intuitive because any ->get calls update the callCache in the unit tests (so even if you don't actively ->set anything, you still get a write on destruct). It's only once the callCache is already warmed up that no more write will occur. |
@GuySartorelli any other wishes :-) ? |
This PR is in our peer review column, just waiting for someone on the team to free up to take a proper look. |
Co-authored-by: Steve Boyd <[email protected]>
Co-authored-by: Steve Boyd <[email protected]>
Co-authored-by: Steve Boyd <[email protected]>
Co-authored-by: Steve Boyd <[email protected]>
@emteknetnz ok i've split the test and renamed them in a more meaningful way |
Another attempt at fixing #34