test(ci): complete the infra-skip preflight sweep across the gated live itests - #63
Conversation
…ve itests
Completes PRD-034's FR-4 / Suggestion S-1: the infra-degraded → NEUTRAL-skip preflight
(neutralizeIfInfraDegraded, tests/integration/_infra-skip.ts) was wired into only 2
demonstration itests (compaction, sources-purge). So a TRANSIENT backend failure
(502 / query_error / timeout / connection — isTransientResult) in any other gated live
itest red the suite instead of neutralizing. On the post-merge run that surfaced as
golden-path f-AC-2: the summary `memory`-row APPEND transiently returned {written:false}
→ a hard red. (It correctly did NOT block main — the push-soft job is continue-on-error —
but it would redden the nightly canary as noise.)
Wire the preflight into the 25 write/read-heavy gated live itests so a confirmed
transient-class failure neutralizes (marker + skip) while a NON-transient failure
(wrong value, broken tenancy, real SQL error, lost write on a healthy backend) still
REDS — the teeth are preserved. For domain ops that return a boolean rather than a
QueryResult (e.g. writeSummary → {written}), failure triggers a lightweight storage
probe (SELECT 1) whose QueryResult is classified, so we only neutralize on confirmed
backend-transient degradation, never on a bare domain false.
teeth-proof-live.itest.ts and deeplake-stress-live.itest.ts are deliberately left
untouched (the first must red on a real regression; the second's weather IS its data).
Verified: typecheck clean; default suite 2276 passed (integration excluded); audit:sql
clean; golden-path live passes on a healthy backend (no false-neutralize); full live
suite green/neutral. Test-only change — no product src, no ci.yaml.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughTwenty-four live integration test files gain an infra-degradation preflight guard. Each file imports ChangesInfra-degradation preflight across live integration tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/recall-eval-live.itest.ts`:
- Line 372: In the test file, replace the `return` statement in the condition
checking `!embedReady` with a call to the `skip()` function with the message
"embed daemon unavailable". This will explicitly mark the test as skipped rather
than silently passing when the embed daemon is unavailable, ensuring
infrastructure issues are properly reflected in CI results.
In `@tests/integration/semantic-recall-live.itest.ts`:
- Line 189: Replace the plain return statement in the embedReady check with a
call to skip() function to properly mark the test as skipped when the embed
daemon is unavailable. The skip parameter is already available in the test
context (from line 188), so call skip with a descriptive message like "embed
daemon unavailable" instead of using return, which will ensure the test is
marked as skipped rather than passed, aligning with the documented behavior in
the file header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40fa8312-465e-42ac-8c83-2c523fa77af8
📒 Files selected for processing (25)
tests/integration/api-keys-live.itest.tstests/integration/capture-sessions-live.itest.tstests/integration/connect-parity-live.itest.tstests/integration/controlled-writes-live.itest.tstests/integration/dashboard-logs-live.itest.tstests/integration/data-api-assembled-live.itest.tstests/integration/deeplake-concurrent-writers-chaos.itest.tstests/integration/deeplake-live.itest.tstests/integration/document-worker-live.itest.tstests/integration/dreaming-consolidation-live.itest.tstests/integration/golden-path-live.itest.tstests/integration/graph-persist-live.itest.tstests/integration/hook-capture-roundtrip-live.itest.tstests/integration/memories-api-live.itest.tstests/integration/memory-jobs-live.itest.tstests/integration/missing-table-heal-live.itest.tstests/integration/product-data-api-live.itest.tstests/integration/read-converge-live.itest.tstests/integration/recall-eval-live.itest.tstests/integration/retention-live.itest.tstests/integration/semantic-recall-live.itest.tstests/integration/skills-write-live.itest.tstests/integration/synthesis-live.itest.tstests/integration/vfs-goal-dispatch-live.itest.tstests/integration/write-readback-noflap-live.itest.ts
| "AC-5: seeds the golden set, runs recall per query, and emits recall@k / MRR / nDCG + a per-query report", | ||
| async () => { | ||
| async ({ skip }) => { | ||
| if (!embedReady) return; // skip cleanly when the embed daemon is unavailable. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the test file
cd tests/integration && wc -l recall-eval-live.itest.tsRepository: legioncodeinc/honeycomb
Length of output: 97
🏁 Script executed:
# Read the context around line 372 to understand the test structure
sed -n '360,385p' tests/integration/recall-eval-live.itest.tsRepository: legioncodeinc/honeycomb
Length of output: 1284
🏁 Script executed:
# Look at the beginning of the file to identify the test framework and imports
head -50 tests/integration/recall-eval-live.itest.tsRepository: legioncodeinc/honeycomb
Length of output: 3172
🏁 Script executed:
# Check if skip is used elsewhere in this test file or similar files
rg -n "skip\(" tests/integration/recall-eval-live.itest.tsRepository: legioncodeinc/honeycomb
Length of output: 49
🏁 Script executed:
# Look for similar patterns in other test files to see how skip is used
rg -n "skip\(" tests/integration/ -A 1 | head -20Repository: legioncodeinc/honeycomb
Length of output: 546
🌐 Web query:
vitest skip function test behavior return vs skip
💡 Result:
In Vitest, using the context-provided skip() function and returning early from a test function have fundamentally different behaviors [1]. 1. Using skip() from the Test Context When you call skip() (accessible via the test context object), Vitest explicitly marks the test as "skipped" [2][3]. This ensures the test is reported as skipped in your test results, providing clear visibility that the test was intentionally bypassed rather than just exiting [1]. The skip() function works by throwing a internal error (a PendingError) that terminates the test execution path, which is why its type is defined as never [4][5]. Because it stops execution immediately, it is safe to use anywhere within your test function [3]. 2. Returning Early (return) If you simply return from a test function early, Vitest treats the test as having "passed" (assuming it reached the end of the function without failure) [1]. If the test function had no assertions and finished by returning, it may still be marked as a PASS, potentially creating a "silent pass" that masks untested code [1]. This is often undesirable for quality gates, as it gives a false sense of security that the test completed its work [1]. Recommendation To avoid silent passes, you should avoid using early return to bypass test logic [1]. If a condition requires bypassing a test, you should explicitly use the context-provided skip() function or mark the test as failed/errored [1]. To further protect your test suite from silent passes, consider setting passWithNoAssertions: false in your Vitest configuration [1]. Summary Table Behavior | skip() function | return statement --- | --- | --- Test Result | Marked as "skipped" | Marked as "passed" [1] Visibility | Clearly visible in reports [6] | Hidden; may look like a success [1] Execution | Terminates test immediately [4][5] | Exits function normally [1] Intent | Explicitly bypasses test [3] | Implicitly exits (can lead to silent passes) [1] Top results: [2][1][3][7]
Citations:
- 1: https://zenn.dev/saytooy_arch/articles/10-vitest-silent-pass?locale=en
- 2: https://github.com/vitest-dev/vitest/blob/main/docs/guide/test-context.md
- 3: https://vitest.dev/guide/test-context
- 4: fix: typings of
ctx.skip()asnevervitest-dev/vitest#7608 - 5: How to skip a test if a particular error is thrown? vitest-dev/vitest#4318
- 6: https://vitest.dev/guide/filtering
- 7: https://vitest.dev/api/test
Use skip(...) instead of return when embed daemon is unavailable.
Currently, return marks the test as passed (silent pass), which makes infrastructure unavailability appear green in CI. Use the available skip parameter to explicitly mark the test as skipped: skip("embed daemon unavailable").
Proposed fix
- if (!embedReady) return; // skip cleanly when the embed daemon is unavailable.
+ if (!embedReady) skip("embed daemon unavailable");📝 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 (!embedReady) return; // skip cleanly when the embed daemon is unavailable. | |
| if (!embedReady) skip("embed daemon unavailable"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/recall-eval-live.itest.ts` at line 372, In the test file,
replace the `return` statement in the condition checking `!embedReady` with a
call to the `skip()` function with the message "embed daemon unavailable". This
will explicitly mark the test as skipped rather than silently passing when the
embed daemon is unavailable, ensuring infrastructure issues are properly
reflected in CI results.
| "AC-2: a stored memory lands a non-NULL 768-dim content_embedding (poll-convergent read-back)", | ||
| async () => { | ||
| async ({ skip }) => { | ||
| if (!embedReady) return; // skip cleanly when the embed daemon is unavailable. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/integration/semantic-recall-live.itest.ts | head -50Repository: legioncodeinc/honeycomb
Length of output: 3629
🏁 Script executed:
cat -n tests/integration/semantic-recall-live.itest.ts | sed -n '180,200p'Repository: legioncodeinc/honeycomb
Length of output: 1189
🏁 Script executed:
rg "import.*skip|from.*test" tests/integration/semantic-recall-live.itest.ts -A 2 -B 2Repository: legioncodeinc/honeycomb
Length of output: 682
🏁 Script executed:
rg "skip\(" tests/integration/ -A 1 -B 1 | head -30Repository: legioncodeinc/honeycomb
Length of output: 714
🌐 Web query:
vitest skip function syntax usage test context parameter
💡 Result:
In Vitest, there are two primary ways to skip tests: using the test function itself or using the test context [1][2]. 1. Using the Test Function (Static) You can skip tests at the time of definition by appending .skip to the test function, or by passing an options object [2][3]. test.skip('skipped test', => { // This test will be skipped }); // Or using options test('skipped test', { skip: true }, => { // This test will be skipped }); 2. Using the Test Context (Dynamic) You can skip a test dynamically from within the test body by accessing the skip function provided via the test context [1][4]. test('my test', ({ skip }) => { if (someCondition) { skip; // Skips the test execution } // Logic continues if condition is false }); Conditional Skipping (Vitest 3.1+) Starting with Vitest 3.1, the skip function from the test context accepts a boolean argument, allowing you to skip the test conditionally without an explicit if-statement [1][5]. test('conditional skip', ({ skip, isFeatureEnabled }) => { skip(!isFeatureEnabled, 'Feature not enabled'); }); When called, skip marks the test as skipped and prevents subsequent code within that test from executing [1][6].
Citations:
- 1: https://vitest.dev/guide/test-context
- 2: https://main.vitest.dev/api/test
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.9/docs/api/test.md
- 4: https://github.com/antfu/skills/blob/main/skills/vitest/references/features-context.md
- 5: https://github.com/vitest-dev/vitest/blob/main/docs/guide/test-context.md
- 6: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/guide/test-context.md
Replace early return with skip("embed daemon unavailable") for embed-unavailable path.
This currently exits as pass, not skip. The test context already provides the skip parameter (line 188); use it to explicitly mark the test as skipped when the embed daemon is unavailable, which aligns with the documented behavior (file header lines 22–26: "SKIPS CLEANLY").
Proposed fix
- if (!embedReady) return; // skip cleanly when the embed daemon is unavailable.
+ if (!embedReady) skip("embed daemon unavailable");📝 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 (!embedReady) return; // skip cleanly when the embed daemon is unavailable. | |
| if (!embedReady) skip("embed daemon unavailable"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/semantic-recall-live.itest.ts` at line 189, Replace the
plain return statement in the embedReady check with a call to skip() function to
properly mark the test as skipped when the embed daemon is unavailable. The skip
parameter is already available in the test context (from line 188), so call skip
with a descriptive message like "embed daemon unavailable" instead of using
return, which will ensure the test is marked as skipped rather than passed,
aligning with the documented behavior in the file header.
Why
Completes PRD-034's FR-4 / Suggestion S-1. The infra-degraded → NEUTRAL-skip preflight (
neutralizeIfInfraDegraded,tests/integration/_infra-skip.ts) was wired into only 2 demonstration itests (compaction,sources-purge). So a transient backend failure (502 / query_error / timeout / connection —isTransientResult) in any other gated live itest red the suite instead of neutralizing.That surfaced on the post-merge run as golden-path f-AC-2: the summary
memory-row APPEND transiently returned{written: false}→ a hard red. It correctly did not block main (theintegration-push-softjob iscontinue-on-error— main concludedsuccess), but it would redden the nightly canary as noise, defeating the point of a trustworthy canary.What
Wires the preflight into the 25 write/read-heavy gated live itests so a confirmed transient-class failure neutralizes (marker + skip) while a non-transient failure (wrong value, broken tenancy, real SQL error, lost write on a healthy backend) still REDS — the teeth are preserved.
For domain ops that return a boolean rather than a
QueryResult(e.g.writeSummary → {written}), a failure triggers a lightweight storage probe (SELECT 1) whoseQueryResultis classified — so we neutralize only on confirmed backend-transient degradation, never on a bare domainfalse.teeth-proof-live.itest.tsanddeeplake-stress-live.itest.tsare deliberately untouched (the first must red on a real regression; the second's weather is its data).Verification
src, noci.yaml🤖 Generated with Claude Code
Summary by CodeRabbit