diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 8818bf2474..de9a156c04 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -772,7 +772,7 @@ module Homebrew end bin_names.each do |name| ["system", "shell_output", "pipe_output"].each do |cmd| - if text =~ %r{(def test|test do).*(#{Regexp.escape(HOMEBREW_PREFIX)}/bin/)?#{cmd}[\(\s]+['"]#{Regexp.escape(name)}[\s'"]}m + if text =~ /test do.*#{cmd}[\(\s]+['"]#{Regexp.escape(name)}[\s'"]/m problem %Q(fully scope test #{cmd} calls e.g. #{cmd} "\#{bin}/#{name}") end end @@ -803,10 +803,6 @@ module Homebrew problem "Use separate make calls" if line.include?("make && make") - if line =~ /shell_output\(['"].+['"], 0\)/ - problem "Passing 0 to shell_output() is redundant" - end - if line =~ /JAVA_HOME/i && !formula.requirements.map(&:class).include?(JavaRequirement) problem "Use `depends_on :java` to set JAVA_HOME" end diff --git a/Library/Homebrew/rubocops/class_cop.rb b/Library/Homebrew/rubocops/class_cop.rb index 8e0df3cbc8..3b667e75c8 100644 --- a/Library/Homebrew/rubocops/class_cop.rb +++ b/Library/Homebrew/rubocops/class_cop.rb @@ -22,6 +22,38 @@ module RuboCop end end end + + class TestCalls < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + test_calls(find_block(body_node, :test)) do |node, params| + p1, p2 = params + if match = string_content(p1).match(%r{(/usr/local/(s?bin))}) + offending_node(p1) + problem "use \#{#{match[2]}} instead of #{match[1]} in #{node}" + end + + if node == :shell_output && node_equals?(p2, 0) + offending_node(p2) + problem "Passing 0 to shell_output() is redundant" + end + end + end + + def autocorrect(node) + lambda do |corrector| + case node.type + when :str, :dstr + corrector.replace(node.source_range, node.source.to_s.sub(%r{(/usr/local/(s?bin))}, '#{\2}')) + when :int + corrector.remove(range_with_surrounding_comma(range_with_surrounding_space(range: node.source_range, side: :left))) + end + end + end + + def_node_search :test_calls, <<~EOS + (send nil? ${:system :shell_output :pipe_output} $...) + EOS + end end module FormulaAuditStrict diff --git a/Library/Homebrew/test/rubocops/class_cop_spec.rb b/Library/Homebrew/test/rubocops/class_cop_spec.rb index 6314978b45..7ada0bebd3 100644 --- a/Library/Homebrew/test/rubocops/class_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/class_cop_spec.rb @@ -48,6 +48,61 @@ describe RuboCop::Cop::FormulaAudit::ClassName do end end +describe RuboCop::Cop::FormulaAudit::TestCalls do + subject(:cop) { described_class.new } + + it "reports an offense when /usr/local/bin is found in test calls" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'https://example.com/foo-1.0.tgz' + + test do + system "/usr/local/bin/test" + ^^^^^^^^^^^^^^^^^^^^^ use \#{bin} instead of /usr/local/bin in system + end + end + RUBY + end + + it "reports an offense when passing 0 as the second parameter to shell_output" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'https://example.com/foo-1.0.tgz' + + test do + shell_output("\#{bin}/test", 0) + ^ Passing 0 to shell_output() is redundant + end + end + RUBY + end + + it "supports auto-correcting test calls" do + source = <<~RUBY + class Foo < Formula + url 'https://example.com/foo-1.0.tgz' + + test do + shell_output("/usr/local/sbin/test", 0) + end + end + RUBY + + corrected_source = <<~RUBY + class Foo < Formula + url 'https://example.com/foo-1.0.tgz' + + test do + shell_output("\#{sbin}/test") + end + end + RUBY + + new_source = autocorrect_source(source) + expect(new_source).to eq(corrected_source) + end +end + describe RuboCop::Cop::FormulaAuditStrict::Test do subject(:cop) { described_class.new }