diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2018-01-23 15:12:51 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2018-01-26 14:42:48 +0300 |
commit | 54ca8d0d6c4744be53c7044b9bbfa05acc358090 (patch) | |
tree | b4258bc91245eca92b2ef5321f18e67ee524c61c | |
parent | a0d57ee2b35f0e232aff930a94dfbcabe2860158 (diff) |
Fail static-analysis if there's output to stderr
TODO: fix offenders
-rw-r--r-- | lib/gitlab/popen.rb | 23 | ||||
-rw-r--r-- | lib/gitlab/popen/runner.rb | 46 | ||||
-rwxr-xr-x | scripts/static-analysis | 57 | ||||
-rw-r--r-- | spec/lib/gitlab/popen/runner_spec.rb | 139 | ||||
-rw-r--r-- | spec/lib/gitlab/popen_spec.rb | 16 | ||||
-rw-r--r-- | spec/support/javascript_fixtures_helpers.rb | 1 |
6 files changed, 258 insertions, 24 deletions
diff --git a/lib/gitlab/popen.rb b/lib/gitlab/popen.rb index 4bc5cda8cb5..05526a9ac94 100644 --- a/lib/gitlab/popen.rb +++ b/lib/gitlab/popen.rb @@ -5,7 +5,17 @@ module Gitlab module Popen extend self - def popen(cmd, path = nil, vars = {}) + Result = Struct.new(:cmd, :stdout, :stderr, :status, :duration) + + # Returns [stdout + stderr, status] + def popen(cmd, path = nil, vars = {}, &block) + result = popen_with_detail(cmd, path, vars, &block) + + [result.stdout << result.stderr, result.status] + end + + # Returns Result + def popen_with_detail(cmd, path = nil, vars = {}) unless cmd.is_a?(Array) raise "System commands must be given as an array of strings" end @@ -18,18 +28,21 @@ module Gitlab FileUtils.mkdir_p(path) end - cmd_output = "" + cmd_stdout = '' + cmd_stderr = '' cmd_status = 0 + start = Time.now + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| yield(stdin) if block_given? stdin.close - cmd_output << stdout.read - cmd_output << stderr.read + cmd_stdout = stdout.read + cmd_stderr = stderr.read cmd_status = wait_thr.value.exitstatus end - [cmd_output, cmd_status] + Result.new(cmd, cmd_stdout, cmd_stderr, cmd_status, Time.now - start) end end end diff --git a/lib/gitlab/popen/runner.rb b/lib/gitlab/popen/runner.rb new file mode 100644 index 00000000000..36284134707 --- /dev/null +++ b/lib/gitlab/popen/runner.rb @@ -0,0 +1,46 @@ +module Gitlab + module Popen + class Runner + attr_reader :results + + def initialize + @results = [] + end + + def run(commands, &block) + commands.each do |cmd| + # yield doesn't support blocks, so we need to use a block variable + block.call(cmd) do # rubocop:disable Performance/RedundantBlockCall + cmd_result = Gitlab::Popen.popen_with_detail(cmd) + + results << cmd_result + + cmd_result + end + end + end + + def all_good? + all_status_zero? && all_stderr_empty? + end + + def all_status_zero? + results.all? { |result| result.status.zero? } + end + + def all_stderr_empty? + results.all? { |result| result.stderr.empty? } + end + + def failed_results + results.select { |result| result.status.nonzero? } + end + + def warned_results + results.select do |result| + result.status.zero? && !result.stderr.empty? + end + end + end + end +end diff --git a/scripts/static-analysis b/scripts/static-analysis index 96d08287ded..392dc784945 100755 --- a/scripts/static-analysis +++ b/scripts/static-analysis @@ -1,6 +1,30 @@ #!/usr/bin/env ruby -require ::File.expand_path('../lib/gitlab/popen', __dir__) +# We don't have auto-loading here +require_relative '../lib/gitlab/popen' +require_relative '../lib/gitlab/popen/runner' + +def emit_warnings(static_analysis) + static_analysis.warned_results.each do |result| + puts + puts "**** #{result.cmd.join(' ')} had the following warnings:" + puts + puts result.stdout + puts result.stderr + puts + end +end + +def emit_errors(static_analysis) + static_analysis.failed_results.each do |result| + puts + puts "**** #{result.cmd.join(' ')} failed with the following error:" + puts + puts result.stdout + puts result.stderr + puts + end +end tasks = [ %w[bundle exec rake config_lint], @@ -17,18 +41,16 @@ tasks = [ %w[scripts/lint-rugged] ] -failed_tasks = tasks.reduce({}) do |failures, task| - start = Time.now - puts - puts "$ #{task.join(' ')}" +static_analysis = Gitlab::Popen::Runner.new - output, status = Gitlab::Popen.popen(task) - puts "==> Finished in #{Time.now - start} seconds" +static_analysis.run(tasks) do |cmd, &run| puts + puts "$ #{cmd.join(' ')}" - failures[task.join(' ')] = output unless status.zero? + result = run.call - failures + puts "==> Finished in #{result.duration} seconds" + puts end puts @@ -36,17 +58,20 @@ puts '===================================================' puts puts -if failed_tasks.empty? +if static_analysis.all_good? puts 'All static analyses passed successfully.' +elsif static_analysis.all_status_zero? + puts 'All static analyses passed successfully, but we have warnings:' + puts + + emit_warnings(static_analysis) + + exit 2 else puts 'Some static analyses failed:' - failed_tasks.each do |failed_task, output| - puts - puts "**** #{failed_task} failed with the following error:" - puts - puts output - end + emit_warnings(static_analysis) + emit_errors(static_analysis) exit 1 end diff --git a/spec/lib/gitlab/popen/runner_spec.rb b/spec/lib/gitlab/popen/runner_spec.rb new file mode 100644 index 00000000000..e0450b96e0f --- /dev/null +++ b/spec/lib/gitlab/popen/runner_spec.rb @@ -0,0 +1,139 @@ +require 'spec_helper' + +describe Gitlab::Popen::Runner do + subject { described_class.new } + + describe '#run' do + it 'runs the command and returns the result' do + run_command + + expect(Gitlab::Popen).to have_received(:popen_with_detail) + end + end + + describe '#all_good?' do + it 'returns true when exit status is 0 and stderr is empty' do + run_command + + expect(subject).to be_all_good + end + + it 'returns false when exit status is not 0' do + run_command(exitstatus: 1) + + expect(subject).not_to be_all_good + end + + it 'returns false when exit stderr has something' do + run_command(stderr: 'stderr') + + expect(subject).not_to be_all_good + end + end + + describe '#all_status_zero?' do + it 'returns true when exit status is 0' do + run_command + + expect(subject).to be_all_status_zero + end + + it 'returns false when exit status is not 0' do + run_command(exitstatus: 1) + + expect(subject).not_to be_all_status_zero + end + + it 'returns true' do + run_command(stderr: 'stderr') + + expect(subject).to be_all_status_zero + end + end + + describe '#all_stderr_empty?' do + it 'returns true when stderr is empty' do + run_command + + expect(subject).to be_all_stderr_empty + end + + it 'returns true when exit status is not 0' do + run_command(exitstatus: 1) + + expect(subject).to be_all_stderr_empty + end + + it 'returns false when exit stderr has something' do + run_command(stderr: 'stderr') + + expect(subject).not_to be_all_stderr_empty + end + end + + describe '#failed_results' do + it 'returns [] when everything is passed' do + run_command + + expect(subject.failed_results).to be_empty + end + + it 'returns the result when exit status is not 0' do + result = run_command(exitstatus: 1) + + expect(subject.failed_results).to contain_exactly(result) + end + + it 'returns [] when exit stderr has something' do + run_command(stderr: 'stderr') + + expect(subject.failed_results).to be_empty + end + end + + describe '#warned_results' do + it 'returns [] when everything is passed' do + run_command + + expect(subject.warned_results).to be_empty + end + + it 'returns [] when exit status is not 0' do + run_command(exitstatus: 1) + + expect(subject.warned_results).to be_empty + end + + it 'returns the result when exit stderr has something' do + result = run_command(stderr: 'stderr') + + expect(subject.warned_results).to contain_exactly(result) + end + end + + def run_command( + command: 'command', + stdout: 'stdout', + stderr: '', + exitstatus: 0, + status: double(exitstatus: exitstatus, success?: exitstatus.zero?), + duration: 0.1) + + result = + Gitlab::Popen::Result.new(command, stdout, stderr, status, duration) + + allow(Gitlab::Popen) + .to receive(:popen_with_detail) + .and_return(result) + + subject.run([command]) do |cmd, &run| + expect(cmd).to eq(command) + + cmd_result = run.call + + expect(cmd_result).to eq(result) + end + + subject.results.first + end +end diff --git a/spec/lib/gitlab/popen_spec.rb b/spec/lib/gitlab/popen_spec.rb index b145ca36f26..3401c86e540 100644 --- a/spec/lib/gitlab/popen_spec.rb +++ b/spec/lib/gitlab/popen_spec.rb @@ -1,11 +1,23 @@ require 'spec_helper' -describe 'Gitlab::Popen' do +describe Gitlab::Popen do let(:path) { Rails.root.join('tmp').to_s } before do @klass = Class.new(Object) - @klass.send(:include, Gitlab::Popen) + @klass.send(:include, described_class) + end + + describe '.popen_with_detail' do + subject { @klass.new.popen_with_detail(cmd) } + + let(:cmd) { %W[#{Gem.ruby} -e $stdout.puts(1);$stderr.puts(2);exit(3)] } + + it { expect(subject.cmd).to eq(cmd) } + it { expect(subject.stdout).to eq("1\n") } + it { expect(subject.stderr).to eq("2\n") } + it { expect(subject.status).to eq(3) } + it { expect(subject.duration).to be_kind_of(Numeric) } end context 'zero status' do diff --git a/spec/support/javascript_fixtures_helpers.rb b/spec/support/javascript_fixtures_helpers.rb index 923c8080e6c..2197bc9d853 100644 --- a/spec/support/javascript_fixtures_helpers.rb +++ b/spec/support/javascript_fixtures_helpers.rb @@ -1,6 +1,5 @@ require 'action_dispatch/testing/test_request' require 'fileutils' -require 'gitlab/popen' module JavaScriptFixturesHelpers include Gitlab::Popen |