test: unit tests for task queue (priority, retry, dead letter) - #452
test: unit tests for task queue (priority, retry, dead letter)#452Kaycee276 wants to merge 6 commits into
Conversation
|
Hi @Kaycee276 👋 — the tests themselves look good (+158, clean diff, Sonar green), but the required CI fails at Fix: rebase your branch onto the latest |
|
Hi @leocagli I've pushed an update that addresses your feedback:
The CI should be completely green now. Let me know if anything else is needed! |
|
Hi @Kaycee276 👋 — quick update: video/screenshots are not required to merge anymore, just a bonus — if you add one it'll be considered for GrantFox rewards on this issue. So I'll review this on code + checks alone now. 🙏 |
Needs a rebase — and that is our fault, not yoursWe just merged a batch of PRs that had been waiting far too long for review. Yours was in that batch and was green, but the merges ahead of it touched overlapping files, so this branch now conflicts with To get it in: git fetch origin
git rebase origin/main
# resolve conflicts, then
git push --force-with-leaseOnce CI goes green again, comment here and it gets merged — no second review round needed for the parts that were already approved. Sorry for the churn. The delay that caused this was on our side: this PR sat for weeks while other work landed on top of it. |
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @@ -17,9 +14,6 @@ | |||
| "secretlint": "secretlint \"**/*\"", | |||
| "size-limit": "size-limit", | |||
| "test": "vitest run", | |||
There was a problem hiding this comment.
⚠️ Bug: package.json reverts recent main deps & removes Playwright E2E
This test-only PR's package.json changes revert changes recently merged to main (introduced by #444/#430): it deletes the engines (node >=22) field, removes the test:e2e/test:e2e:headed/test:e2e:ui scripts and the @playwright/test devDependency, and downgrades framer-motion (^12.42.2→^11.15.0), eslint-config-next (^16.2.10→^16.2.0), and webpack (^5.108.4→^5.97.1). These look like a botched merge-conflict resolution (commit 9fed45e) rather than intentional edits. Merging this would silently roll back the E2E test infrastructure and pin older dependencies. Restore the target-branch versions of these package.json entries and keep only the additions relevant to this PR.
Was this helpful? React with 👍 / 👎
CI failed: CI failures caused by a package-lock sync mismatch during `npm ci` and a permission denied error (`EACCES`) when writing task queue files to the root directory `/.data` during unit tests.OverviewTwo distinct change-related failures occurred across the CI jobs: an FailuresPackage-Lock Sync Mismatch (confidence: high)
Task Queue Permission Denied Error (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
| beforeEach(() => { | ||
| resetTaskQueueForTests() | ||
| jest.useFakeTimers() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers() | ||
| }) |
There was a problem hiding this comment.
⚠️ Bug: New task-queue test writes DLQ to real /.data path
The new test at lib/task-queue/tests/task-queue.test.ts dead-letters tasks (the retry/dead-letter and retryAll cases) but never sets process.env.TASK_DLQ_FILE, so persistDeadLetterQueue() falls back to the production default /.data/task-dlq.json and calls mkdirSync('/.data') + writeFileSync. On a non-root CI runner this throws EACCES and fails the test; elsewhere it creates stray files and can pollute state across runs. The existing suite (tests/api/tasks.test.ts) sets TASK_DLQ_FILE to a temp dir in beforeEach — do the same here (e.g. point it at an os.tmpdir() path in beforeEach and restore/clean up in afterEach).
Was this helpful? React with 👍 / 👎
|
Hola @Kaycee276. No mergeo este PR todavía porque rompe el build, y te paso exactamente qué es y dónde, así lo cerrás rápido. El fallo, en La causa está en return process.env.TASK_DLQ_FILE ?? "/.data/task-dlq.json";Esa ruta por defecto es absoluta a la raíz del disco, no relativa al proyecto. Después, en la línea 150: mkdirSync(dirname(dlqFilePath), { recursive: true });intenta crear literalmente El resto del repo ya resuelve esto de una forma, y conviene seguirla para no tener dos criterios: // lib/webhooks/store.ts:24
const DEFAULT_WEBHOOKS_PATH = join(process.cwd(), ".data", "webhooks.json")
// lib/error-log.ts:22
const DEFAULT_LOG_PATH = join(cwd(), ".data", "error-log.json");
// lib/agents/agent-position-store.ts:75
const DEFAULT_POSITIONS_DIR = join(process.cwd(), ".data", "positions")O sea que el arreglo es agregar el return process.env.TASK_DLQ_FILE ?? join(process.cwd(), ".data", "task-dlq.json");Y una segunda cosa que te recomiendo aunque el arreglo de arriba alcance para que pase el CI. El test no setea beforeEach(() => {
process.env.TASK_DLQ_FILE = join(mkdtempSync(join(tmpdir(), "dlq-")), "task-dlq.json")
})Así los dos tests que fallan quedan aislados de verdad, y no dependen de en qué directorio se los corra ni de lo que haya quedado de la corrida anterior. Para tests de cola con dead-letter eso importa, porque el segundo test ( El resto del PR está bien: Aviso aparte que no tiene que ver con vos: el check Soy Leo Cagliero, del programa Starmaker, trabajando en Cosmos LATAM y Open Stellar. |
2 participants
Title
test: unit tests for task queue (priority, retry, dead letter)
Context
PR #178 introduced a task queue with priority ordering, a dead-letter queue, and retry logic with backoff, but it did not include tests or fully expose the
dequeue(),peekNext(), andretryAll()methods. This PR fulfills the acceptance criteria for issue #186 by implementing the missing methods and writing comprehensive unit tests.Implementation Details
peekNext()tolib/agent-runtime/task-queue.ts: Retrieves the highest-priority pending task without modifying its state.dequeue()tolib/agent-runtime/task-queue.ts: UsespeekNext()to get the next task and transitions its status to"leased".retryAll()tolib/agent-runtime/task-queue.ts: Iterates over the dead-letter queue and callsretryDeadLetterTask()on all failed tasks.lib/task-queue/__tests__/task-queue.test.ts:vi.useFakeTimers()to fast-forward through the N retry attempts and verify the task eventually moves to thedead-letterqueue.retryAll()accurately resets dead-letter tasks back topending.dequeue()on an empty queue safely returnsnull.Requestobjects to test both thePOST /api/tasksandGET /api/tasks/[id]Next.js route handlers directly.Verification
enqueueTask,failTask, or API routes.closes #186