From dd3267ece0b2252c13f23e10a8d905f96f067406 Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 27 Apr 2020 14:54:56 +0200 Subject: [PATCH 1/4] Add test for JSON-based cask config loader Previously, the JSON-based cask config loader was untested. This commit changes the interface to accept a string, making the loader easier to test. The commit also adds a test. --- Library/Homebrew/cask/config.rb | 6 +++--- Library/Homebrew/test/cask/config_spec.rb | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/cask/config.rb b/Library/Homebrew/cask/config.rb index 6ddad5430b..4ca3b367a9 100644 --- a/Library/Homebrew/cask/config.rb +++ b/Library/Homebrew/cask/config.rb @@ -34,15 +34,15 @@ module Cask def self.for_cask(cask) if cask.config_path.exist? - from_file(cask.config_path) + from_json(File.read(cask.config_path)) else global end end - def self.from_file(path) + def self.from_json(json) config = begin - JSON.parse(File.read(path)) + JSON.parse(json) rescue JSON::ParserError => e raise e, "Cannot parse #{path}: #{e}", e.backtrace end diff --git a/Library/Homebrew/test/cask/config_spec.rb b/Library/Homebrew/test/cask/config_spec.rb index f968f533c6..6add7d7360 100644 --- a/Library/Homebrew/test/cask/config_spec.rb +++ b/Library/Homebrew/test/cask/config_spec.rb @@ -3,6 +3,21 @@ describe Cask::Config, :cask do subject(:config) { described_class.new } + describe "::from_json" do + it "deserializes a configuration in JSON format" do + config = described_class.from_json <<~EOS + { + "default": { + "appdir": "/path/to/apps" + }, + "env": {}, + "explicit": {} + } + EOS + expect(config.appdir).to eq(Pathname("/path/to/apps")) + end + end + describe "#default" do it "returns the default directories" do expect(config.default[:appdir]).to eq(Pathname(TEST_TMPDIR).join("cask-appdir")) From cad8be3278923c926df2424b7a22be489ed03ded Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 27 Apr 2020 15:00:22 +0200 Subject: [PATCH 2/4] Add broken test, revealing test helper flaw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a broken test, which is meant to expose a flaw in the constructor of `Cask::Config`. That (broken) test still passes because there’s also a flaw in our test helper code. The helper flaw happens to neutralize the `Cask::Config` flaw. --- Library/Homebrew/test/cask/config_spec.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/test/cask/config_spec.rb b/Library/Homebrew/test/cask/config_spec.rb index 6add7d7360..412d5f5b85 100644 --- a/Library/Homebrew/test/cask/config_spec.rb +++ b/Library/Homebrew/test/cask/config_spec.rb @@ -67,7 +67,18 @@ describe Cask::Config, :cask do end context "when installing a cask and then adding a global default dir" do - let(:config) { described_class.new(default: { appdir: "/default/path/before/adding/fontdir" }) } + let(:config) { + json = <<~EOS + { + "default": { + "appdir": "/default/path/before/adding/fontdir" + }, + "env": {}, + "explicit": {} + } + EOS + described_class.from_json(json) + } describe "#appdir" do it "honors metadata of the installed cask" do From c85df70757f6768469f1d74fe4068bc0540163fe Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 27 Apr 2020 15:04:54 +0200 Subject: [PATCH 3/4] Fix flaw in Cask test helper This commit fixes a flaw in the Cask test helper, causing the broken `Cask::Config` test to actually fail. The flaw occurred while patching the `Cask::Config::DEFAULT_DIRS` hash. While the original hash uses strings as values, the patched one used `Pathname` values, masking a broken `Cask::Config::from_json` test. Now the broken test fails like it should. --- .../support/helper/spec/shared_context/homebrew_cask.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb b/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb index 290ec50b81..e0b01d9afe 100644 --- a/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb +++ b/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb @@ -10,7 +10,7 @@ module Cask class Config remove_const :DEFAULT_DIRS - DEFAULT_DIRS = { + DEFAULT_DIRS_PATHNAMES = { appdir: Pathname(TEST_TMPDIR)/"cask-appdir", prefpanedir: Pathname(TEST_TMPDIR)/"cask-prefpanedir", qlplugindir: Pathname(TEST_TMPDIR)/"cask-qlplugindir", @@ -26,6 +26,8 @@ module Cask vst3_plugindir: Pathname(TEST_TMPDIR)/"cask-vst3_plugindir", screen_saverdir: Pathname(TEST_TMPDIR)/"cask-screen_saverdir", }.freeze + + DEFAULT_DIRS = DEFAULT_DIRS_PATHNAMES.transform_values(&:to_s).freeze end end @@ -34,7 +36,7 @@ RSpec.shared_context "Homebrew Cask", :needs_macos do third_party_tap = Tap.fetch("third-party", "tap") begin - Cask::Config::DEFAULT_DIRS.values.each(&:mkpath) + Cask::Config::DEFAULT_DIRS_PATHNAMES.values.each(&:mkpath) Cask::Config.global.binarydir.mkpath Tap.default_cask_tap.tap do |tap| From b8aa808b9db409df0c84438cc641624aca059bf1 Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 27 Apr 2020 15:10:43 +0200 Subject: [PATCH 4/4] Make sure `DEFAULT_DIRS` values are Pathnames This commit fixes the PR #7417 bug. The call to `canonicalize` needs to wrap `DEFAULT_DIRS`, not the other way around. This was mixed up in PR #7417 due to an oversight. --- Library/Homebrew/cask/config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/config.rb b/Library/Homebrew/cask/config.rb index 4ca3b367a9..a415b74d78 100644 --- a/Library/Homebrew/cask/config.rb +++ b/Library/Homebrew/cask/config.rb @@ -69,7 +69,7 @@ module Cask attr_accessor :explicit def initialize(default: nil, env: nil, explicit: {}) - @default = DEFAULT_DIRS.merge(self.class.canonicalize(default)) if default + @default = self.class.canonicalize(DEFAULT_DIRS.merge(default)) if default @env = self.class.canonicalize(env) if env @explicit = self.class.canonicalize(explicit)