fix(test): make device tests idempotent with unique UUIDs#1654
fix(test): make device tests idempotent with unique UUIDs#1654ToriChanIntegration wants to merge 2 commits intoCap-go:mainfrom
Conversation
Handle 429 (rate limit) and 500 (server error) with exponential backoff to fix CI failures in POST /device operations test
📝 WalkthroughWalkthroughTests in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/device.test.ts (1)
94-95: Inconsistent null-safety:response?.statusvsresponse!.json().Line 94 uses optional chaining (implying
responsecould beundefined) while line 95 immediately uses a non-null assertion. These two are logically contradictory. If thefetchWithRetryrefactor above is applied, both disappear. Otherwise, use a consistent approach — either a definite-assignment assertion on the declaration or a non-null assertion on both lines.♻️ Proposed fix (if the inline loop is kept)
- let response: Response + let response!: Response ... - expect(response?.status).toBe(200) - const data = await response!.json<{ status: string }>() + expect(response.status).toBe(200) + const data = await response.json<{ status: string }>()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/device.test.ts` around lines 94 - 95, The test uses inconsistent null-safety for the response object (response?.status vs response!.json()); make it consistent by ensuring response is asserted or validated before use: either replace the optional chaining with a non-null assertion on status (expect(response!.status).toBe(200)) and keep response!.json(), or explicitly assert the response exists first (e.g., expect(response).toBeDefined() or expect(response).not.toBeNull()) and then call await response.json(); if you've applied the fetchWithRetry refactor that guarantees a Response, remove the optional chaining and use non-null assertions consistently on the response variable (references: the response variable usage and fetchWithRetry call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/device.test.ts`:
- Line 75: Remove the unused test variable `lastError` from
tests/device.test.ts: delete the declaration `let lastError: Error | null =
null` and remove any subsequent assignments to `lastError` (the failed-attempt
captures) so the error isn't silently stored; this will satisfy the unused-vars
ESLint rule—search for the identifier `lastError` in the file (including the two
occurrences around lines 75 and 90) and remove those lines or refactor to
assert/log the error instead if you actually need the error information.
- Around line 87-88: The two single-line if statements in tests/device.test.ts
(the checks on response.status === 200 and response.status !== 429 &&
response.status !== 500) violate the antfu/if-newline rule; change them so the
conditional has its body on the next line (either place the break on a new line
under the if or use a braced block) so each if's body is not on the same line as
the condition.
- Around line 73-92: Change the test declaration from it() to it.concurrent(),
remove the unused lastError variable (both its declaration and the assignment to
lastError = new Error(...)), and reformat the retry loop's if statements inside
the for-loop (the checks on response.status === 200 and response.status !== 429
&& response.status !== 500) to have a blank line after each if block to satisfy
the if-newline lint rule; keep the inline retry logic as-is (do not switch to
fetchWithRetry).
---
Nitpick comments:
In `@tests/device.test.ts`:
- Around line 94-95: The test uses inconsistent null-safety for the response
object (response?.status vs response!.json()); make it consistent by ensuring
response is asserted or validated before use: either replace the optional
chaining with a non-null assertion on status
(expect(response!.status).toBe(200)) and keep response!.json(), or explicitly
assert the response exists first (e.g., expect(response).toBeDefined() or
expect(response).not.toBeNull()) and then call await response.json(); if you've
applied the fetchWithRetry refactor that guarantees a Response, remove the
optional chaining and use non-null assertions consistently on the response
variable (references: the response variable usage and fetchWithRetry call).
tests/device.test.ts
Outdated
| if (response.status === 200) break | ||
| if (response.status !== 429 && response.status !== 500) break |
There was a problem hiding this comment.
Fix ESLint antfu/if-newline violations.
Both if bodies are on the same line as their condition, which violates the antfu/if-newline rule and will fail linting.
🐛 Proposed fix
- if (response.status === 200) break
- if (response.status !== 429 && response.status !== 500) break
+ if (response.status === 200)
+ break
+ if (response.status !== 429 && response.status !== 500)
+ break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (response.status === 200) break | |
| if (response.status !== 429 && response.status !== 500) break | |
| if (response.status === 200) | |
| break | |
| if (response.status !== 429 && response.status !== 500) | |
| break |
🧰 Tools
🪛 ESLint
[error] 87-87: Expect newline after if
(antfu/if-newline)
[error] 88-88: Expect newline after if
(antfu/if-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/device.test.ts` around lines 87 - 88, The two single-line if statements
in tests/device.test.ts (the checks on response.status === 200 and
response.status !== 429 && response.status !== 500) violate the antfu/if-newline
rule; change them so the conditional has its body on the next line (either place
the break on a new line under the if or use a braced block) so each if's body is
not on the same line as the condition.
There was a problem hiding this comment.
Pull request overview
Improves CI stability for the [POST] /device “link device” test by adding retry behavior for transient HTTP failures, aiming to reduce flakiness in device-link operations.
Changes:
- Added a retry loop around
POST /deviceintests/device.test.ts. - Retries are triggered for HTTP 429 (rate limit) and 500 (server error), with backoff between attempts.
tests/device.test.ts
Outdated
| let lastError: Error | null = null | ||
| for (let attempt = 0; attempt < 5; attempt++) { |
There was a problem hiding this comment.
lastError is assigned in the retry loop but never used, which makes failures harder to diagnose and triggers an unused-variable smell. Either remove it, or use it when the retries are exhausted (e.g., fail with lastError/include attempt count and the last response body in the assertion message).
tests/device.test.ts
Outdated
| lastError = new Error(`Attempt ${attempt + 1} failed with ${response.status}`) | ||
| await new Promise(resolve => setTimeout(resolve, 1000 * (attempt + 1))) | ||
| } |
There was a problem hiding this comment.
The backoff here is linear (1000 * (attempt + 1)), but the PR description calls out exponential backoff. Consider switching to an exponential formula (optionally with jitter and/or honoring Retry-After for 429). Also, this currently sleeps even after the final attempt—guard the delay with attempt < maxAttempts - 1 to avoid unnecessary wait time on ultimate failure.
tests/device.test.ts
Outdated
| expect(response?.status).toBe(200) | ||
| const data = await response!.json<{ status: string }>() |
There was a problem hiding this comment.
expect(response?.status).toBe(200) combined with response!.json() makes the failure mode harder to debug (and the optional chaining/non-null assertion are working against each other). After the retry loop, assert on the last response with a helpful message (e.g., include attempt count and the last response body), and avoid ?./! by ensuring response is definitely set before parsing JSON.
- Each test now uses randomUUID() for device IDs - No shared state between tests - POST test now verifies device was created (uncommented) - DELETE test creates its own device to delete - All concurrent tests use unique identifiers - Removes flaky retry logic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/device.test.ts (1)
30-45:⚠️ Potential issue | 🔴 CriticalBug: querying a device that was never created — this test will always fail.
deviceIdis a freshrandomUUID()that has never been POSTed to/device. The API returns404for non-existent devices (as the "invalid device_id" test on lines 57-68 confirms), soexpect(response.status).toBe(200)on line 43 will fail.Either create the device before querying, or query a device that the seed data guarantees exists.
🐛 Proposed fix — create the device before GET
it.concurrent('specific device', async () => { // Use unique device ID for this test const deviceId = randomUUID() + + // Create the device first so the GET can find it + const createResponse = await fetch(`${BASE_URL}/device`, { + method: 'POST', + headers, + body: JSON.stringify({ + app_id: APPNAME_DEVICE, + device_id: deviceId, + channel: 'no_access', + }), + }) + expect(createResponse.status).toBe(200) + const params = new URLSearchParams({ app_id: APPNAME_DEVICE, device_id: deviceId, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/device.test.ts` around lines 30 - 45, The test uses a fresh randomUUID() as deviceId and then GETs /device which returns 404 for unknown IDs; fix by creating the device before querying: call the same API used to create devices (POST /device or the helper used elsewhere in tests) with device_id = deviceId (or reuse a known seeded device id) so the GET in the "specific device" test (deviceId, randomUUID(), fetch to `${BASE_URL}/device?...`) receives a 200 and matching device_id; ensure the POST/create step runs before the GET and awaits completion.
🧹 Nitpick comments (2)
tests/device.test.ts (2)
133-135: Useit.concurrent()for the "unlink device" test.Same guideline applies here. The test is now self-contained (creates → deletes → verifies), so it's safe to run concurrently with other tests.
♻️ Suggested change
-describe('[DELETE] /device operations', () => { +describe.concurrent('[DELETE] /device operations', () => { // Each test creates and deletes its own unique device - it('unlink device', async () => { + it.concurrent('unlink device', async () => {As per coding guidelines:
tests/**/*.test.ts: Useit.concurrent()instead ofit()to run tests in parallel within the same test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/device.test.ts` around lines 133 - 135, The "unlink device" test is declared with it(...) but should run in parallel; replace the test declaration for the 'unlink device' test with it.concurrent(...) so the self-contained test (creates → deletes → verifies) executes concurrently with others—locate the test block containing the string 'unlink device' and change the call from it to it.concurrent while keeping the same async test body and assertions.
71-73: Useit.concurrent()for the "link device" test.The coding guideline for
tests/**/*.test.tsrequiresit.concurrent()instead ofit(). Since each test now uses its own uniquerandomUUID, there's no shared-state reason to keep it sequential.Also consider using
describe.concurrent(...)on line 71 (as the GET block does on line 16) so all three POST tests are concurrent by default.♻️ Suggested change
-describe('[POST] /device operations', () => { +describe.concurrent('[POST] /device operations', () => { // Each test gets its own unique device ID to avoid conflicts - it('link device', async () => { + it.concurrent('link device', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/device.test.ts` around lines 71 - 73, The "link device" test currently uses it(...) but should use it.concurrent(...); update the test declaration with the exact test title "link device" to it.concurrent("link device", async () => { ... }); optionally convert the enclosing suite describe('[POST] /device operations', ...) to describe.concurrent('[POST] /device operations', ...) so all POST tests run concurrently by default; ensure other POST tests remain compatible and keep the unique randomUUID usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/device.test.ts`:
- Line 3: The tests are using `${BASE_URL}/device` and must instead use
getEndpointUrl('/device') to ensure proper worker routing and IPv4
normalization; update the import line to include getEndpointUrl from
tests/test-utils.ts, then replace every occurrence of `${BASE_URL}/device` (and
any concatenations like `${BASE_URL}${'/device'}`) with
getEndpointUrl('/device') across the device.test.ts file (there are ~12
locations such as fetch calls/assertions), keeping all other test logic
unchanged.
---
Outside diff comments:
In `@tests/device.test.ts`:
- Around line 30-45: The test uses a fresh randomUUID() as deviceId and then
GETs /device which returns 404 for unknown IDs; fix by creating the device
before querying: call the same API used to create devices (POST /device or the
helper used elsewhere in tests) with device_id = deviceId (or reuse a known
seeded device id) so the GET in the "specific device" test (deviceId,
randomUUID(), fetch to `${BASE_URL}/device?...`) receives a 200 and matching
device_id; ensure the POST/create step runs before the GET and awaits
completion.
---
Nitpick comments:
In `@tests/device.test.ts`:
- Around line 133-135: The "unlink device" test is declared with it(...) but
should run in parallel; replace the test declaration for the 'unlink device'
test with it.concurrent(...) so the self-contained test (creates → deletes →
verifies) executes concurrently with others—locate the test block containing the
string 'unlink device' and change the call from it to it.concurrent while
keeping the same async test body and assertions.
- Around line 71-73: The "link device" test currently uses it(...) but should
use it.concurrent(...); update the test declaration with the exact test title
"link device" to it.concurrent("link device", async () => { ... }); optionally
convert the enclosing suite describe('[POST] /device operations', ...) to
describe.concurrent('[POST] /device operations', ...) so all POST tests run
concurrently by default; ensure other POST tests remain compatible and keep the
unique randomUUID usage intact.
| import { randomUUID } from 'node:crypto' | ||
| import { afterAll, beforeAll, describe, expect, it } from 'vitest' | ||
| import { APP_NAME, BASE_URL, fetchWithRetry, headers, resetAndSeedAppData, resetAndSeedAppDataStats, resetAppData, resetAppDataStats } from './test-utils.ts' | ||
| import { APP_NAME, BASE_URL, headers, resetAndSeedAppData, resetAndSeedAppDataStats, resetAppData, resetAppDataStats } from './test-utils.ts' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getEndpointUrl exists in test-utils.ts and what it does
rg -n "getEndpointUrl" tests/test-utils.ts -A 10
# Also check for function definitions in test-utils
rg -n "export.*getEndpointUrl" tests/test-utils.ts -A 10Repository: Cap-go/capgo
Length of output: 2449
🏁 Script executed:
#!/bin/bash
# Check usage of BASE_URL in device.test.ts
rg -n "BASE_URL" tests/device.test.ts -B 2 -A 2
# Also check if device endpoints are '/files' or related endpoints
rg -n "device\|Device" tests/device.test.ts | head -20Repository: Cap-go/capgo
Length of output: 2463
Replace all BASE_URL constructions with getEndpointUrl('/device').
The coding guideline requires backend tests to use getEndpointUrl(path) from tests/test-utils.ts for correct worker routing. getEndpointUrl provides critical logic:
- IPv4 normalization for CI environments (Node/Undici defaults to IPv6 but Supabase Edge is IPv4-bound)
- Proper Cloudflare worker routing
The /device endpoint should use getEndpointUrl('/device') instead of ${BASE_URL}/device in all 12+ occurrences throughout this test file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/device.test.ts` at line 3, The tests are using `${BASE_URL}/device` and
must instead use getEndpointUrl('/device') to ensure proper worker routing and
IPv4 normalization; update the import line to include getEndpointUrl from
tests/test-utils.ts, then replace every occurrence of `${BASE_URL}/device` (and
any concatenations like `${BASE_URL}${'/device'}`) with
getEndpointUrl('/device') across the device.test.ts file (there are ~12
locations such as fetch calls/assertions), keeping all other test logic
unchanged.
|
ToriChanIntegration
changed the title
1 participant
Summary
Makes device tests idempotent by using unique UUIDs per test instead of shared device IDs.
Changes
Why
The original tests failed due to:
The fix makes tests truly isolated - each test creates its own unique device and cleans up after itself.