Issue 338

[scottwb]

I believe this should re-close GH issue #338. To summarize:

• The previous attempt at fixing this didn't work. It always silently failed on an incorrect relative path to the cache_id dir.
• The test for that passed because the new delete_cache_id method was stubbed and never actually tested.
• Fixing that broke anything having to do with a multi-version uploader, because it used FileUtils.rm_rf to delete the entire cache dir, deleting other versions before they were stored.
• That was fixed by using Dir.rmdir which fails if the directory is not empty.
• @jnicklas previously brought up the danger of deleting directories when the user could possibly override the cache_dir mechanism. I believe these concerns are addressed because:
  • The cache_dir is never deleted. Only the subdirs underneath it, that are created by CarrierWave, are.
  • Only empty directories are deleted now (with specs to confirm this)

If this gets pulled into master and released with the next gem version, I'd be happy to update the notes in this wiki to reflect the differences in the new version: https://github.com/jnicklas/carrierwave/wiki/How-to:-Delete-cache-garbage-directories

[clyfe]

Thanks for the fixes and sorry for leaving warts behind me.

I'm new to testing, and my original issue was stubbing methods on globals like Dir, File.
As far as I can tell your test actually creates real files .
Is that considered ok? Where do they get created in test environment? Shouldn't these methods be stubbed?

[scottwb]

@clyfe - The real files were being created during tests before I ever touched them - in spec/public/uploads/tmp. Note that the store_spec.rb cleans up after itself with this:

  after do
    FileUtils.rm_rf(public_path)
  end

I actually prefer it this way. The test code in question is spec'ing the behavior surrounding these files and directories, so in my opinion, it really should be exercising what you expect it to do in the real world as close as is practical.

In my experience very strict unit testing where you stub and mock everything outside your test concern can become a bit cumbersome. You would have had to unit test the delete_cache_id method separately and stub expectations on your calls to File and Dir as you mentioned. I find a happy middle ground between behavior/integration testing and strict unit testing is more practical with rspec, and tools like rcov are great for helping you visualize the coverage you are getting.

Anyway - thanks for carrying the ball most of the way on this. This was a good one to get nailed.

[trevorturk]

Great work, guys. I'm hoping we get a couple more issues cleaned up in the next week or two. I think we should have a quick turnaround for another gem release -- so bug me if you haven't seen any movement after a few weeks.