Cover env binding detection with tests - #211
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a test suite for 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@packages/otel-cf-workers/test/instrumentation/env.test.ts`:
- Line 12: Extend the “instrumentEnv binding detection” suite to cover the
isJSRPC(item) path in instrumentEnv, using a service-binding fixture that
reaches instrumentServiceBinding. Assert the binding’s observable method
behavior, while preserving the existing coverage for other binding types.
- Around line 28-35: Update the instrumentation tests around the KV, Durable
Object, D1, and plain-object binding cases to invoke each returned method and
assert its expected result or mock invocation, rather than checking only typeof.
In the plain-object case, assert the returned result is exactly the original
plain object using identity equality, while preserving the KV method-wrapping
assertion.
🪄 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: 19788efd-b14d-4ce4-b0ea-0a4ae85cc5ae
📒 Files selected for processing (1)
packages/otel-cf-workers/test/instrumentation/env.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/otel-cf-workers/test/instrumentation/env.test.ts`:
- Around line 115-118: Update the span-name assertion following the single
service call to compare the complete finished-span name list exactly with
['Service Binding MY_SERVICE'] using toEqual, replacing toContain while
preserving the existing mapping and test setup.
🪄 Autofix
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: 57b58216-67c2-494a-be7b-f8b87e393013
📒 Files selected for processing (1)
packages/otel-cf-workers/test/instrumentation/env.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)
petyosi
left a comment
petyosi
left a comment
There was a problem hiding this comment.
Reviewed the full diff and relevant surrounding code. No significant correctness, failure-mode, compatibility, or test-coverage issues found.
8b50a71 to
bcad8fb
Compare
|
Rebased onto current main (79bcac0) and re-verified there. Approved and clean, no conflicts. Recap: this is test-only coverage for Happy to close it if you would rather not take this one. |
instrumentEnvdecides which instrumentation every Worker binding gets, and it had no tests. A regression in one of those shape checks would quietly route a binding to the wrong wrapper or leave it uninstrumented, and nothing would fail, so this adds coverage for each branch: values that are not proxyable pass through, version metadata is returned untouched so accesses stay untraced, and KV, queues, Durable Objects, Analytics Engine and D1 each get wrapped, plus an object matching no shape being left alone.The cases are driven through
instrumentEnvrather than the detector functions, since those are private, and they identify each binding by whether its methods came back wrapped. I sanity-checked that they actually bite by stubbing individual detectors tofalse: neutering the queue check fails the queue case and neutering the Analytics Engine check fails that one, so the assertions are pinned to the dispatch and not just to the mocks.Ran the repo preflight and
pnpm run check, both green, 65 passing in the package. Test only, so no changeset.One thing I noticed while writing these and did not touch:
isD1Databaseis the only detector that omits the!isJSRPC(item)guard its five siblings all have. It is unreachable today becauseisJSRPCis checked first in the dispatch chain, so a service binding never gets that far, but it would matter if that ordering ever changed. Happy to send it as a separate one-liner if you want the guard there for symmetry.quick disclosure: put this together with Claude Code and read it over myself. Freshman still learning, so if these read as low value I would rather hear it than keep adding them :)