Improve specs for build state update and verification

This commit adds more test coverage for cases when a new metric is being
sent by a runner.

lib/gitlab/ci/trace/checksum.rb

@@ -65,7 +65,9 @@ module Gitlab
         end
 
         def state_bytesize
-          strong_memoize(:state_crc32) { build.pending_state&.trace_bytesize }
+          strong_memoize(:state_bytesize) do
+            build.pending_state&.trace_bytesize
+          end
         end
 
         def trace_size
@@ -77,7 +79,8 @@ module Gitlab
         end
 
         def corrupted?
-          return false if valid? || state_bytesize.nil?
+          return false unless has_bytesize?
+          return false if valid?
 
           state_bytesize.to_i != trace_size.to_i
         end
@@ -86,6 +89,10 @@ module Gitlab
           trace_chunks.to_a.size
         end
 
+        def has_bytesize?
+          state_bytesize.present?
+        end
+
         private
 
         def chunk_size(chunk)

spec/lib/gitlab/ci/trace/checksum_spec.rb

@@ -8,8 +8,12 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
   subject { described_class.new(build) }
 
   context 'when build pending state exists' do
+    let(:trace_details) do
+      { trace_checksum: 'crc32:d4777540', trace_bytesize: 262161 }
+    end
+
     before do
-      create(:ci_build_pending_state, build: build, trace_checksum: 'crc32:d4777540')
+      create(:ci_build_pending_state, build: build, **trace_details)
     end
 
     context 'when matching persisted trace chunks exist' do
@@ -22,6 +26,7 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
       it 'calculates combined trace chunks CRC32 correctly' do
         expect(subject.chunks_crc32).to eq 3564598592
         expect(subject).to be_valid
+        expect(subject).not_to be_corrupted
       end
     end
 
@@ -32,8 +37,9 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
         create_chunk(index: 2, data: 'ccccccccccccccccc')
       end
 
-      it 'makes trace checksum invalid' do
+      it 'makes trace checksum invalid but not corrupted' do
         expect(subject).not_to be_valid
+        expect(subject).not_to be_corrupted
       end
     end
 
@@ -43,8 +49,9 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
         create_chunk(index: 2, data: 'ccccccccccccccccc')
       end
 
-      it 'makes trace checksum invalid' do
+      it 'makes trace checksum invalid and corrupted' do
         expect(subject).not_to be_valid
+        expect(subject).to be_corrupted
       end
     end
 
@@ -55,8 +62,9 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
         create_chunk(index: 2, data: 'ccccccccccccccccc')
       end
 
-      it 'makes trace checksum invalid' do
+      it 'makes trace checksum invalid but not corrupted' do
         expect(subject).not_to be_valid
+        expect(subject).not_to be_corrupted
       end
     end
 
@@ -99,6 +107,14 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
       it 'returns nil' do
         expect(subject.last_chunk).to be_nil
       end
+
+      it 'is not a valid trace' do
+        expect(subject).not_to be_valid
+      end
+
+      it 'is not a corrupted trace' do
+        expect(subject).not_to be_corrupted
+      end
     end
 
     context 'when there are multiple chunks' do
@@ -110,6 +126,14 @@ RSpec.describe Gitlab::Ci::Trace::Checksum do
       it 'returns chunk with the highest index' do
         expect(subject.last_chunk.chunk_index).to eq 1
       end
+
+      it 'is not a valid trace' do
+        expect(subject).not_to be_valid
+      end
+
+      it 'is not a corrupted trace' do
+        expect(subject).not_to be_corrupted
+      end
     end
   end
 

spec/services/ci/update_build_state_service_spec.rb

@@ -80,7 +80,11 @@ RSpec.describe Ci::UpdateBuildStateService do
 
   context 'when build has a checksum' do
     let(:params) do
-      { checksum: 'crc32:12345678', state: 'failed', failure_reason: 'script_failure' }
+      {
+        output: { checksum: 'crc32:12345678', bytesize: 123 },
+        failure_reason: 'script_failure',
+        state: 'failed'
+      }
     end
 
     context 'when build does not have associated trace chunks' do
@@ -154,14 +158,74 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
 
       context 'when trace checksum is valid' do
-        let(:params) { { checksum: 'crc32:ed82cd11', state: 'success' } }
+        let(:params) do
+          { output: { checksum: 'crc32:ed82cd11', bytesize: 4 }, state: 'success' }
+        end
 
-        it 'does not increment invalid trace metric' do
+        it 'does not increment invalid or corrupted trace metric' do
           execute_with_stubbed_metrics!
 
           expect(metrics)
             .not_to have_received(:increment_trace_operation)
             .with(operation: :invalid)
+
+          expect(metrics)
+            .not_to have_received(:increment_trace_operation)
+            .with(operation: :corrupted)
+        end
+
+        context 'when using deprecated parameters' do
+          let(:params) do
+            { checksum: 'crc32:ed82cd11', state: 'success' }
+          end
+
+          it 'does not increment invalid or corrupted trace metric' do
+            execute_with_stubbed_metrics!
+
+            expect(metrics)
+              .not_to have_received(:increment_trace_operation)
+              .with(operation: :invalid)
+
+            expect(metrics)
+              .not_to have_received(:increment_trace_operation)
+              .with(operation: :corrupted)
+          end
+        end
+      end
+
+      context 'when trace checksum is invalid and the log is corrupted' do
+        let(:params) do
+          { output: { checksum: 'crc32:12345678', bytesize: 1 }, state: 'success' }
+        end
+
+        it 'increments invalid and corrupted trace metrics' do
+          execute_with_stubbed_metrics!
+
+          expect(metrics)
+            .to have_received(:increment_trace_operation)
+            .with(operation: :invalid)
+
+          expect(metrics)
+            .to have_received(:increment_trace_operation)
+            .with(operation: :corrupted)
+        end
+      end
+
+      context 'when trace checksum is invalid but the log is seems fine' do
+        let(:params) do
+          { output: { checksum: 'crc32:12345678', bytesize: 4 }, state: 'success' }
+        end
+
+        it 'does not increment corrupted trace metric' do
+          execute_with_stubbed_metrics!
+
+          expect(metrics)
+            .to have_received(:increment_trace_operation)
+            .with(operation: :invalid)
+
+          expect(metrics)
+            .not_to have_received(:increment_trace_operation)
+            .with(operation: :corrupted)
         end
       end