Skip to content

test: assert TaskQueueProducer burst keeps newest values#1007

Merged
mairas merged 1 commit into
mainfrom
test/taskqueue-burst-bounded
Jun 16, 2026
Merged

test: assert TaskQueueProducer burst keeps newest values#1007
mairas merged 1 commit into
mainfrom
test/taskqueue-burst-bounded

Conversation

@mairas

@mairas mairas commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Problem

test_task_queue_producer_burst asserted that a no-yield burst of 50 values is delivered losslessly and in order. But SafeQueue is intentionally bounded (default capacity 10) and evicts the oldest on overflow (it logs "Queue full, dropping oldest entry"). So the test's expectation contradicts the queue's documented contract — it's a test bug, not a product bug (confirmed with the maintainer).

Fix

Rewrite the assertion to match the lossy-bounded contract: let the burst complete without draining (the event loop isn't ticked while blocked on the producer's done semaphore), then drain once and assert the survivors are the newest contiguous, in-order suffix ending at the last value pushed. This avoids the producer/consumer race and doesn't hardcode the capacity.

How it was found

On-device execution (test_task_queue_producer_burst — "Expected 50 Was 10"); compile-only CI never ran it. Passes on-device after this change.

SafeQueue is bounded (default 10) and evicts the oldest on overflow, so a
no-yield burst of 50 cannot deliver all 50. Drain only after the burst
completes and assert the survivors are the newest contiguous suffix.
@mairas

mairas commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Automated review

Verdict: LGTM. The rewrite is correct and deterministic; the assertions match SafeQueue's bounded evict-oldest contract and don't hardcode the capacity. Source-traced, no blocking issues.

Correctness (verified)

  • No draining before the burst completes. After xTaskCreate, the main task blocks on xSemaphoreTake(ctx.done, ...) and never ticks the loop while blocked. The producer (tqp_burst_producer_task) gives ctx.done only after the full set(i) loop, and each set is queue_.push. So all 50 pushes complete before the take returns and before the single event_loop->tick(). This holds even under true dual-core parallelism: the producer running on the other core cannot make the take return early, because the semaphore is given only post-burst. The determinism argument is sound on both single-core C3 and dual-core ESP32.
  • Survivors = newest 10 (40..49). Default max_size_=10; push evicts the front when size>=max_size. After 50 no-yield pushes the queue holds 40..49 in FIFO order. The poll_rate=0 producer registers via onTick, and the lambda's while(queue_.pop(...)) drains all survivors in one tick → received_values=[40..49], received_count=10.
  • Assertions hold and are capacity-agnostic. >0, <=kTQPCount, last==kTQPCount-1 (==49), and the suffix received_values[i]==kTQPCount-received_count+i (40+i) all pass and are derived from received_count, so they stay valid if the capacity changes — provided survivors remain a contiguous in-order suffix ending at the last value pushed, which the evict-oldest contract guarantees. The attach-lambda's < kTQPCount guard never trips with only 10 emits.

Nit (non-blocking)

  • The xSemaphoreTake(ctx.done, pdMS_TO_TICKS(2000)) return value is ignored. If the burst ever exceeded 2 s the take would time out and tick() would run mid-burst, breaking determinism. Not a practical risk for 50 trivial pushes, but asserting the take returns pdTRUE would make the precondition explicit and fail loudly rather than flakily if that assumption is ever violated. Optional.

Meta

  • Good catch that compile-only CI never executed this; the original "Expected 50 Was 10" was a genuine test bug against a documented contract, not a product regression. The new test still meaningfully exercises overflow (burst 50 ≫ capacity 10).

@mairas mairas merged commit cc0ecc9 into main Jun 16, 2026
24 checks passed
@mairas mairas deleted the test/taskqueue-burst-bounded branch June 16, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant