From e5b56e485f20f48bc4170ec688611a0104b47cfe Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Fri, 4 Sep 2020 04:14:37 +0200 Subject: [PATCH 1/4] Move cask audit implications into `Audit`. --- Library/Homebrew/cask/audit.rb | 22 ++++-- Library/Homebrew/cask/auditor.rb | 71 +++++++++++++------- Library/Homebrew/cask/cmd/audit.rb | 12 ++-- Library/Homebrew/test/cask/audit_spec.rb | 2 - Library/Homebrew/test/cask/cmd/audit_spec.rb | 17 ++--- 5 files changed, 71 insertions(+), 53 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index ae7463fb24..4edce3cc5a 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -14,22 +14,32 @@ module Cask class Audit extend Predicable - attr_reader :cask, :commit_range, :download + attr_reader :cask, :download attr_predicate :appcast?, :new_cask?, :strict?, :online? - def initialize(cask, appcast: false, download: false, quarantine: nil, - token_conflicts: false, online: false, strict: false, - new_cask: false, commit_range: nil, command: SystemCommand) + def initialize(cask, appcast: nil, download: nil, quarantine: nil, + token_conflicts: nil, online: nil, strict: nil, + new_cask: nil) + + # `new_cask` implies `online` and `strict` + online = new_cask if online.nil? + strict = new_cask if strict.nil? + + # `online` implies `appcast` and `download` + appcast = online if appcast.nil? + download = online if download.nil? + + # `strict` implies `token_conflicts` + token_conflicts = strict if token_conflicts.nil? + @cask = cask @appcast = appcast @download = Download.new(cask, quarantine: quarantine) if download @online = online @strict = strict @new_cask = new_cask - @commit_range = commit_range @token_conflicts = token_conflicts - @command = command end def run! diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index 9aa4820a56..87179cd3e9 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -9,33 +9,50 @@ module Cask class Auditor extend Predicable - def self.audit(cask, audit_download: false, audit_appcast: false, - audit_online: false, audit_strict: false, - audit_token_conflicts: false, audit_new_cask: false, - quarantine: true, commit_range: nil, language: nil) - new(cask, audit_download: audit_download, - audit_appcast: audit_appcast, - audit_online: audit_online, - audit_new_cask: audit_new_cask, - audit_strict: audit_strict, - audit_token_conflicts: audit_token_conflicts, - quarantine: quarantine, commit_range: commit_range, language: language).audit + def self.audit( + cask, + audit_download: nil, + audit_appcast: nil, + audit_online: nil, + audit_new_cask: nil, + audit_strict: nil, + audit_token_conflicts: nil, + quarantine: nil, + language: nil + ) + new( + cask, + audit_download: audit_download, + audit_appcast: audit_appcast, + audit_online: audit_online, + audit_new_cask: audit_new_cask, + audit_strict: audit_strict, + audit_token_conflicts: audit_token_conflicts, + quarantine: quarantine, + language: language, + ).audit end - attr_reader :cask, :commit_range, :language + attr_reader :cask, :language - def initialize(cask, audit_download: false, audit_appcast: false, - audit_online: false, audit_strict: false, - audit_token_conflicts: false, audit_new_cask: false, - quarantine: true, commit_range: nil, language: nil) + def initialize( + cask, + audit_download: nil, + audit_appcast: nil, + audit_online: nil, + audit_strict: nil, + audit_token_conflicts: nil, + audit_new_cask: nil, + quarantine: nil, + language: nil + ) @cask = cask @audit_download = audit_download @audit_appcast = audit_appcast @audit_online = audit_online - @audit_strict = audit_strict @audit_new_cask = audit_new_cask + @audit_strict = audit_strict @quarantine = quarantine - @commit_range = commit_range @audit_token_conflicts = audit_token_conflicts @language = language end @@ -76,14 +93,16 @@ module Cask end def audit_cask_instance(cask) - audit = Audit.new(cask, appcast: audit_appcast?, - online: audit_online?, - strict: audit_strict?, - new_cask: audit_new_cask?, - token_conflicts: audit_token_conflicts?, - download: audit_download?, - quarantine: quarantine?, - commit_range: commit_range) + audit = Audit.new( + cask, + appcast: audit_appcast?, + online: audit_online?, + strict: audit_strict?, + new_cask: audit_new_cask?, + token_conflicts: audit_token_conflicts?, + download: audit_download?, + quarantine: quarantine?, + ) audit.run! audit end diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 6bf08325a4..9c416965c7 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -38,16 +38,14 @@ module Cask require "cask/auditor" Homebrew.auditing = true - strict = args.new_cask? || args.strict? - online = args.new_cask? || args.online? options = { - audit_download: online || args.download?, - audit_appcast: online || args.appcast?, - audit_online: online, - audit_strict: strict, + audit_download: args.download?, + audit_appcast: args.appcast?, + audit_online: args.online?, + audit_strict: args.strict?, audit_new_cask: args.new_cask?, - audit_token_conflicts: strict || args.token_conflicts?, + audit_token_conflicts: args.token_conflicts?, quarantine: args.quarantine?, language: args.language, }.compact diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index c39a09590a..803865054f 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -34,11 +34,9 @@ describe Cask::Audit, :cask do let(:token_conflicts) { false } let(:strict) { false } let(:new_cask) { false } - let(:fake_system_command) { class_double(SystemCommand) } let(:audit) { described_class.new(cask, download: download, token_conflicts: token_conflicts, - command: fake_system_command, strict: strict, new_cask: new_cask) } diff --git a/Library/Homebrew/test/cask/cmd/audit_spec.rb b/Library/Homebrew/test/cask/cmd/audit_spec.rb index 587734b0a6..968e61ce64 100644 --- a/Library/Homebrew/test/cask/cmd/audit_spec.rb +++ b/Library/Homebrew/test/cask/cmd/audit_spec.rb @@ -57,10 +57,10 @@ describe Cask::Cmd::Audit, :cask do described_class.run("casktoken", "--token-conflicts") end - it "passes `audit_strict` and `audit_token_conflicts` if the `--strict` flag is specified" do + it "passes `audit_strict` if the `--strict` flag is specified" do allow(Cask::CaskLoader).to receive(:load).and_return(cask) expect(Cask::Auditor).to receive(:audit) - .with(cask, audit_strict: true, audit_token_conflicts: true, quarantine: true) + .with(cask, audit_strict: true, quarantine: true) .and_return(result) described_class.run("casktoken", "--strict") @@ -69,23 +69,16 @@ describe Cask::Cmd::Audit, :cask do it "passes `audit_online` if the `--online` flag is specified" do allow(Cask::CaskLoader).to receive(:load).and_return(cask) expect(Cask::Auditor).to receive(:audit) - .with(cask, audit_online: true, audit_appcast: true, audit_download: true, quarantine: true) + .with(cask, audit_online: true, quarantine: true) .and_return(result) described_class.run("casktoken", "--online") end - it "passes `audit_appcast`, `audit_download`, `audit_new_cask`, `audit_online`, `audit_strict` " \ - "and `audit_token_conflicts` if the `--new-cask` flag is specified" do + it "passes `audit_new_cask` if the `--new-cask` flag is specified" do allow(Cask::CaskLoader).to receive(:load).and_return(cask) expect(Cask::Auditor).to receive(:audit) - .with(cask, audit_appcast: true, - audit_download: true, - audit_new_cask: true, - audit_online: true, - audit_strict: true, - audit_token_conflicts: true, - quarantine: true) + .with(cask, audit_new_cask: true, quarantine: true) .and_return(result) described_class.run("casktoken", "--new-cask") From 7c05b2f2311151da9ec83726979c7021f87c83c5 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Fri, 4 Sep 2020 04:15:18 +0200 Subject: [PATCH 2/4] Support `--no-appcast` for `brew cask audit`. --- Library/Homebrew/cask/cmd/audit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 9c416965c7..8c4626b773 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -19,7 +19,7 @@ module Cask super do switch "--download", description: "Audit the downloaded file" - switch "--appcast", + switch "--[no-]appcast", description: "Audit the appcast" switch "--token-conflicts", description: "Audit for token conflicts" From 16c17433d5e1a024a567389585f43bd2bebf19fd Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Fri, 4 Sep 2020 05:19:33 +0200 Subject: [PATCH 3/4] Fix passing options through `Auditor`. --- Library/Homebrew/cask/auditor.rb | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index 87179cd3e9..df3e396005 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -7,8 +7,6 @@ module Cask # # @api private class Auditor - extend Predicable - def self.audit( cask, audit_download: nil, @@ -57,9 +55,6 @@ module Cask @language = language end - attr_predicate :audit_appcast?, :audit_download?, :audit_online?, - :audit_strict?, :audit_new_cask?, :audit_token_conflicts?, :quarantine? - def audit warnings = Set.new errors = Set.new @@ -95,13 +90,13 @@ module Cask def audit_cask_instance(cask) audit = Audit.new( cask, - appcast: audit_appcast?, - online: audit_online?, - strict: audit_strict?, - new_cask: audit_new_cask?, - token_conflicts: audit_token_conflicts?, - download: audit_download?, - quarantine: quarantine?, + appcast: @audit_appcast, + online: @audit_online, + strict: @audit_strict, + new_cask: @audit_new_cask, + token_conflicts: @audit_token_conflicts, + download: @audit_download, + quarantine: @quarantine, ) audit.run! audit From f1bf6c03c3f6585cdcc648ccc387ef933ab9a446 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Fri, 4 Sep 2020 05:29:56 +0200 Subject: [PATCH 4/4] Add tests for implications. --- Library/Homebrew/cask/audit.rb | 4 +- Library/Homebrew/test/cask/audit_spec.rb | 49 ++++++++++++++++++++---- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index 4edce3cc5a..b605908b79 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -16,7 +16,7 @@ module Cask attr_reader :cask, :download - attr_predicate :appcast?, :new_cask?, :strict?, :online? + attr_predicate :appcast?, :new_cask?, :strict?, :online?, :token_conflicts? def initialize(cask, appcast: nil, download: nil, quarantine: nil, token_conflicts: nil, online: nil, strict: nil, @@ -355,7 +355,7 @@ module Cask end def check_token_conflicts - return unless @token_conflicts + return unless token_conflicts? return unless core_formula_names.include?(cask.token) add_warning "possible duplicate, cask token conflicts with Homebrew core formula: #{core_formula_url}" diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index 803865054f..db741fd133 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -30,17 +30,52 @@ describe Cask::Audit, :cask do end let(:cask) { instance_double(Cask::Cask) } - let(:download) { false } - let(:token_conflicts) { false } - let(:strict) { false } - let(:new_cask) { false } + let(:new_cask) { nil } + let(:online) { nil } + let(:strict) { nil } + let(:token_conflicts) { nil } let(:audit) { - described_class.new(cask, download: download, - token_conflicts: token_conflicts, + described_class.new(cask, online: online, + strict: strict, - new_cask: new_cask) + new_cask: new_cask, + token_conflicts: token_conflicts) } + describe "#new" do + context "when `new_cask` is specified" do + let(:new_cask) { true } + + it "implies `online`" do + expect(audit).to be_online + end + + it "implies `strict`" do + expect(audit).to be_strict + end + end + + context "when `online` is specified" do + let(:online) { true } + + it "implies `appcast`" do + expect(audit.appcast?).to be true + end + + it "implies `download`" do + expect(audit.download).to be_truthy + end + end + + context "when `strict` is specified" do + let(:strict) { true } + + it "implies `token_conflicts`" do + expect(audit.token_conflicts?).to be true + end + end + end + describe "#result" do subject { audit.result }