Refactor: move record_stats out of record_error/success/requeue

- BuildStatusRecorder: record outcome first, then call build.record_stats(stats) only when acknowledged; combine into single if acknowledged block
- Redis BuildRecord: record_error/record_success/record_requeue no longer take stats; add public record_stats(stats, pipeline: nil)
- In-memory BuildRecord: same API change; record_stats public
- GrindRecord/GrindRecorder: same pattern - record_error/record_success without stats, record_stats at end
- Tests: update callers to use new API (no stats in record_*; record_stats called separately where needed)

Author: kangze-jia

ruby/lib/ci/queue/build_record.rb

@@ -18,22 +18,25 @@ def queue_exhausted?
         @queue.exhausted?
       end
 
-      def record_error(id, payload, stats: nil)
+      def record_error(id, payload)
         error_reports[id] = payload
-        record_stats(stats)
         true
       end
 
-      def record_success(id, stats: nil, skip_flaky_record: false, acknowledge: true)
+      def record_success(id, skip_flaky_record: false, acknowledge: true)
         error_reports.delete(id)
-        record_stats(stats)
         true
       end
 
-      def record_requeue(id, stats: nil)
+      def record_requeue(id)
         true
       end
 
+      def record_stats(builds_stats)
+        return unless builds_stats
+        stats.merge!(builds_stats)
+      end
+
       def fetch_stats(stat_names)
         stat_names.zip(stats.values_at(*stat_names).map(&:to_f))
       end
@@ -53,11 +56,6 @@ def worker_errors
       private
 
       attr_reader :stats
-
-      def record_stats(builds_stats)
-        return unless builds_stats
-        stats.merge!(builds_stats)
-      end
     end
   end
 end

ruby/lib/ci/queue/redis/build_record.rb

@@ -56,39 +56,46 @@ def record_warning(type, attributes)
           redis.rpush(key('warnings'), Marshal.dump([type, attributes]))
         end
 
-        def record_error(id, payload, stats: nil)
+        def record_error(id, payload)
           # Run acknowledge first so we know whether we're the first to ack
           acknowledged = @queue.acknowledge(id, error: payload)
 
           if acknowledged
             # We were the first to ack; another worker already ack'd would get falsy from SADD
-            redis.pipelined do |pipeline|
-              record_stats(stats, pipeline: pipeline)
-            end
             @queue.increment_test_failed
           end
           # Return so caller can roll back local counter when not acknowledged
           !!acknowledged
         end
 
-        def record_success(id, stats: nil, skip_flaky_record: false)
-          acknowledged, error_reports_deleted_count, requeued_count, _ = redis.multi do |transaction|
+        def record_success(id, skip_flaky_record: false)
+          acknowledged, error_reports_deleted_count, requeued_count = redis.multi do |transaction|
             @queue.acknowledge(id, pipeline: transaction)
             transaction.hdel(key('error-reports'), id)
             transaction.hget(key('requeues-count'), id)
-            record_stats(stats, pipeline: transaction)
           end
           record_flaky(id) if !skip_flaky_record && (error_reports_deleted_count.to_i > 0 || requeued_count.to_i > 0)
           !!acknowledged
         end
 
-        def record_requeue(id, stats: nil)
-          redis.pipelined do |pipeline|
-            record_stats(stats, pipeline: pipeline)
-          end
+        def record_requeue(id)
           true
         end
 
