Skip to content
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

Fixed file store to handle delete_matched being called before cache dir is created. #3174

Merged
merged 2 commits into from Oct 2, 2011

Conversation

phuibonhoa
Copy link
Contributor

Added fix so that file store does not raise an exception when cache dir does not exist yet. This can happen if a delete_matched is called before anything is saved in the cache.

…ir does not exist yet. This can happen if a delete_matched is called before anything is saved in the cache.
@josevalim
Copy link
Contributor

Thanks for the pull request. Could we improve the test here? The current test is calling a private method but i believe we should be testing the real behavior that causes the failure. For example, the test should probably 1) create a cache store 2) cache something in it 3) call delete_matched to remove the directory 4) try to lookup for something and ensure an exception won't be raised. Otherwise, someone can refactor the code later, realize that the coupled test to the method does not makes sense anymore while the bug still exists.

@phuibonhoa
Copy link
Contributor Author

Thanks for the input, you're right that wasn't the best testing approach. I've updated it to test the case where the exception normally is raised (delete_matched is called before anything is ever stored in the cache, so the containing directories are not yet created).

josevalim added a commit that referenced this pull request Oct 2, 2011
Fixed file store to handle delete_matched being called before cache dir is created.
@josevalim josevalim merged commit 1efe41d into rails:master Oct 2, 2011
@josevalim josevalim merged commit 119a484 into rails:master Oct 2, 2011
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.

None yet

2 participants