Fail static-analysis if there's output to stderr

TODO: fix offenders

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