Bug fixes and cleanup

Author: danielahrnsbrak

ruby/lib/ci/queue.rb

@@ -64,8 +64,10 @@ def shuffle(tests, random)
     end
 
     def debug?
+      return @debug if defined?(@debug)
+
       value = ENV['CI_QUEUE_DEBUG']
-      value && !value.strip.empty? && !%w[0 false].include?(value.strip.downcase)
+      @debug = value && !value.strip.empty? && !%w[0 false].include?(value.strip.downcase)
     end
 
     def from_uri(url, config)

ruby/lib/ci/queue/configuration.rb

@@ -7,7 +7,8 @@ class Configuration
       attr_accessor :max_test_duration, :max_test_duration_percentile, :track_test_duration
       attr_accessor :max_test_failed, :redis_ttl, :warnings_file, :debug_log, :max_missed_heartbeat_seconds
       attr_accessor :lazy_load, :lazy_load_stream_batch_size
-      attr_accessor :lazy_load_streaming_timeout, :lazy_load_test_helpers
+      attr_writer :lazy_load_streaming_timeout
+      attr_accessor :lazy_load_test_helpers
       attr_accessor :skip_stale_tests
       attr_reader :circuit_breakers
       attr_writer :seed, :build_id

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

@@ -47,7 +47,7 @@ def stream_populate(tests, random: Random.new, batch_size: 10_000)
 
           if @master = (value == status)
             @total = 0
-            puts "Worker elected as leader, streaming tests to the queue."                          
+            puts "Worker elected as leader, streaming tests to the queue."
 
             duration = measure do
               start_streaming!
@@ -213,6 +213,7 @@ def requeue(test, offset: Redis.requeue_offset)
           unless requeued
             reserved_tests << test_id
             reserved_entries[test_id] = entry
+            reserved_entry_ids[entry] = test_id
           end
           requeued
         end

ruby/lib/minitest/queue.rb

@@ -173,8 +173,8 @@ def run(reporter, *)
             report_load_stats(queue)
           ensure
             store_worker_profile(queue)
+            queue.stop_heartbeat!
           end
-          queue.stop_heartbeat!
         end
       end
 
@@ -433,18 +433,15 @@ def with_timestamps
 
       def run
         with_timestamps do
-          begin
-            return build_error_result(@load_error) if @load_error
-
-            klass = runnable
-            if skip_stale_tests? && !(klass.method_defined?(@method_name) || klass.private_method_defined?(@method_name))
-              return build_stale_skip_result
-            end
-
-            Minitest.run_one_method(klass, @method_name)
-          rescue StandardError, ScriptError => error
-            build_error_result(error)
+          if @load_error
+            build_error_result(@load_error)
+          elsif skip_stale_tests? && !(runnable.method_defined?(@method_name) || runnable.private_method_defined?(@method_name))
+            build_stale_skip_result
+          else
+            Minitest.run_one_method(runnable, @method_name)
           end
+        rescue StandardError, ScriptError => error
+          build_error_result(error)
         end
       end
 

ruby/lib/minitest/queue/lazy_entry_resolver.rb

@@ -26,6 +26,18 @@ def call(entry)
               load_error: load_error,
               queue_entry: entry,
             )
+          else
+            error = StandardError.new("Corrupt load error payload for #{class_name}##{method_name}")
+            load_error = CI::Queue::FileLoadError.new("unknown", error)
+            return Minitest::Queue::LazySingleExample.new(
+              class_name,
+              method_name,
+              nil,
+              loader: @loader,
+              resolver: @resolver,
+              load_error: load_error,
+              queue_entry: entry,
+            )
           end
         end
 

ruby/lib/minitest/queue/lazy_test_discovery.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'set'
+require 'zlib'
 
 module Minitest
   module Queue
@@ -39,7 +40,7 @@ def each_test(files)
           begin
             @loader.load_file(file_path)
           rescue CI::Queue::FileLoadError => error
