Add missing tests for all new features

6 files changed, 361 insertions, 123 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 0674b40a898..1fdd674139d 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -455,7 +455,7 @@ module Ci
     end
 
     def erase_trace!
-      trace.erase_trace!
+      trace.erase!
     end
 
     def update_erased!(user = nil)
diff --git a/lib/ci/ansi2html.rb b/lib/ci/ansi2html.rb
index 9338ab062dd..1020452480a 100644
--- a/lib/ci/ansi2html.rb
+++ b/lib/ci/ansi2html.rb
@@ -23,8 +23,6 @@ module Ci
       cross:      0x10,
     }.freeze
 
-    Result = Struct.new(:html, :state, :append, :truncated, :offset, :size, :total_size)
-
     def self.convert(ansi, state = nil)
       Converter.new.convert(ansi, state)
     end
@@ -173,8 +171,15 @@ module Ci
 
         close_open_tags()
 
-        Result.new(@out, state, append, truncated,
-          start_offset, stream.tell - start_offset, stream.size)
+        OpenStruct.new(
+          html: @out,
+          state: state,
+          append: append,
+          truncated: truncated,
+          offset: start_offset,
+          size: stream.tell - start_offset,
+          total: stream.size
+        )
       end
 
       def handle_sequence(s)
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index b17c6ff4488..5b835bb669a 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -9,10 +9,6 @@ module Gitlab
         @job = job
       end
 
-      def exist?
-        current_path.present? || old_trace.present?
-      end
-
       def html(last_lines: nil)
         read do |stream|
           stream.html(last_lines: last_lines)
@@ -49,12 +45,8 @@ module Gitlab
         end
       end
 
-      def erase_trace!
-        paths.find do |trace_path|
-          FileUtils.rm(trace_path, force: true)
-        end
-
-        job.erase_old_trace!
+      def exist?
+        current_path.present? || old_trace.present?
       end
 
       def read
@@ -83,6 +75,14 @@ module Gitlab
         stream&.close
       end
 
+      def erase!
+        paths.each do |trace_path|
+          FileUtils.rm(trace_path, force: true)
+        end
+
+        job.erase_old_trace!
+      end
+
       private
 
       def ensure_path
diff --git a/lib/gitlab/ci/trace/stream.rb b/lib/gitlab/ci/trace/stream.rb
index 015a3ae13ba..2af94e2c60e 100644
--- a/lib/gitlab/ci/trace/stream.rb
+++ b/lib/gitlab/ci/trace/stream.rb
@@ -10,12 +10,14 @@ module Gitlab
 
         delegate :close, :tell, :seek, :size, :path, :truncate, to: :stream, allow_nil: true
 
+        delegate :valid?, to: :stream, as: :present?, allow_nil: true
+
         def initialize
           @stream = yield
         end
 
         def valid?
-          self.stream&.ready?
+          self.stream.present?
         end
 
         def file?
@@ -46,7 +48,7 @@ module Gitlab
         def raw(last_lines: nil)
           return unless valid?
 
-          if last_lines
+          if last_lines.to_i > 0
             read_last_lines(last_lines)
           else
             stream.read
@@ -73,13 +75,11 @@ module Gitlab
 
           stream.each_line do |line|
             matches = line.scan(regex)
-            match = matches.last if matches.is_a?(Array)
-          end
-
-          coverage = match.gsub(/\d+(\.\d+)?/).first
+            next unless matches.is_a?(Array)
 
-          if coverage.present?
-            coverage.to_f
+            match = matches.flatten.last
+            coverage = match.gsub(/\d+(\.\d+)?/).first
+            return coverage.to_f if coverage.present?
           end
         rescue
           # if bad regex or something goes wrong we dont want to interrupt transition
diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb
index 52014203f09..667e0ec21a4 100644
--- a/spec/lib/gitlab/ci/trace/stream_spec.rb
+++ b/spec/lib/gitlab/ci/trace/stream_spec.rb
@@ -1,42 +1,106 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Trace::Stream do
-  describe '#raw' do
-    context 'limit max lines' do
-      let(:path) { __FILE__ }
-      let(:lines) { File.readlines(path) }
+  describe 'delegates' do
+    subject { described_class.new { nil } }
 
