Merge pull request #4698 from jonchang/audit-scope

audit: fixes for test checks

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

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

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 }