diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock index 47237fd7..c916e87e 100644 --- a/ruby/Gemfile.lock +++ b/ruby/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - ci-queue (0.88.0) + ci-queue (0.89.0) logger GEM diff --git a/ruby/lib/ci/queue/queue_entry.rb b/ruby/lib/ci/queue/queue_entry.rb index 137e6691..3572b2f4 100644 --- a/ruby/lib/ci/queue/queue_entry.rb +++ b/ruby/lib/ci/queue/queue_entry.rb @@ -17,7 +17,9 @@ def self.parse(entry) end def self.format(test_id, file_path) - JSON.dump({ test_id: test_id, file_path: file_path }) + raise ArgumentError, "file_path is required for '#{test_id}' — the test file path must be resolvable" if file_path.nil? || file_path.empty? + canonical = load_error_payload?(file_path) ? file_path : ::File.expand_path(file_path) + JSON.dump({ test_id: test_id, file_path: canonical }) end def self.load_error_payload?(file_path) diff --git a/ruby/lib/ci/queue/redis/build_record.rb b/ruby/lib/ci/queue/redis/build_record.rb index 870a7f89..7ea6174f 100644 --- a/ruby/lib/ci/queue/redis/build_record.rb +++ b/ruby/lib/ci/queue/redis/build_record.rb @@ -145,6 +145,10 @@ def error_reports redis.hgetall(key('error-reports')).transform_keys { |entry| CI::Queue::QueueEntry.test_id(entry) } end + def failed_test_entries + redis.hkeys(key('error-reports')) + end + def flaky_reports redis.smembers(key('flaky-reports')).map { |entry| CI::Queue::QueueEntry.test_id(entry) } end @@ -177,6 +181,16 @@ def reset_stats(stat_names) pipeline.hdel(key(stat_name), config.worker_id) end end + # Purge any error-report-deltas that reference this worker so that + # apply_error_report_delta_correction cannot double-subtract from + # the now-zeroed counters on a subsequent successful retry. + deltas = redis.hgetall(key('error-report-deltas')) + to_delete = deltas.filter_map do |entry, delta_json| + entry if JSON.parse(delta_json)['worker_id'].to_s == config.worker_id.to_s + rescue JSON::ParserError + nil + end + redis.hdel(key('error-report-deltas'), *to_delete) unless to_delete.empty? end private diff --git a/ruby/lib/ci/queue/redis/retry.rb b/ruby/lib/ci/queue/redis/retry.rb index be79e26c..ef5bc65b 100644 --- a/ruby/lib/ci/queue/redis/retry.rb +++ b/ruby/lib/ci/queue/redis/retry.rb @@ -28,9 +28,24 @@ def stream_populate(tests, random: nil, batch_size: nil) self end + # Queue a Redis SADD so that BuildRecord#record_success can include this + # in its multi-exec transaction. Without this, Static#acknowledge returns + # a Ruby value (not a Redis future), shifting the result indices and + # breaking the stats delta correction. + def acknowledge(entry, error: nil, pipeline: redis) + @progress += 1 + return @progress unless pipeline + test_id = CI::Queue::QueueEntry.test_id(entry) + pipeline.sadd(key('processed'), test_id) + end + private attr_reader :redis + + def key(*args) + ['build', config.build_id, *args].join(':') + end end end end diff --git a/ruby/lib/ci/queue/redis/worker.rb b/ruby/lib/ci/queue/redis/worker.rb index 8470eab1..b3fd6daa 100644 --- a/ruby/lib/ci/queue/redis/worker.rb +++ b/ruby/lib/ci/queue/redis/worker.rb @@ -158,6 +158,15 @@ def retry_queue log.select! { |entry| failures.include?(CI::Queue::QueueEntry.test_id(entry)) } log.uniq! { |entry| CI::Queue::QueueEntry.test_id(entry) } log.reverse! + + if log.empty? + # Per-worker log has no matching failures — this worker didn't run + # the failing tests (e.g. Buildkite rebuild with new worker IDs, + # or a different parallel slot). Fall back to ALL unresolved + # failures from error-reports so any worker can retry them. + log = redis.hkeys(key('error-reports')) + end + Retry.new(log, config, redis: redis) end diff --git a/ruby/lib/ci/queue/version.rb b/ruby/lib/ci/queue/version.rb index 1a6afbee..e71afe8c 100644 --- a/ruby/lib/ci/queue/version.rb +++ b/ruby/lib/ci/queue/version.rb @@ -2,7 +2,7 @@ module CI module Queue - VERSION = '0.88.0' + VERSION = '0.89.0' DEV_SCRIPTS_ROOT = ::File.expand_path('../../../../../redis', __FILE__) RELEASE_SCRIPTS_ROOT = ::File.expand_path('../redis', __FILE__) end diff --git a/ruby/lib/minitest/queue.rb b/ruby/lib/minitest/queue.rb index bcbcfc95..5bb3755b 100644 --- a/ruby/lib/minitest/queue.rb +++ b/ruby/lib/minitest/queue.rb @@ -330,7 +330,16 @@ def id end def queue_entry - @queue_entry ||= CI::Queue::QueueEntry.format(id, nil) + @queue_entry ||= begin + unless runnable.is_a?(Module) + raise ArgumentError, "runnable must be a Module (got #{runnable.class}). " \ + "Do not create SingleExample with string class names." + end + file_path = runnable.instance_method(method_name).source_location&.first + raise ArgumentError, "Cannot resolve source file for #{id} — " \ + "ensure the test method is defined in a Ruby source file" if file_path.nil? + CI::Queue::QueueEntry.format(id, file_path) + end end def <=>(other) diff --git a/ruby/lib/rspec/queue.rb b/ruby/lib/rspec/queue.rb index bc0c9bc8..0828a9db 100644 --- a/ruby/lib/rspec/queue.rb +++ b/ruby/lib/rspec/queue.rb @@ -254,7 +254,10 @@ def id end def queue_entry - @queue_entry ||= CI::Queue::QueueEntry.format(id, nil) + @queue_entry ||= begin + file_path = example.metadata[:absolute_file_path] || example.file_path + CI::Queue::QueueEntry.format(id, file_path) + end end def <=>(other) diff --git a/ruby/lib/rspec/queue/build_status_recorder.rb b/ruby/lib/rspec/queue/build_status_recorder.rb index 3050fe28..1cbe2e80 100644 --- a/ruby/lib/rspec/queue/build_status_recorder.rb +++ b/ruby/lib/rspec/queue/build_status_recorder.rb @@ -18,13 +18,13 @@ def initialize(*) def example_passed(notification) example = notification.example - entry = CI::Queue::QueueEntry.format(example.id, nil) + entry = CI::Queue::QueueEntry.format(example.id, example.file_path) build.record_success(entry) end def example_failed(notification) example = notification.example - entry = CI::Queue::QueueEntry.format(example.id, nil) + entry = CI::Queue::QueueEntry.format(example.id, example.file_path) build.record_error(entry, dump(notification)) end diff --git a/ruby/test/ci/queue/queue_entry_test.rb b/ruby/test/ci/queue/queue_entry_test.rb index 7a9ef3e2..f6ccb294 100644 --- a/ruby/test/ci/queue/queue_entry_test.rb +++ b/ruby/test/ci/queue/queue_entry_test.rb @@ -2,11 +2,9 @@ require 'test_helper' class CI::Queue::QueueEntryTest < Minitest::Test - def test_parse_without_file_path - entry = CI::Queue::QueueEntry.format("FooTest#test_bar", nil) - parsed = CI::Queue::QueueEntry.parse(entry) - assert_equal "FooTest#test_bar", parsed[:test_id] - assert_nil parsed[:file_path] + def test_format_raises_without_file_path + assert_raises(ArgumentError) { CI::Queue::QueueEntry.format("FooTest#test_bar", nil) } + assert_raises(ArgumentError) { CI::Queue::QueueEntry.format("FooTest#test_bar", "") } end def test_parse_with_file_path @@ -16,18 +14,6 @@ def test_parse_with_file_path assert_equal "/tmp/foo_test.rb", parsed[:file_path] end - def test_format_without_file_path - entry_nil = CI::Queue::QueueEntry.format("FooTest#test_bar", nil) - parsed_nil = JSON.parse(entry_nil, symbolize_names: true) - assert_equal "FooTest#test_bar", parsed_nil[:test_id] - assert_nil parsed_nil[:file_path] - - entry_empty = CI::Queue::QueueEntry.format("FooTest#test_bar", "") - parsed_empty = JSON.parse(entry_empty, symbolize_names: true) - assert_equal "FooTest#test_bar", parsed_empty[:test_id] - assert_equal "", parsed_empty[:file_path] - end - def test_format_with_file_path entry = CI::Queue::QueueEntry.format("FooTest#test_bar", "/tmp/foo_test.rb") parsed = JSON.parse(entry, symbolize_names: true) @@ -60,11 +46,6 @@ def test_round_trip_preserves_test_id assert_equal file_path, parsed[:file_path] end - def test_test_id_without_file_path - entry = CI::Queue::QueueEntry.format("FooTest#test_bar", nil) - assert_equal "FooTest#test_bar", CI::Queue::QueueEntry.test_id(entry) - end - def test_test_id_with_file_path entry = CI::Queue::QueueEntry.format("FooTest#test_bar", "/tmp/foo_test.rb") assert_equal "FooTest#test_bar", CI::Queue::QueueEntry.test_id(entry) diff --git a/ruby/test/integration/minitest_redis_test.rb b/ruby/test/integration/minitest_redis_test.rb index c0b691ac..6d95ea93 100644 --- a/ruby/test/integration/minitest_redis_test.rb +++ b/ruby/test/integration/minitest_redis_test.rb @@ -689,7 +689,7 @@ def test_automatic_retry_reruns_failed_tests_so_report_can_succeed '--queue', @redis_url, '--seed', 'foobar', '--build', '1', - '--worker', '1', + '--worker', '2', '--timeout', '1', '-Itest', 'test/flaky_test.rb', @@ -698,7 +698,7 @@ def test_automatic_retry_reruns_failed_tests_so_report_can_succeed end assert_empty filter_deprecation_warnings(err) # After the fix, the automatic retry should re-run the failed test and report success. - # Currently this is "All tests were ran already" (the bug: test was NOT re-run). + # Worker 2 has no per-worker log — must fall back to error-reports. assert_match(/Retrying failed tests/, out) # The report step runs after all workers complete. After the fix, the failed test @@ -743,6 +743,102 @@ def test_automatic_retry_report_still_fails_when_test_keeps_failing out, err = capture_subprocess_io do system( { 'BUILDKITE_RETRY_COUNT' => '1', 'BUILDKITE_RETRY_TYPE' => 'automatic' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '2', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + + # Report must still fail — the test failed on retry too + out, err = capture_subprocess_io do + system( + @exe, 'report', + '--queue', @redis_url, + '--build', '1', + '--timeout', '1', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + # Test failed on original run AND on retry — aggregate shows multiple failures. + # The key invariant: error-reports are non-empty and the report still fails. + assert_match(/FlakyTest#test_flaky/, out) + refute_match(/0 failures/, normalize(out)) + end + + def test_rebuild_retries_failed_tests_from_different_worker + # Simulates a Buildkite rebuild: worker 1 runs and fails a test, then + # a DIFFERENT worker (worker 2) is spawned in the rebuild with + # BUILDKITE_RETRY_COUNT=1, BUILDKITE_RETRY_TYPE=manual. + # Worker 2 has an empty per-worker log — it must fall back to + # error-reports to find the failed test and retry it. + + # First run: worker 1 runs flaky_test.rb, test_flaky fails + out, err = capture_subprocess_io do + system( + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '1', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + output = normalize(out.lines.last.strip) + assert_equal 'Ran 2 tests, 2 assertions, 1 failures, 0 errors, 0 skips, 0 requeues in X.XXs', output + + # Rebuild: DIFFERENT worker (--worker 2) retries with manual retry env vars. + # Worker 2 has no per-worker log for this build — retry_queue must fall back + # to error-reports to find the failed test. + out, err = capture_subprocess_io do + system( + { 'BUILDKITE_RETRY_COUNT' => '1', 'BUILDKITE_RETRY_TYPE' => 'manual', 'FLAKY_TEST_PASS' => '1' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '2', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + assert_match(/Retrying failed tests/, out) + + # Report should show 0 failures — the test passed on retry + out, err = capture_subprocess_io do + system( + @exe, 'report', + '--queue', @redis_url, + '--build', '1', + '--timeout', '1', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + assert_match(/0 failures/, normalize(out)) + end + + def test_rebuild_report_still_fails_when_test_keeps_failing + # Inverse: different worker retries the failing test but it still fails. + # Report must still show the failure. + + # First run: worker 1 runs flaky_test.rb, test_flaky fails + out, err = capture_subprocess_io do + system( @exe, 'run', '--queue', @redis_url, '--seed', 'foobar', @@ -755,6 +851,25 @@ def test_automatic_retry_report_still_fails_when_test_keeps_failing ) end assert_empty filter_deprecation_warnings(err) + output = normalize(out.lines.last.strip) + assert_equal 'Ran 2 tests, 2 assertions, 1 failures, 0 errors, 0 skips, 0 requeues in X.XXs', output + + # Rebuild: different worker, FLAKY_TEST_PASS NOT set → test fails again + out, err = capture_subprocess_io do + system( + { 'BUILDKITE_RETRY_COUNT' => '1', 'BUILDKITE_RETRY_TYPE' => 'manual' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '2', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) # Report must still fail — the test failed on retry too out, err = capture_subprocess_io do @@ -767,7 +882,99 @@ def test_automatic_retry_report_still_fails_when_test_keeps_failing ) end assert_empty filter_deprecation_warnings(err) - assert_match(/1 failures/, normalize(out)) + assert_match(/FlakyTest#test_flaky/, out) + refute_match(/0 failures/, normalize(out)) + end + + def test_same_worker_manual_retry_reruns_failed_tests + # Same worker retries (per-worker log exists): the fallback to error-reports + # should NOT be needed — the per-worker log intersection finds the failure directly. + + # First run: worker 1 fails test_flaky + out, err = capture_subprocess_io do + system( + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '1', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + output = normalize(out.lines.last.strip) + assert_equal 'Ran 2 tests, 2 assertions, 1 failures, 0 errors, 0 skips, 0 requeues in X.XXs', output + + # Same worker retries — per-worker log should yield the failed test + out, err = capture_subprocess_io do + system( + { 'BUILDKITE_RETRY_COUNT' => '1', 'BUILDKITE_RETRY_TYPE' => 'manual', 'FLAKY_TEST_PASS' => '1' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '1', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + assert_match(/Retrying failed tests/, out) + + out, err = capture_subprocess_io do + system( + @exe, 'report', + '--queue', @redis_url, + '--build', '1', + '--timeout', '1', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + assert_match(/0 failures/, normalize(out)) + end + + def test_rebuild_different_worker_with_no_failures_exits_cleanly + # Different worker retries but there's nothing in error-reports (no failures). + # Both per-worker log AND fallback yield empty → should exit cleanly. + + # First run: worker 1, all tests pass (FLAKY_TEST_PASS=1 so no failures) + out, err = capture_subprocess_io do + system( + { 'FLAKY_TEST_PASS' => '1' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '1', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + # Worker 2 retries with empty per-worker log AND empty error-reports + out, err = capture_subprocess_io do + system( + { 'BUILDKITE_RETRY_COUNT' => '1', 'BUILDKITE_RETRY_TYPE' => 'manual', 'FLAKY_TEST_PASS' => '1' }, + @exe, 'run', + '--queue', @redis_url, + '--seed', 'foobar', + '--build', '1', + '--worker', '2', + '--timeout', '1', + '-Itest', + 'test/flaky_test.rb', + chdir: 'test/fixtures/', + ) + end + assert_empty filter_deprecation_warnings(err) + assert_match(/All tests were ran already/, out) end def test_retry_fails_when_test_run_is_expired @@ -858,8 +1065,8 @@ def test_retry_report error_reports = queue.build.error_reports assert_equal 100, error_reports.size - error_reports.keys.each_with_index do |test_id, index| - entry = CI::Queue::QueueEntry.format(test_id, nil) + queue.build.failed_test_entries.each_with_index do |entry, index| + test_id = CI::Queue::QueueEntry.test_id(entry) queue.instance_variable_set(:@reserved_tests, Concurrent::Set.new([test_id])) reserved_entries = queue.instance_variable_get(:@reserved_entries) || Concurrent::Map.new reserved_entries[test_id] = entry diff --git a/ruby/test/minitest/queue/build_status_recorder_test.rb b/ruby/test/minitest/queue/build_status_recorder_test.rb index 1c9788c6..dbfa9d1a 100644 --- a/ruby/test/minitest/queue/build_status_recorder_test.rb +++ b/ruby/test/minitest/queue/build_status_recorder_test.rb @@ -3,6 +3,17 @@ require 'concurrent/set' module Minitest::Queue + # Lightweight stand-in for a test object in unit tests that don't run real tests. + # Holds test_id and file_path directly so no source_location lookup is needed. + FakeEntry = Struct.new(:id, :queue_entry, :method_name) + + def self.fake_entry(method_name) + test_id = "Minitest::Test##{method_name}" + # Use the same file_path as ReporterTestHelper#result so entries match across reserve/record calls + file_path = "#{Minitest::Queue.project_root}/test/my_test.rb" + FakeEntry.new(test_id, CI::Queue::QueueEntry.format(test_id, file_path), method_name) + end + class BuildStatusRecorderTest < Minitest::Test include ReporterTestHelper @@ -152,9 +163,9 @@ 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") - entry_a = CI::Queue::QueueEntry.format("Minitest::Test#a", nil) + entry_a = Minitest::Queue.fake_entry("a").queue_entry assert_equal true, @queue.build.record_success(entry_a) - entry_b = CI::Queue::QueueEntry.format("Minitest::Test#b", nil) + entry_b = Minitest::Queue.fake_entry("b").queue_entry assert_equal true, @queue.build.record_requeue(entry_b) second_queue = worker(2) @@ -166,7 +177,7 @@ 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 - entry = CI::Queue::QueueEntry.format("test_example", nil) + entry = CI::Queue::QueueEntry.format("test_example", __FILE__) assert_equal true, build.record_success(entry) assert_equal true, build.record_requeue(entry) assert_equal true, build.record_error(entry, "payload") @@ -175,11 +186,10 @@ def test_static_build_record_returns_true private def reserve(queue, method_name) - test_id = Minitest::Queue::SingleExample.new("Minitest::Test", method_name).id - entry = CI::Queue::QueueEntry.format(test_id, nil) - queue.instance_variable_set(:@reserved_tests, Concurrent::Set.new([test_id])) + entry = Minitest::Queue.fake_entry(method_name) + queue.instance_variable_set(:@reserved_tests, Concurrent::Set.new([entry.id])) reserved_entries = queue.instance_variable_get(:@reserved_entries) || Concurrent::Map.new - reserved_entries[test_id] = entry + reserved_entries[entry.id] = entry.queue_entry queue.instance_variable_set(:@reserved_entries, reserved_entries) end @@ -193,9 +203,7 @@ def worker(id) worker_id: id.to_s, timeout: 0.2, ), - ).populate([ - Minitest::Queue::SingleExample.new("Minitest::Test", "a") - ]) + ).populate([Minitest::Queue.fake_entry("a")]) end result end diff --git a/ruby/test/minitest/queue/build_status_reporter_test.rb b/ruby/test/minitest/queue/build_status_reporter_test.rb index db2f9059..b5f14a32 100644 --- a/ruby/test/minitest/queue/build_status_reporter_test.rb +++ b/ruby/test/minitest/queue/build_status_reporter_test.rb @@ -64,9 +64,7 @@ def worker(id) worker_id: id.to_s, timeout: 0.2, ), - ).populate([ - Minitest::Queue::SingleExample.new("Minitest::Test", "a") - ]) + ).populate([Minitest::Queue.fake_entry("a")]) end result end diff --git a/ruby/test/support/reporter_test_helper.rb b/ruby/test/support/reporter_test_helper.rb index e7840e1c..8cd68afd 100644 --- a/ruby/test/support/reporter_test_helper.rb +++ b/ruby/test/support/reporter_test_helper.rb @@ -5,7 +5,7 @@ module ReporterTestHelper def result(name, **kwargs) result = Minitest::Result.from(runnable(name, **kwargs)) result.source_location = ["#{Minitest::Queue.project_root}/test/my_test.rb", 12] - result.queue_entry = CI::Queue::QueueEntry.format("#{result.klass}##{result.name}", nil) if result.respond_to?(:queue_entry=) + result.queue_entry = CI::Queue::QueueEntry.format("#{result.klass}##{result.name}", "#{Minitest::Queue.project_root}/test/my_test.rb") if result.respond_to?(:queue_entry=) result end diff --git a/ruby/test/support/shared_test_cases.rb b/ruby/test/support/shared_test_cases.rb index 9bdfc9e3..1cb566ab 100644 --- a/ruby/test/support/shared_test_cases.rb +++ b/ruby/test/support/shared_test_cases.rb @@ -16,7 +16,7 @@ def id end def queue_entry - CI::Queue::QueueEntry.format(id, nil) + CI::Queue::QueueEntry.format(id, __FILE__) end def to_s