-      before do
-        allow(described_class).to receive(:LIMIT_SIZE).and_return(random_buffer)
+    it { is_expected.to delegate_method(:close).to(:stream) }
+    it { is_expected.to delegate_method(:tell).to(:stream) }
+    it { is_expected.to delegate_method(:seek).to(:stream) }
+    it { is_expected.to delegate_method(:size).to(:stream) }
+    it { is_expected.to delegate_method(:path).to(:stream) }
+    it { is_expected.to delegate_method(:truncate).to(:stream) }
+    it { is_expected.to delegate_method(:valid?).to(:stream).as(:present?) }
+    it { is_expected.to delegate_method(:file?).to(:path).as(:present?) }
+  end
+
+  describe '#limit' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
       end
+    end
+
+    it 'if size is larger we start from beggining' do
+      stream.limit(10)
+
+      expect(stream.tell).to eq(0)
+    end
+
+    it 'if size is smaller we start from the end' do
+      stream.limit(2)
 
-      subject do
-        described_class.new do
-          File.open(path)
-        end
+      expect(stream.tell).to eq(6)
+    end
+  end
+
+  describe '#append' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
       end
+    end
 
-      it 'returns last few lines' do
-        10.times do
-          last_lines = random_lines
+    it "truncates and append content" do
+      stream.append("89", 4)
+      stream.seek(0)
 
-          expected = lines.last(last_lines).join
-          result = subject.raw(last_lines: last_lines)
+      expect(stream.size).to eq(6)
+      expect(stream.raw).to eq("123489")
+    end
+  end
 
-          expect(result).to eq(expected)
-          expect(result.encoding).to eq(Encoding.default_external)
-        end
+  describe '#set' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
       end
+    end
 
-      it 'returns everything if trying to get too many lines' do
-        result = subject.raw(last_lines: lines.size * 2)
+    before do
+      stream.set("8901")
+    end
 
-        expect(result).to eq(lines.join)
+    it "overwrite content" do
+      stream.seek(0)
+
+      expect(stream.size).to eq(4)
+      expect(stream.raw).to eq("8901")
+    end
+  end
+
+  describe '#raw' do
+    let(:path) { __FILE__ }
+    let(:lines) { File.readlines(path) }
+    let(:stream) do
+      described_class.new do
+        File.open(path)
+      end
+    end
+
+    it 'returns all contents if last_lines is not specified' do
+      result = stream.raw
+
+      expect(result).to eq(lines.join)
+      expect(result.encoding).to eq(Encoding.default_external)
+    end
+
+    context 'limit max lines' do
+      before do
+        # specifying BUFFER_SIZE forces to seek backwards
+        allow(described_class).to receive(:BUFFER_SIZE)
+          .and_return(2)
+      end
+
+      it 'returns last few lines' do
+        result = stream.raw(last_lines: 2)
+
+        expect(result).to eq(lines.last(2).join)
         expect(result.encoding).to eq(Encoding.default_external)
       end
 
-      it 'returns all contents if last_lines is not specified' do
-        result = subject.raw
+      it 'returns everything if trying to get too many lines' do
+        result = stream.raw(last_lines: lines.size * 2)
 
         expect(result).to eq(lines.join)
         expect(result.encoding).to eq(Encoding.default_external)
@@ -44,17 +108,100 @@ describe Gitlab::Ci::Trace::Stream do
 
       it 'raises an error if not passing an integer for last_lines' do
         expect do
-          subject.raw(last_lines: lines)
+          stream.raw(last_lines: lines)
         end.to raise_error(ArgumentError)
       end
+    end
+  end
+
+  describe '#html_with_state' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("1234")
+      end
+    end
+
+    it 'returns html content with state' do
+      result = stream.html_with_state
+
+      expect(result.html).to eq("1234")
+    end
+
+    context 'follow-up state' do
+      let!(:last_result) { stream.html_with_state }
+
+      before do
+        stream.append("5678", 4)
+        stream.seek(0)
+      end
+
+      it "returns appended trace" do
+        result = stream.html_with_state(last_result.state)
 
-      def random_lines
-        Random.rand(lines.size) + 1
+        expect(result.append).to be_truthy
+        expect(result.html).to eq("5678")
       end
+    end
+  end
 
-      def random_buffer
-        Random.rand(subject.size) + 1
+  describe '#html' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12\n34\n56")
       end
     end