+        def record_stats(stats, pipeline: nil)
+          return unless stats
+          if pipeline
+            stats.each do |stat_name, stat_value|
+              pipeline.hset(key(stat_name), config.worker_id, stat_value)
+              pipeline.expire(key(stat_name), config.redis_ttl)
+            end
+          else
+            redis.pipelined do |p|
+              record_stats(stats, pipeline: p)
+            end
+          end
+        end
+
         def record_flaky(id, stats: nil)
           redis.pipelined do |pipeline|
             pipeline.sadd?(
@@ -136,14 +143,6 @@ def reset_stats(stat_names)
 
         attr_reader :config, :redis
 
-        def record_stats(stats, pipeline: redis)
-          return unless stats
-          stats.each do |stat_name, stat_value|
-            pipeline.hset(key(stat_name), config.worker_id, stat_value)
-            pipeline.expire(key(stat_name), config.redis_ttl)
-          end
-        end
-
         def key(*args)
           KeyShortener.key(config.build_id, *args)
         end

ruby/lib/ci/queue/redis/grind_record.rb

@@ -10,20 +10,32 @@ def initialize(queue, redis, config)
           @config = config
         end
 
-        def record_error(payload, stats: nil)
+        def record_error(payload)
           redis.pipelined do |pipeline|
             pipeline.lpush(
               key('error-reports'),
               payload,
             )
             pipeline.expire(key('error-reports'), config.redis_ttl)
-            record_stats(stats, pipeline: pipeline)
           end
           nil
         end
 
-        def record_success(stats: nil)
-          record_stats(stats)
+        def record_success
+        end
+
+        def record_stats(stats, pipeline: nil)
+          return unless stats
+          if pipeline
+            stats.each do |stat_name, stat_value|
+              pipeline.hset(key(stat_name), config.worker_id, stat_value)
+              pipeline.expire(key(stat_name), config.redis_ttl)
+            end
+          else
+            redis.pipelined do |p|
+              record_stats(stats, pipeline: p)
+            end
+          end
         end
 
         def record_warning(_,_)
@@ -54,14 +66,6 @@ def pop_warnings
         def key(*args)
           KeyShortener.key(config.build_id, *args)
         end
-
-        def record_stats(stats, pipeline: redis)
-          return unless stats
-          stats.each do |stat_name, stat_value|
-            pipeline.hset(key(stat_name), config.worker_id, stat_value)
-            pipeline.expire(key(stat_name), config.redis_ttl)
-          end
-        end
       end
     end
   end

ruby/lib/minitest/queue/build_status_recorder.rb

@@ -38,14 +38,16 @@ def record(test)
         super
 
         self.total_time = Minitest.clock_time - start_time
-        stats = COUNTERS.zip(COUNTERS.map { |c| send(c) + stat_delta(c, test) }).to_h
+        
+        # Determine what type of result this is and record it
+        test_id = "#{test.klass}##{test.name}"
 
         acknowledged = if (test.failure || test.error?) && !test.skipped?
-          build.record_error("#{test.klass}##{test.name}", dump(test), stats: stats)
+          build.record_error(test_id, dump(test))
         elsif test.requeued?
-          build.record_requeue("#{test.klass}##{test.name}", stats: stats)
+          build.record_requeue(test_id)
         else
-          build.record_success("#{test.klass}##{test.name}", stats: stats, skip_flaky_record: test.skipped?)
+          build.record_success(test_id, skip_flaky_record: test.skipped?)
         end
 
         if acknowledged
@@ -56,21 +58,13 @@ def record(test)
           elsif test.skipped?
             self.skips += 1
           end
+          stats = COUNTERS.zip(COUNTERS.map { |c| send(c) }).to_h
+          build.record_stats(stats)
         end
       end
 
       private
 
-      def stat_delta(counter, test)
-        case counter
-        when 'errors'   then test.error?   && !test.skipped? ? 1 : 0
-        when 'failures' then test.failure  && !test.skipped? ? 1 : 0
-        when 'skips'    then test.skipped? ? 1 : 0
-        when 'requeues' then test.requeued? ? 1 : 0
-        else 0
-        end
-      end
-
       def dump(test)
         ErrorReport.new(self.class.failure_formatter.new(test).to_h).dump
       end

ruby/lib/minitest/queue/grind_recorder.rb

@@ -32,12 +32,12 @@ def record(test)
       private
 
       def record_test(test)
-        stats = self.class.counters
         if (test.failure || test.error?) && !test.skipped?
-          build.record_error(dump(test), stats: stats)
+          build.record_error(dump(test))
         else
-          build.record_success(stats: stats)
+          build.record_success
         end
+        build.record_stats(self.class.counters)
       end
 
       def increment_counter(test)

ruby/test/integration/minitest_redis_test.rb

@@ -513,14 +513,15 @@ def test_retry_report
 
       error_reports.keys.each_with_index do |test_id, index|
         queue.instance_variable_set(:@reserved_tests, Concurrent::Set.new([test_id]))
-        queue.build.record_success(test_id.dup, stats: {
+        queue.build.record_success(test_id.dup)
+        queue.build.record_stats(
           'assertions' => index + 1,
           'errors' => 0,
           'failures' => 0,
           'skips' => 0,
           'requeues' => 0,
           'total_time' => index + 1,
-        })
+        )
       end
 
       # Retry first worker, bailing out

ruby/test/minitest/queue/build_status_recorder_test.rb

@@ -145,21 +145,21 @@ def test_duplicate_success_does_not_increment_skips
     def test_build_record_methods_return_boolean
       # Redis build: first to ack returns true, duplicate returns false
       reserve(@queue, "a")
-      assert_equal true, @queue.build.record_success("Minitest::Test#a", stats: { 'assertions' => 1, 'errors' => 0, 'failures' => 0, 'skips' => 0, 'requeues' => 0, 'total_time' => 0.1 })
-      assert_equal true, @queue.build.record_requeue("Minitest::Test#b", stats: { 'assertions' => 1, 'errors' => 0, 'failures' => 0, 'skips' => 0, 'requeues' => 1, 'total_time' => 0.1 })
+      assert_equal true, @queue.build.record_success("Minitest::Test#a")
+      assert_equal true, @queue.build.record_requeue("Minitest::Test#b")
 
       second_queue = worker(2)
       reserve(second_queue, "a")
-      assert_equal false, second_queue.build.record_success("Minitest::Test#a", stats: { 'assertions' => 1, 'errors' => 0, 'failures' => 0, 'skips' => 0, 'requeues' => 0, 'total_time' => 0.1 })
+      assert_equal false, second_queue.build.record_success("Minitest::Test#a")
     end
 
     def test_static_build_record_returns_true
       static_queue = CI::Queue::Static.new(['test_example'], CI::Queue::Configuration.new(build_id: '42', worker_id: '1'))
       build = static_queue.build
 
-      assert_equal true, build.record_success("test_example", stats: { 'assertions' => 0, 'errors' => 0, 'failures' => 0, 'skips' => 0, 'requeues' => 0, 'total_time' => 0 })
-      assert_equal true, build.record_requeue("test_example", stats: { 'assertions' => 0, 'errors' => 0, 'failures' => 0, 'skips' => 0, 'requeues' => 1, 'total_time' => 0 })
-      assert_equal true, build.record_error("test_example", "payload", stats: {})
+      assert_equal true, build.record_success("test_example")
+      assert_equal true, build.record_requeue("test_example")
+      assert_equal true, build.record_error("test_example", "payload")
     end
 
     private