From 6df9fc2221f735a10c78d93020d2e4f60699c6e7 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:08:23 -0500 Subject: [PATCH] test(server): consolidate duplicated startup-tracing unit suites (#17842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://entire.com/gh/paperclipai/-/trails/48/changes/paperclip/48 ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The server can export manual OpenTelemetry startup spans through `server/src/instrumentation.ts` (`resolveProtocol` plus the `getStartupTracer` / `getStartupTraceContext` accessors). > - Two suites covered that one module in every server Vitest run: `server/src/instrumentation.test.ts` (unit cases added with the manual-span seam in #10565) and `server/src/__tests__/instrumentation.test.ts` (the bootstrap machinery from #3735/#12249). > - `resolveProtocol` and the startup tracer accessors were registered twice per run with overlapping but unequal tables, so the same contracts were executed by two files while each held a few unique rows the other lacked. > - This pull request moves the unique assertions into the `__tests__` owner and retires the colocated file, so one suite exercises the full contract once. > - The benefit is one fewer suite registration per server run and one less duplicated unit surface to maintain, with no lost assertion. ## Linked Issues or Issue Description **What happened?** The server Vitest project ran two test files against `server/src/instrumentation.ts`. Both registered `resolveProtocol` and startup-tracer accessor cases. The colocated file held unique coverage (protocol input trimming, warn-once, the `getStartupTraceContext` accessor, the `setAttribute`/`setStatus` span surface) while the `__tests__` file held the bootstrap, peer-version, shutdown, and span-recording coverage. The split duplicated executions and made the contract's owner ambiguous. **Expected behavior** One suite (`server/src/__tests__/instrumentation.test.ts`) owns every `server/src/instrumentation.ts` unit contract. Every assertion from the retired file still runs, inside the owner. No production code changes. ## What Changed - Retired `server/src/instrumentation.test.ts` after migrating its unique assertions into `server/src/__tests__/instrumentation.test.ts`. - Added the `OTEL_EXPORTER_OTLP_PROTOCOL=" HTTP/PROTOBUF "` row, proving surrounding space is trimmed (previously only the letter-case row existed). - Strengthened the unknown-protocol case to assert the warning fires exactly once. - Extended the no-op tracer case to exercise `setAttribute` and `setStatus` before `end`. - Added the `getStartupTraceContext` accessor case (tracer surface plus the `contextWithSpan` helper), previously covered only by the retired file. Six production callers use this seam (heartbeat runtime execution, native run trace, duplex observability recorder, and others). ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/instrumentation.test.ts` — 21 passed, 2 skipped (SDK-gated span-recording cases skip without `@opentelemetry/*`, as before). The two new/extended cases pass inside the owner. - Deliberate failure controls, both restored byte-exact afterward and re-run green: - removing `.trim()` from `resolveProtocol` made exactly the migrated `" HTTP/PROTOBUF "` row fail; - making the no-op `contextWithSpan` throw made exactly the migrated accessor case fail. - The retired file's six remaining `resolveProtocol` rows and one `getStartupTracer` case were exact-value duplicates of owner rows (same inputs, same expected protocol/package pairs, same no-op tracer shape). - Full repository typecheck/test/build were not run locally (no Rust toolchain for the Runner build in this environment; server-wide `tsc --noEmit` exceeds the session window). CI is the authoritative gate for those; this diff touches only test code under `server/src`. ## Risks Low. Test-only change; no production behavior, routing, or CI configuration touched. Known stale mentions of the retired path are non-functional: `.devin/wiki.json` lists it as a non-exhaustive wiki anchor (its own instructions say to relocate removed anchors at generation time) and `scripts/general-server-shard-durations.json` keeps a historical duration key that no longer matches any selected file. ## Model Used - Provider: Z.ai via OpenRouter (agent: Paperclip Test Gardener) - Model ID: `openrouter/z-ai/glm-5.3` - Reasoning: standard chain-of-thought mode, tool use (shell, file edit, GitHub CLI) - Context window: 200k tokens (provider default for this model) ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes: #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (none required: test-only change) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --- server/src/__tests__/instrumentation.test.ts | 40 ++++++- server/src/instrumentation.test.ts | 114 ------------------- 2 files changed, 37 insertions(+), 117 deletions(-) delete mode 100644 server/src/instrumentation.test.ts diff --git a/server/src/__tests__/instrumentation.test.ts b/server/src/__tests__/instrumentation.test.ts index 475dc770355..cf9764967cd 100644 --- a/server/src/__tests__/instrumentation.test.ts +++ b/server/src/__tests__/instrumentation.test.ts @@ -43,6 +43,8 @@ describe("resolveProtocol", () => { ["http/protobuf", "http/protobuf", "@opentelemetry/exporter-trace-otlp-proto"], ["http/json", "http/json", "@opentelemetry/exporter-trace-otlp-http"], ["HTTP/JSON", "http/json", "@opentelemetry/exporter-trace-otlp-http"], + // Surrounding space is trimmed before the switch, not only letter case. + [" HTTP/PROTOBUF ", "http/protobuf", "@opentelemetry/exporter-trace-otlp-proto"], ])("maps OTEL_EXPORTER_OTLP_PROTOCOL=%s to %s", async (raw, protocol, packageName) => { if (raw === undefined) delete process.env[PROTOCOL_ENV]; else process.env[PROTOCOL_ENV] = raw; @@ -60,8 +62,15 @@ describe("resolveProtocol", () => { const { resolveProtocol } = await importFreshInstrumentation(); - expect(resolveProtocol().protocol).toBe("grpc"); + // The fallback must also select the grpc exporter package, not just a + // grpc-labeled protocol. + expect(resolveProtocol()).toEqual({ + protocol: "grpc", + packageName: "@opentelemetry/exporter-trace-otlp-grpc", + }); expect(warn).toHaveBeenCalledWith(expect.stringContaining("carrier-pigeon")); + // One unknown input logs exactly one warning, not one per lookup. + expect(warn).toHaveBeenCalledOnce(); }); }); @@ -249,8 +258,14 @@ describe("getStartupTracer", () => { // startup seam calls, so the caller needs no null check. expect(tracer).not.toBeNull(); expect(typeof tracer.startSpan).toBe("function"); - // A no-op tracer must open and end a span without throwing. - expect(() => tracer.startSpan("workspace.resolve").end()).not.toThrow(); + // A no-op tracer must carry attribute, status, and end calls without + // throwing: the startup seam sets attributes and status before it ends. + expect(() => { + const span = tracer.startSpan("workspace.resolve"); + span.setAttribute("k", "v"); + span.setStatus({ code: 0 }); + span.end(); + }).not.toThrow(); }); it("loads no OTel SDK package when the endpoint is unset", async () => { @@ -277,6 +292,25 @@ describe("getStartupTracer", () => { }); }); +describe("getStartupTraceContext", () => { + it("returns a tracer and a parent-context helper that never throw when the endpoint is unset", async () => { + const { getStartupTraceContext } = await importFreshInstrumentation(); + + const traceContext = getStartupTraceContext(); + + // The tracer handle carries the same span surface as getStartupTracer. + expect(() => { + const span = traceContext.tracer.startSpan("runtime.exec"); + span.setAttribute("k", "v"); + span.setStatus({ code: 0 }); + span.end(); + }).not.toThrow(); + // The helper never throws. It returns a parent token when the api + // resolves, or `undefined` from the no-op fallback when it does not. + expect(() => traceContext.contextWithSpan({})).not.toThrow(); + }); +}); + describe("shutdownInstrumentation", () => { it("is a no-op when tracing is off and idempotent across calls", async () => { const { shutdownInstrumentation } = await importFreshInstrumentation(); diff --git a/server/src/instrumentation.test.ts b/server/src/instrumentation.test.ts deleted file mode 100644 index 76c29a65ec5..00000000000 --- a/server/src/instrumentation.test.ts +++ /dev/null @@ -1,114 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { - getStartupTraceContext, - getStartupTracer, - resolveProtocol, -} from "./instrumentation.js"; - -// The span export path selects the OTLP trace-exporter package from the -// `OTEL_EXPORTER_OTLP_PROTOCOL` env var. `resolveProtocol` owns that choice. -// A wrong package name sends spans over the wrong wire protocol, so the -// exporter never reaches the collector. -describe("resolveProtocol", () => { - const PROTOCOL_ENV = "OTEL_EXPORTER_OTLP_PROTOCOL"; - let previous: string | undefined; - - beforeEach(() => { - previous = process.env[PROTOCOL_ENV]; - delete process.env[PROTOCOL_ENV]; - }); - - afterEach(() => { - if (previous === undefined) { - delete process.env[PROTOCOL_ENV]; - } else { - process.env[PROTOCOL_ENV] = previous; - } - vi.restoreAllMocks(); - }); - - it("defaults to grpc when the protocol is unset", () => { - expect(resolveProtocol()).toEqual({ - protocol: "grpc", - packageName: "@opentelemetry/exporter-trace-otlp-grpc", - }); - }); - - it("defaults to grpc when the protocol is an empty string", () => { - process.env[PROTOCOL_ENV] = ""; - expect(resolveProtocol()).toEqual({ - protocol: "grpc", - packageName: "@opentelemetry/exporter-trace-otlp-grpc", - }); - }); - - it("selects the grpc exporter for grpc", () => { - process.env[PROTOCOL_ENV] = "grpc"; - expect(resolveProtocol()).toEqual({ - protocol: "grpc", - packageName: "@opentelemetry/exporter-trace-otlp-grpc", - }); - }); - - it("selects the protobuf exporter for http/protobuf", () => { - process.env[PROTOCOL_ENV] = "http/protobuf"; - expect(resolveProtocol()).toEqual({ - protocol: "http/protobuf", - packageName: "@opentelemetry/exporter-trace-otlp-proto", - }); - }); - - it("selects the json exporter for http/json", () => { - process.env[PROTOCOL_ENV] = "http/json"; - expect(resolveProtocol()).toEqual({ - protocol: "http/json", - packageName: "@opentelemetry/exporter-trace-otlp-http", - }); - }); - - it("normalizes surrounding space and letter case", () => { - process.env[PROTOCOL_ENV] = " HTTP/PROTOBUF "; - expect(resolveProtocol()).toEqual({ - protocol: "http/protobuf", - packageName: "@opentelemetry/exporter-trace-otlp-proto", - }); - }); - - it("warns and falls back to grpc for an unknown protocol", () => { - process.env[PROTOCOL_ENV] = "carrier-pigeon"; - const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); - - expect(resolveProtocol()).toEqual({ - protocol: "grpc", - packageName: "@opentelemetry/exporter-trace-otlp-grpc", - }); - expect(warn).toHaveBeenCalledOnce(); - }); -}); - -// The manual startup spans read their tracer through these accessors. Both -// accessors must return a usable handle and never throw, whether or not -// `@opentelemetry/api` resolves. When the package is absent, the handle is a -// no-op; when it is present, the api returns a real no-op tracer while no SDK -// is registered. The span methods work in both cases. -describe("startup tracer accessors", () => { - it("returns a usable span from getStartupTracer", () => { - const tracer = getStartupTracer(); - const span = tracer.startSpan("unit-test"); - - expect(() => { - span.setAttribute("k", "v"); - span.setStatus({ code: 0 }); - span.end(); - }).not.toThrow(); - }); - - it("returns a tracer and a parent-context helper from getStartupTraceContext", () => { - const traceContext = getStartupTraceContext(); - - expect(() => traceContext.tracer.startSpan("unit-test").end()).not.toThrow(); - // The helper never throws. It returns a parent token when the api resolves, - // or `undefined` from the no-op fallback when it does not. - expect(() => traceContext.contextWithSpan({})).not.toThrow(); - }); -});