+
+    it "returns html" do
+      expect(stream.html).to eq("12<br>34<br>56")
+    end
+
+    it "returns html for last line only" do
+      expect(stream.html(last_lines: 1)).to eq("56")
+    end
+  end
+
+  describe '#extract_coverage' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new(data)
+      end
+    end
+
+    subject { stream.extract_coverage(regex) }
+
+    context 'valid content & regex' do
+      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to eq(98.29) }
+    end
+
+    context 'valid content & bad regex' do
+      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered\n' }
+      let(:regex) { 'very covered' }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'no coverage content & regex' do
+      let(:data) { 'No coverage for today :sad:' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'multiple results in content & regex' do
+      let(:data) { ' (98.39%) covered. (98.29%) covered' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to eq(98.29) }
+    end
+
+    context 'using a regex capture' do
+      let(:data) { 'TOTAL      9926   3489    65%' }
+      let(:regex) { 'TOTAL\s+\d+\s+\d+\s+(\d{1,3}\%)' }
+
+      it { is_expected.to eq(65) }
+    end
   end
 end
diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb
index 8276afe89f1..d658ac9e1ec 100644
--- a/spec/lib/gitlab/ci/trace_spec.rb
+++ b/spec/lib/gitlab/ci/trace_spec.rb
@@ -4,132 +4,218 @@ describe Gitlab::Ci::Trace do
   let(:build) { create(:ci_build) }
   let(:trace) { described_class.new(build) }
 
-  describe '#append' do
-    subject { trace.html }
+  describe "associations" do
+    it { expect(trace).to respond_to(:job) }
+    it { expect(trace).to delegate_method(:old_trace).to(:job) }
+  end
+
+  describe '#html' do
+    before do
+      trace.set("12\n34")
+    end
+
+    it "returns formatted html" do
+      expect(trace.html).to eq("12<br>34")
+    end
+
+    it "returns last line of formatted html" do
+      expect(trace.html(last_lines: 1)).to eq("34")
+    end
+  end
+
+  describe '#raw' do
+    before do
+      trace.set("12\n34")
+    end
+
+    it "returns raw output" do
+      expect(trace.raw).to eq("12\n34")
+    end
+
+    it "returns last line of raw output" do
+      expect(trace.raw(last_lines: 1)).to eq("34")
+    end
+  end
+
+  describe '#extract_coverage' do
+    let(:regex) { '\(\d+.\d+\%\) covered' }
+
+    before do
+      trace.set('Coverage 1033 / 1051 LOC (98.29%) covered')
+    end
+
+    it "returns valid coverage" do
+      expect(trace.extract_coverage(regex)).to eq(98.29)
+    end
+  end
 
-    context 'when build.trace hides runners token' do
+  describe '#set' do
+    before do
+      trace.set("12")
+    end
+
+    it "returns trace" do
+      expect(trace.raw).to eq("12")
+    end
+
+    context 'overwrite trace' do
+      before do
+        trace.set("34")
+      end
+
+      it "returns new trace" do
+        expect(trace.raw).to eq("34")
+      end
+    end
+
+    context 'runners token' do
       let(:token) { 'my_secret_token' }
 
       before do
         build.project.update(runners_token: token)
-        trace.append(token, 0)
+        trace.set(token)
       end
 
-      it { is_expected.not_to include(token) }
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
     end
 
-    context 'when build.trace hides build token' do
+    context 'hides build token' do
       let(:token) { 'my_secret_token' }
 
       before do
         build.update(token: token)
-        trace.append(token, 0)
+        trace.set(token)
       end
 
-      it { is_expected.not_to include(token) }
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
     end
   end
 
-  describe '#extract_coverage' do
-    subject { trace.extract_coverage(regex) }
-
+  describe '#append' do
     before do
-      trace.set(data)
+      trace.set("1234")
     end
 
-    context 'valid content & regex' do
-      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered' }
-      let(:regex) { '\(\d+.\d+\%\) covered' }
+    context 'append trace' do
+      before do
+      end
 
-      it { is_expected.to eq(98.29) }
+      it "returns correct trace" do
+        expect(trace.append("56", 4)).to eq(6)
+        expect(trace.raw).to eq("123456")
+      end
     end
 
-    context 'valid content & bad regex' do
-      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered\n' }
-      let(:regex) { 'very covered' }
-
-      it { is_expected.to be_nil }
+    context 'tries to append trace at different offset' do
+      it "fails with append" do
+        expect(trace.append("56", 2)).to eq(-4)
+        expect(trace.raw).to eq("1234")
+      end
     end
 
-    context 'no coverage content & regex' do
-      let(:data) { 'No coverage for today :sad:' }
-      let(:regex) { '\(\d+.\d+\%\) covered' }
-
-      it { is_expected.to be_nil }
-    end
+    context 'runners token' do
+      let(:token) { 'my_secret_token' }
 
-    context 'multiple results in content & regex' do
-      let(:data) { ' (98.39%) covered. (98.29%) covered' }
-      let(:regex) { '\(\d+.\d+\%\) covered' }
+      before do
+        build.project.update(runners_token: token)
+        trace.append(token, 0)
+      end
 
-      it { is_expected.to eq(98.29) }
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
     end
 
-    context 'using a regex capture' do
-      let(:data) { 'TOTAL      9926   3489    65%' }
-      let(:regex) { 'TOTAL\s+\d+\s+\d+\s+(\d{1,3}\%)' }
+    context 'build token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(token: token)
+        trace.append(token, 0)
+      end
 
-      it { is_expected.to eq(65) }
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
     end
   end
 
-  describe '#has_trace_file?' do
-    context 'when there is no trace' do
-      it { expect(build.has_trace_file?).to be_falsey }
-      it { expect(build.trace.raw).to be_nil }
+  describe 'trace handling' do
+    context 'trace does not exist' do
+      it { expect(trace.exist?).to be(false) }
     end
 
-    context 'when there is a trace' do
-      context 'when trace is stored in file' do
-        let(:build_with_trace) { create(:ci_build, :trace) }
+    context 'new trace path is used' do
+      before do
+        trace.send(:ensure_directory)
 
-        it { expect(build_with_trace.has_trace_file?).to be_truthy }
-        it { expect(build_with_trace.trace.raw).to eq('BUILD TRACE') }
+        File.open(trace.send(:default_path), "w") do |file|
+          file.write("data")
+        end
       end
 
-      context 'when trace is stored in old file' do
-        before do
-          allow(build.project).to receive(:ci_id).and_return(999)
-          allow(File).to receive(:exist?).with(build.path_to_trace).and_return(false)
-          allow(File).to receive(:exist?).with(build.old_path_to_trace).and_return(true)
-          allow(File).to receive(:read).with(build.old_path_to_trace).and_return(test_trace)
-        end
+      it "trace exist" do
+        expect(trace.exist?).to be(true)
+      end
 
-        it { expect(build.has_trace_file?).to be_truthy }
-        it { expect(build.trace.raw).to eq(test_trace) }
+      it "can be erased" do
+        trace.erase!
+        expect(trace.exist?).to be(false)
       end
+    end
+
+    context 'deprecated path' do
+      let(:path) { trace.send(:deprecated_path) }
 
-      context 'when trace is stored in DB' do
+      context 'with valid ci_id' do
         before do
-          allow(build.project).to receive(:ci_id).and_return(nil)
-          allow(build).to receive(:read_attribute).with(:trace).and_return(test_trace)
-          allow(File).to receive(:exist?).with(build.path_to_trace).and_return(false)
-          allow(File).to receive(:exist?).with(build.old_path_to_trace).and_return(false)
+          build.project.update(ci_id: 1000)
+
+          FileUtils.mkdir_p(File.dirname(path))
+
+          File.open(path, "w") do |file|
+            file.write("data")
+          end
         end
 
-        it { expect(build.has_trace_file?).to be_falsey }
-        it { expect(build.trace.raw).to eq(test_trace) }
+        it "trace exist" do
+          expect(trace.exist?).to be(true)
+        end
+
+        it "can be erased" do
+          trace.erase!
+          expect(trace.exist?).to be(false)
+        end
+      end
+
+      context 'without valid ci_id' do
+        it "does not return deprecated path" do
+          expect(path).to be_nil
+        end
       end
     end
-  end
 
-  describe '#trace_file_path' do
-    context 'when trace is stored in file' do
+    context 'stored in database' do
       before do
-        allow(build).to receive(:has_trace_file?).and_return(true)
-        allow(build).to receive(:has_old_trace_file?).and_return(false)
+        build.send(:write_attribute, :trace, "data")
       end
 
-      it { expect(build.trace_file_path).to eq(build.path_to_trace) }
-    end
+      it "trace exist" do
+        expect(trace.exist?).to be(true)
+      end
 
-    context 'when trace is stored in old file' do
-      before do
-        allow(build).to receive(:has_trace_file?).and_return(true)
-        allow(build).to receive(:has_old_trace_file?).and_return(true)
+      it "can be erased" do
+        trace.erase!
+        expect(trace.exist?).to be(false)
       end
 
-      it { expect(build.trace_file_path).to eq(build.old_path_to_trace) }
+      it "returns database data" do
+        expect(trace.raw).to eq("data")
+      end
     end
   end
 end