Add tests for upgrade recovery

Library/Homebrew/cask/lib/hbc/cli/upgrade.rb

@@ -41,6 +41,9 @@ module Hbc
                                     require_sha:    require_sha?,
                                     upgrade: true)
 
+          started_upgrade = false
+          new_artifacts_installed = false
+
           begin
             # Start new Cask's installation steps
             new_cask_installer.check_conflicts
@@ -51,9 +54,12 @@ module Hbc
 
             # Move the old Cask's artifacts back to staging
             old_cask_installer.start_upgrade
+            # And flag it so in case of error
+            started_upgrade = true
 
             # Install the new Cask
             new_cask_installer.install_artifacts
+            new_artifacts_installed = true
 
             new_cask_installer.enable_accessibility_access
 
@@ -61,8 +67,9 @@ module Hbc
             old_cask_installer.finalize_upgrade
           rescue CaskError => e
             opoo e.message
-            new_cask_installer.uninstall
-            old_cask_installer.revert_upgrade
+            new_cask_installer.uninstall_artifacts if new_artifacts_installed
+            new_cask_installer.purge_versioned_files
+            old_cask_installer.revert_upgrade if started_upgrade
           end
         end
       end

Library/Homebrew/test/cask/cli/upgrade_spec.rb

@@ -3,21 +3,39 @@ require_relative "shared_examples/invalid_option"
 describe Hbc::CLI::Upgrade, :cask do
   it_behaves_like "a command that handles invalid options"
 
-  before(:example) do
-    installed =
-      [
-        "outdated/local-caffeine",
-        "outdated/local-transmission",
-        "outdated/auto-updates",
-      ]
+  shared_context "Proper Casks" do
+    before(:example) do
+      installed =
+        [
+          "outdated/local-caffeine",
+          "outdated/local-transmission",
+          "outdated/auto-updates",
+        ]
+
+      installed.each { |cask| Hbc::CLI::Install.run(cask) }
+
+      allow_any_instance_of(described_class).to receive(:verbose?).and_return(true)
+    end
+  end
+
+  shared_context "Casks that will fail upon upgrade" do
+    before(:example) do
+      installed =
+        [
+          "outdated/bad-checksum",
+          "outdated/will-fail-if-upgraded",
+        ]
 
-    installed.each { |cask| Hbc::CLI::Install.run(cask) }
+      installed.each { |cask| Hbc::CLI::Install.run(cask) }
 
-    allow_any_instance_of(described_class).to receive(:verbose?).and_return(true)
+      allow_any_instance_of(described_class).to receive(:verbose?).and_return(true)
+    end
   end
 
   describe 'without --greedy it ignores the Casks with "version latest" or "auto_updates true"' do
-    it "updates all the installed Casks when no token is provided" do
+    include_context "Proper Casks"
+
+    it "and updates all the installed Casks when no token is provided" do
       expect(Hbc::CaskLoader.load("local-caffeine")).to be_installed
       expect(Hbc.appdir.join("Caffeine.app")).to be_a_directory
       expect(Hbc::CaskLoader.load("local-caffeine").versions).to include("1.2.2")
@@ -37,7 +55,7 @@ describe Hbc::CLI::Upgrade, :cask do
       expect(Hbc::CaskLoader.load("local-transmission").versions).to include("2.61")
     end
 
-    it "updates only the Casks specified in the command line" do
+    it "and updates only the Casks specified in the command line" do
       expect(Hbc::CaskLoader.load("local-caffeine")).to be_installed
       expect(Hbc.appdir.join("Caffeine.app")).to be_a_directory
       expect(Hbc::CaskLoader.load("local-caffeine").versions).to include("1.2.2")
@@ -58,7 +76,7 @@ describe Hbc::CLI::Upgrade, :cask do
       expect(Hbc::CaskLoader.load("local-transmission").versions).to include("2.60")
     end
 
-    it 'ignores "auto_updates" and "latest" Casks even when their tokens are provided in the command line' do
+    it 'and ignores "auto_updates" and "latest" Casks even when their tokens are provided in the command line' do
       expect(Hbc::CaskLoader.load("local-caffeine")).to be_installed
       expect(Hbc.appdir.join("Caffeine.app")).to be_a_directory
       expect(Hbc::CaskLoader.load("local-caffeine").versions).to include("1.2.2")
