Finish the skipped Durable Object storage instrumentation tests

[ayaangazali]

packages/otel-cf-workers/test/instrumentation/do-storage.test.ts had five test.skip cases that were left unfinished, so the rejection path of instrumentStorage had no coverage at all. Three of them called toMatchInlineSnapshot() with no argument, the other two still expected the pre-semconv shape (do:storage:deleteAll as the span name, bare allowConcurrency/hasResult attributes), and the get one mocked storage.get to reject while actually calling instrument.list(). All five also asserted events was empty, which cannot hold on a rejection since the instrumentation records an exception event.

This enables all five and writes the assertions against current behavior: the Durable Object Storage <op> span name, the db.* attributes actually emitted (no db.cf.do.has_result on the error path, since that is only set after the call resolves), an ERROR status, and one exception event carrying the thrown message. The deleteAll case picks up db.cf.do.allow_unconfirmed, which nothing covered before.

Ran pnpm run check from the root, all green, and the package went from 48 passing with 5 skipped to 53 passing. I also sanity-checked that the new assertions bite: deleting the recordException and setStatus lines from do-storage.ts makes four of them fail. Test-only change, so no changeset.

quick disclosure: put this together with Claude Code and read the diff myself before opening. Freshman still learning, so if you would rather just delete these stubs than finish them, say so and I will send that instead :)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0431a388-8163-4fec-9900-3478886d9e76

📥 Commits

Reviewing files that changed from the base of the PR and between e4762be and 8b7e85d.

📒 Files selected for processing (1)

• packages/otel-cf-workers/test/instrumentation/do-storage.test.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• pydantic/logfire (manual)
• pydantic/pydantic-ai (manual)

---

📝 Walkthrough

Walkthrough

The tests now cover rejected delete, get, list, and put operations. They verify current span names, OpenTelemetry database attributes, error status codes, exception events, and exception messages. The deleteAll options test is enabled and checks current attributes for concurrency, unconfirmed, and cache options. SpanStatusCode is imported for error-status assertions.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes completing the skipped Durable Object storage instrumentation tests.
Description check	✅ Passed	The description directly explains the completed tests, corrected expectations, rejection-path coverage, and validation results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[petyosi]

The whole Cloudflare instrumentation package/approach needs reconsideration. After we made this package, Cloudflare themselves added excellent first-party OTel support (with the exception of missing way to send manual spans, which I guess will come soon), so the value of a third-party package is quite questionable.

[ayaangazali]

Rebased onto current main (817fca1) and re-verified there, since the original run was against a base from before the dependency refresh in #212.

State on HEAD: the 5 test.skip cases are still present on main, the branch takes the package from 48 passing plus 5 skipped to 62 passing, and CI is green. Re-ran the mutation check after the rebase to confirm the un-skipped tests still bite: removing recordException from do-storage.ts fails exactly 4 of them.

No code changes, tests only. Ready whenever you want it.

[petyosi]

Reviewed the full diff and relevant surrounding code. No significant correctness, failure-mode, compatibility, or test-coverage issues found.

[ayaangazali]

Rebased onto current main (79bcac0) and re-verified there.

State on HEAD: the 5 test.skip cases are still present on main, this branch takes the package from 48 passing plus 5 skipped to 62 passing, and CI is green. Mutation check still holds after the rebase: removing recordException from do-storage.ts fails exactly 4 of the un-skipped tests.

Tests only, no source change. Let me know if you would rather I close it.

[ayaangazali]

You approved this on 08-10 and it has been green and conflict-free since. It is still clean against 7874752, and the five test.skip cases it removes are still skipped on main.

I am not asking you to prioritise it. I am asking whether approved-but-unmerged is a state you are holding deliberately, because I have a few in that state and I would rather close them than have them sit in your queue looking like work you owe me. Happy either way.

Tests only, no source change.