-            method_name = "load_file_#{file_path.hash.abs}"
+            method_name = "load_file_#{Zlib.crc32(file_path).to_s(16)}"
             class_name = "CIQueue::FileLoadError"
             test_id = "#{class_name}##{method_name}"
             entry = CI::Queue::QueueEntry.format(

ruby/lib/minitest/queue/runner.rb

@@ -55,7 +55,7 @@ def retry_command
       end
 
       def run_command
-        @run_start = Runner.run_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+        Runner.run_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
 
         require_worker_id!
         # if it's an automatic job retry we should process the main queue
@@ -577,7 +577,7 @@ def parser
           end
 
           help = <<~EOS
-            Sepcify a seed used to shuffle the test suite.
+            Specify a seed used to shuffle the test suite.
             On Buildkite, CircleCI, Heroku CI, and Travis, the commit revision will be used by default.
           EOS
           opts.separator ""

ruby/test/minitest/queue/lazy_entry_resolver_test.rb

@@ -16,6 +16,20 @@ def test_builds_lazy_single_example_for_regular_entry
       assert_equal entry, resolved.queue_entry
     end
 
+    def test_builds_error_result_for_corrupt_load_error_payload
+      loader = CI::Queue::FileLoader.new
+      resolver = CI::Queue::ClassResolver
+      corrupt_payload = "__ciq_load_error__:not_valid_base64!!!"
+      entry = CI::Queue::QueueEntry.format("CIQueue::FileLoadError#load_file_abc123", corrupt_payload)
+
+      resolved = LazyEntryResolver.new(loader: loader, resolver: resolver).call(entry)
+      result = resolved.run
+
+      assert_instance_of Minitest::Queue::LazySingleExample, resolved
+      assert result.error?
+      assert_match(/Corrupt load error payload/, result.failure.error.message)
+    end
+
     def test_builds_lazy_single_example_with_load_error
       loader = CI::Queue::FileLoader.new
       resolver = CI::Queue::ClassResolver

ruby/test/minitest/queue/lazy_single_example_test.rb

@@ -114,6 +114,52 @@ def test_run_errors_on_stale_entry_when_skip_stale_tests_disabled
       end
     end
 
+    def test_run_sets_timestamps_on_load_error_result
+      loader = CI::Queue::FileLoader.new
+      resolver = CI::Queue::ClassResolver
+      error = StandardError.new('boom')
+      example = LazySingleExample.new('MissingClass', 'test_missing', '/tmp/missing.rb', loader: loader, resolver: resolver, load_error: error)
+
+      result = example.run
+
+      assert result.error?
+      assert_kind_of Integer, result.start_timestamp
+      assert_kind_of Integer, result.finish_timestamp
+      assert result.finish_timestamp >= result.start_timestamp
+    end
+
+    def test_run_sets_timestamps_on_stale_skip_result
+      Dir.mktmpdir do |dir|
+        class_name = "StaleTimestamp#{Process.pid}#{rand(1000)}"
+        file_path = File.join(dir, "stale_timestamp_test.rb")
+        File.write(
+          file_path,
+          "class #{class_name} < Minitest::Test\n" \
+          "  def test_exists\n" \
+          "    assert true\n" \
+          "  end\n" \
+          "end\n"
+        )
+
+        loader = CI::Queue::FileLoader.new
+        resolver = CI::Queue::ClassResolver
+        example = LazySingleExample.new(class_name, 'test_no_longer_exists', file_path, loader: loader, resolver: resolver)
+
+        old_queue = Minitest.queue
+        Minitest.queue = Struct.new(:config).new(CI::Queue::Configuration.new(skip_stale_tests: true))
+
+        result = example.run
+
+        assert result.skipped?
+        assert_kind_of Integer, result.start_timestamp
+        assert_kind_of Integer, result.finish_timestamp
+        assert result.finish_timestamp >= result.start_timestamp
+      ensure
+        Minitest.queue = old_queue
+        Object.send(:remove_const, class_name) if Object.const_defined?(class_name)
+      end
+    end
+
     def test_marshal_round_trip
       Dir.mktmpdir do |dir|
         class_name = "LazyMarshal#{Process.pid}#{rand(1000)}"

ruby/test/minitest/queue/lazy_test_discovery_test.rb

@@ -1,9 +1,37 @@
 # frozen_string_literal: true
 require 'test_helper'
 require 'minitest/queue/lazy_test_discovery'
+require 'zlib'
 
 module Minitest::Queue
   class LazyTestDiscoveryTest < Minitest::Test
+    def test_load_error_generates_deterministic_test_id
+      loader = CI::Queue::FileLoader.new
+      resolver = CI::Queue::ClassResolver
+      discovered = []
+
+      Dir.mktmpdir do |dir|
+        file_path = File.join(dir, "broken_test.rb")
+        File.write(file_path, "raise 'boom'\n")
+
+        2.times do
+          discovered.clear
+          discovery = LazyTestDiscovery.new(loader: loader, resolver: resolver)
+          discovery.each_test([file_path]) { |test| discovered << test.id }
+        end
+
+        assert_equal 1, discovered.size
+        assert_match(/\ACIQueue::FileLoadError#load_file_[0-9a-f]+\z/, discovered.first)
+      end
+    end
+
+    def test_load_error_test_id_is_stable_across_invocations
+      file_path = "/tmp/stable_hash_test_#{rand(1000)}.rb"
+      method_name_a = "load_file_#{Zlib.crc32(file_path).to_s(16)}"
+      method_name_b = "load_file_#{Zlib.crc32(file_path).to_s(16)}"
+      assert_equal method_name_a, method_name_b
+    end
+
     def test_discovers_methods_added_by_reopened_class
       loader = CI::Queue::FileLoader.new
       resolver = CI::Queue::ClassResolver