@@ -80,6 +98,8 @@ describe Hbc::CLI::Upgrade, :cask do
   end
 
   describe "with --greedy it checks additional Casks" do
+    include_context "Proper Casks"
+
     it 'includes the Casks with "auto_updates true" or "version latest" with --greedy' do
       expect(Hbc::CaskLoader.load("auto-updates")).to be_installed
       expect(Hbc.appdir.join("MyFancyApp.app")).to be_a_directory
@@ -124,4 +144,42 @@ describe Hbc::CLI::Upgrade, :cask do
       expect(Hbc::CaskLoader.load("auto-updates").versions).to include("2.61")
     end
   end
+
+  describe "handles borked upgrades" do
+    include_context "Casks that will fail upon upgrade"
+
+    output_reverted = Regexp.new <<~EOS
+      Warning: Reverting upgrade for Cask .*
+    EOS
+
+    it "by restoring the old Cask if the upgrade's install failed" do
+      expect(Hbc::CaskLoader.load("will-fail-if-upgraded")).to be_installed
+      expect(Hbc.appdir.join("container")).to be_a_file
+      expect(Hbc::CaskLoader.load("will-fail-if-upgraded").versions).to include("1.2.2")
+
+      expect {
+        described_class.run("will-fail-if-upgraded")
+      }.to output(output_reverted).to_stderr
+
+      expect(Hbc::CaskLoader.load("will-fail-if-upgraded")).to be_installed
+      expect(Hbc.appdir.join("container")).to be_a_file
+      expect(Hbc::CaskLoader.load("will-fail-if-upgraded").versions).to include("1.2.2")
+      expect(Hbc::CaskLoader.load("will-fail-if-upgraded").staged_path).to_not exist
+    end
+
+    it "by not restoring the old Cask if the upgrade failed pre-install" do
+      expect(Hbc::CaskLoader.load("bad-checksum")).to be_installed
+      expect(Hbc.appdir.join("Caffeine.app")).to be_a_directory
+      expect(Hbc::CaskLoader.load("bad-checksum").versions).to include("1.2.2")
+
+      expect {
+        described_class.run("bad-checksum")
+      }.to_not output(output_reverted).to_stderr
+
+      expect(Hbc::CaskLoader.load("bad-checksum")).to be_installed
+      expect(Hbc.appdir.join("Caffeine.app")).to be_a_directory
+      expect(Hbc::CaskLoader.load("bad-checksum").versions).to include("1.2.2")
+      expect(Hbc::CaskLoader.load("bad-checksum").staged_path).to_not exist
+    end
+  end
 end

Library/Homebrew/test/support/fixtures/cask/Casks/outdated/bad-checksum.rb

+cask 'bad-checksum' do
+  version '1.2.2'
+  sha256 '67cdb8a02803ef37fdbf7e0be205863172e41a561ca446cd84f0d7ab35a99d94'
+
+  url "file://#{TEST_FIXTURE_DIR}/cask/caffeine.zip"
+  homepage 'http://example.com/local-caffeine'
+
+  app 'Caffeine.app'
+end

Library/Homebrew/test/support/fixtures/cask/Casks/outdated/will-fail-if-upgraded.rb

+cask 'will-fail-if-upgraded' do
+  version '1.2.2'
+  sha256 'fab685fabf73d5a9382581ce8698fce9408f5feaa49fa10d9bc6c510493300f5'
+
+  url "file://#{TEST_FIXTURE_DIR}/cask/container.tar.gz"
+  homepage 'https://example.com/container-tar-gz'
+
+  app 'container'
+end

Library/Homebrew/test/support/fixtures/cask/Casks/will-fail-if-upgraded.rb

+cask 'will-fail-if-upgraded' do
+  version '1.2.3'
+  sha256 'e44ffa103fbf83f55c8d0b1bea309a43b2880798dae8620b1ee8da5e1095ec68'
+
+  url "file://#{TEST_FIXTURE_DIR}/cask/transmission-2.61.dmg"
+  homepage 'http://example.com/local-transmission'
+
+  app 'container'
+end