Skip to content

Commit c04081f

Browse files
committed
Fix resolve_entry returning raw String for non-leader workers
PR #375 introduced a regression where resolve_entry falls through to returning a raw String when neither @index nor entry_resolver is set. This happens for every non-leader worker in eager mode, crashing downstream callers with NoMethodError on queue_entry. Two-part fix: - Call configure_lazy_queue in the eager mode path so all workers get an entry_resolver fallback - Add UnresolvedEntry Struct as defense-in-depth so resolve_entry never returns a bare String
1 parent 97a3fb3 commit c04081f

5 files changed

Lines changed: 42 additions & 2 deletions

File tree

ruby/ci-queue.gemspec

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ Gem::Specification.new do |spec|
4141
spec.add_development_dependency 'simplecov', '~> 0.12'
4242
spec.add_development_dependency 'minitest-reporters', '~> 1.1'
4343

44+
spec.add_development_dependency 'rexml'
4445
spec.add_development_dependency 'snappy'
4546
spec.add_development_dependency 'msgpack'
4647
spec.add_development_dependency 'benchmark'
47-
spec.add_development_dependency 'rexml'
4848
spec.add_development_dependency 'rubocop'
4949
end

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ class << self
1313
self.requeue_offset = 42
1414
self.max_sleep_time = 2
1515

16+
# Minimal wrapper returned by resolve_entry when neither @index nor entry_resolver
17+
# is available. Provides the interface callers expect (.id, .queue_entry) so that
18+
# downstream code doesn't crash with NoMethodError on a raw String.
19+
UnresolvedEntry = Struct.new(:id, :queue_entry)
20+
1621
class Worker < Base
1722
attr_accessor :entry_resolver
1823
attr_reader :first_reserve_at
@@ -295,7 +300,7 @@ def resolve_entry(entry)
295300

296301
return entry_resolver.call(entry) if entry_resolver
297302

298-
entry
303+
UnresolvedEntry.new(test_id, entry)
299304
end
300305

301306
def still_streaming?

ruby/lib/minitest/queue/queue_population_strategy.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ def populate_queue
3535
configure_lazy_queue
3636
queue.stream_populate(lazy_test_enumerator, random: ordering_seed, batch_size: queue_config.lazy_load_stream_batch_size)
3737
else
38+
configure_lazy_queue
3839
queue.populate(Minitest.loaded_tests, random: ordering_seed)
3940
end
4041
end

ruby/test/ci/queue/redis_test.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,16 @@ def test_resolve_entry_falls_back_to_resolver
358358
assert_equal "resolved:MissingTest#test_bar#{DELIMITER}/tmp/missing.rb", resolved
359359
end
360360

361+
def test_resolve_entry_returns_unresolved_entry_without_index_or_resolver
362+
queue = worker(1, populate: false)
363+
364+
result = queue.send(:resolve_entry, "MissingTest#test_bar#{DELIMITER}/tmp/missing.rb")
365+
366+
assert_instance_of CI::Queue::Redis::UnresolvedEntry, result
367+
assert_equal "MissingTest#test_bar", result.id
368+
assert_equal "MissingTest#test_bar#{DELIMITER}/tmp/missing.rb", result.queue_entry
369+
end
370+
361371
def test_continuously_timing_out_tests
362372
3.times do
363373
@redis.flushdb

ruby/test/minitest/queue/queue_population_strategy_test.rb

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,30 @@ def test_eager_mode_populates_loaded_tests
5959
Object.send(:remove_const, class_name) if class_name && Object.const_defined?(class_name)
6060
end
6161

62+
def test_eager_mode_sets_entry_resolver_as_fallback
63+
queue = FakeQueue.new
64+
config = CI::Queue::Configuration.new(lazy_load: false)
65+
class_name = "StrategyEagerResolver#{Process.pid}#{rand(1000)}"
66+
67+
Dir.mktmpdir do |dir|
68+
file = File.join(dir, "strategy_eager_resolver_test.rb")
69+
File.write(file, "class #{class_name} < Minitest::Test\n def test_resolver\n assert true\n end\nend\n")
70+
strategy = QueuePopulationStrategy.new(
71+
queue: queue,
72+
queue_config: config,
73+
argv: [file],
74+
test_files_file: nil,
75+
ordering_seed: Random.new(123),
76+
)
77+
strategy.load_and_populate!
78+
end
79+
80+
assert_instance_of Minitest::Queue::LazyEntryResolver, queue.entry_resolver,
81+
"eager mode should set entry_resolver as fallback for resolve_entry"
82+
ensure
83+
Object.send(:remove_const, class_name) if class_name && Object.const_defined?(class_name)
84+
end
85+
6286
def test_preresolved_mode_streams_entries
6387
queue = FakeQueue.new
6488
config = CI::Queue::Configuration.new(lazy_load: true)

0 commit comments

Comments
 (0)