Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audit: fixes for test checks #4698

Merged
merged 3 commits into from
Aug 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 1 addition & 5 deletions Library/Homebrew/dev-cmd/audit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ def audit_text
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
Expand Down Expand Up @@ -803,10 +803,6 @@ def line_problems(line, _lineno)

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
Expand Down
32 changes: 32 additions & 0 deletions Library/Homebrew/rubocops/class_cop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,38 @@ def autocorrect(node)
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy adding an autocorrect for this?

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
Expand Down
55 changes: 55 additions & 0 deletions Library/Homebrew/test/rubocops/class_cop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,61 @@ class Foo < Formula
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 }

Expand Down