core: add tests for box collection cleanup [GH-6002]

lib/vagrant/action/builtin/box_remove.rb

@@ -111,7 +111,7 @@ module Vagrant
                                 provider: box.provider,
                                 version: box.version))
             box.destroy!
-            env[:box_collection].clean_up(box)
+            env[:box_collection].clean(box.name)
 
             # Passes on the removed box to the rest of the middleware chain
             env[:box_removed] = box

lib/vagrant/box_collection.rb

@@ -12,9 +12,9 @@ module Vagrant
   # for accessing/finding individual boxes, adding new boxes, or deleting
   # boxes.
   class BoxCollection
-    TEMP_PREFIX = "vagrant-box-add-temp-"
-    VAGRANT_SLASH = "-VAGRANTSLASH-"
-    VAGRANT_COLON = "-VAGRANTCOLON-"
+    TEMP_PREFIX = "vagrant-box-add-temp-".freeze
+    VAGRANT_SLASH = "-VAGRANTSLASH-".freeze
+    VAGRANT_COLON = "-VAGRANTCOLON-".freeze
 
     # The directory where the boxes in this collection are stored.
     #
@@ -348,17 +348,12 @@ module Vagrant
       end
     end
 
-    # Removes the whole directory of a given box if there are no
-    # other versions nor providers of the box exist.
-    def clean_up(box)
-      return false if exists?(box.name)
-
-      box_directory = box.name
-        .gsub('/', VAGRANT_SLASH)
-        .gsub(':', VAGRANT_COLON)
-
-      path = File.join(directory, box_directory)
-      FileUtils.rm_r(path)
+    # Cleans the directory for a box by removing the folders that are
+    # empty.
+    def clean(name)
+      return false if exists?(name)
+      path = File.join(directory, dir_name(name))
+      FileUtils.rm_rf(path)
     end
 
     protected

test/unit/vagrant/action/builtin/box_remove_test.rb

@@ -30,7 +30,7 @@ describe Vagrant::Action::Builtin::BoxRemove do
 
     expect(box_collection).to receive(:find).with(
       "foo", :virtualbox, "1.0").and_return(box)
-    expect(box_collection).to receive(:clean_up).with(box)
+    expect(box_collection).to receive(:clean).with(box.name)
       .and_return(true)
     expect(box).to receive(:destroy!).once
     expect(app).to receive(:call).with(env).once
@@ -52,7 +52,7 @@ describe Vagrant::Action::Builtin::BoxRemove do
 
     expect(box_collection).to receive(:find).with(
       "foo", :virtualbox, "1.0").and_return(box)
-    expect(box_collection).to receive(:clean_up).with(box)
+    expect(box_collection).to receive(:clean).with(box.name)
       .and_return(false)
     expect(box).to receive(:destroy!).once
     expect(app).to receive(:call).with(env).once
@@ -74,7 +74,7 @@ describe Vagrant::Action::Builtin::BoxRemove do
 
     expect(box_collection).to receive(:find).with(
       "foo", :virtualbox, "1.0").and_return(box)
-    expect(box_collection).to receive(:clean_up).with(box)
+    expect(box_collection).to receive(:clean).with(box.name)
       .and_return(false)
     expect(box).to receive(:destroy!).once
     expect(app).to receive(:call).with(env).once
@@ -84,22 +84,6 @@ describe Vagrant::Action::Builtin::BoxRemove do
     expect(env[:box_removed]).to equal(box)
   end
 
-  it "deletes the whole directory of the box if it's the last box on the system" do
-    box_collection.stub(
-      all: [
-        ["foo", "1.0", :virtualbox],
-      ])
-
-    env[:box_name] = "foo"
-
-    expect(box_collection).to receive(:find).with(
-      "foo", :virtualbox, "1.0").and_return(box)
-    expect(box_collection).to receive(:clean_up).with(box)
-      .and_return(true)
-
-    subject.call(env)
-  end
-
   context "checking if a box is in use" do
     def new_entry(name, provider, version, valid=true)
       Vagrant::MachineIndex::Entry.new.tap do |entry|
@@ -132,7 +116,7 @@ describe Vagrant::Action::Builtin::BoxRemove do
       expect(box_collection).to receive(:find).with(
         "foo", :virtualbox, "1.0").and_return(box)
       expect(box).to receive(:destroy!).once
-      expect(box_collection).to receive(:clean_up).with(box)
+      expect(box_collection).to receive(:clean).with(box.name)
         .and_return(true)
 
       subject.call(env)
@@ -147,7 +131,7 @@ describe Vagrant::Action::Builtin::BoxRemove do
 
       expect(box_collection).to receive(:find).with(
         "foo", :virtualbox, "1.0").and_return(box)
-      expect(box_collection).to receive(:clean_up).with(box)
+      expect(box_collection).to receive(:clean).with(box.name)
         .and_return(true)
       expect(box).to receive(:destroy!).once
 

test/unit/vagrant/box_collection_test.rb

@@ -44,6 +44,65 @@ describe Vagrant::BoxCollection do
     end
   end
 
+  describe "#clean" do
+    it "removes the directory if no other versions of the box exists" do
+      # Create a few boxes, immediately destroy them
+      environment.box3("foo", "1.0", :virtualbox)
+      environment.box3("foo", "1.0", :vmware)
+
+      # Delete them all
+      subject.all.each do |parts|
+        subject.find(parts[0], parts[2], ">= 0").destroy!
+      end
+
+      # Cleanup
+      subject.clean("foo")
+
+      # Make sure the whole directory is empty
+      expect(environment.boxes_dir.children).to be_empty
+    end
+
+    it "doesn't remove the directory if a provider exists" do
+      # Create a few boxes, immediately destroy them
+      environment.box3("foo", "1.0", :virtualbox)
+      environment.box3("foo", "1.0", :vmware)
+
+      # Delete them all
+      subject.find("foo", :virtualbox, ">= 0").destroy!
+
+      # Cleanup
+      subject.clean("foo")
+
+      # Make sure the whole directory is not empty
+      expect(environment.boxes_dir.children).to_not be_empty
+
+      # Make sure the results still exist
+      results = subject.all
+      expect(results.length).to eq(1)
+      expect(results.include?(["foo", "1.0", :vmware])).to be
+    end
+
+    it "doesn't remove the directory if a version exists" do
+      # Create a few boxes, immediately destroy them
+      environment.box3("foo", "1.0", :virtualbox)
+      environment.box3("foo", "1.2", :virtualbox)
+
+      # Delete them all
+      subject.find("foo", :virtualbox, ">= 1.1").destroy!
+
+      # Cleanup
+      subject.clean("foo")
+
+      # Make sure the whole directory is not empty
+      expect(environment.boxes_dir.children).to_not be_empty
+
+      # Make sure the results still exist
+      results = subject.all
+      expect(results.length).to eq(1)
+      expect(results.include?(["foo", "1.0", :virtualbox])).to be
+    end
+  end
+
   describe "#find" do
     it "returns nil if the box does not exist" do
       expect(subject.find("foo", :i_dont_exist, ">= 0")).to be_nil