mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 21:49:18 +01:00
test(server): consolidate duplicated startup-tracing unit suites (#17842)
<!-- entire-trail-link-start --> https://entire.com/gh/paperclipai/-/trails/48/changes/paperclip/48 <!-- entire-trail-link-end --> ## 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
This commit is contained in:
1 parent
99d27fd5e7
commit
6df9fc2221
2 files changed
+37
-117
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user