Feat/ts integration tests#136
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the TypeScript implementation’s test coverage by adding dedicated integration and performance Vitest suites (and wiring them into CI), while also introducing new/updated TypeScript runtime entrypoint components (CLI parsing, server bootstrap, and shutdown coordination) needed for end-to-end subprocess behavior.
Changes:
- Add separate Vitest configs + npm/make targets for integration and performance test runs, and exclude them from the default unit test run.
- Add TypeScript integration tests that exercise real subprocess PTY behavior, plus performance-focused test suites for response compactness, memory/FD leak checks, and benchmark-style assertions.
- Wire TypeScript CI to run unit → integration → performance on relevant pushes/PRs, and add a postinstall script to repair
node-ptyspawn-helper permissions.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/vitest.config.ts | Excludes integration/performance tests from the default unit suite. |
| typescript/vitest.config.integration.ts | Adds a dedicated Vitest config for integration tests with longer timeouts. |
| typescript/vitest.config.performance.ts | Adds a dedicated Vitest config for performance tests (incl. --expose-gc). |
| typescript/tsconfig.json | Enables skipLibCheck to work around third-party .d.ts typing issues. |
| typescript/src/utils/cleanup.ts | Adds a shutdown coordinator for signals/transport-close with force-exit timer. |
| typescript/src/server.ts | Adds MCP server construction + stdio/http transport handling and request lifecycle. |
| typescript/src/main.ts | Refactors entrypoint to parse CLI/apply logging before importing server modules. |
| typescript/src/interactive/pty_handler.ts | Improves PTY spawn error diagnostics for missing executables/spawn-helper perms. |
| typescript/src/constants.ts | Adds CLI exit code constants (and retains existing runtime constants). |
| typescript/src/config/cli.ts | Adds commander-based CLI parsing with testable ValidationError flow. |
| typescript/scripts/fix-node-pty.cjs | Postinstall script to ensure node-pty spawn-helper is executable. |
| typescript/package.json | Adds postinstall + new test scripts (integration/performance/all). |
| typescript/tests/utils/cleanup.test.ts | Unit tests for CleanupManager signal + handler behavior. |
| typescript/tests/server.test.ts | MCP smoke tests for tool registration/annotations and tool call round-trip. |
| typescript/tests/performance/response_sizes.test.ts | “Token/compactness” checks for response size stability. |
| typescript/tests/performance/memory_monitoring.test.ts | Memory/FD leak detection tests (Linux FD checks guarded). |
| typescript/tests/performance/benchmarks.test.ts | Benchmark-style latency/throughput/stress tests (mocked sessions). |
| typescript/tests/interactive/pty_handler.test.ts | Adds coverage for enhanced PATH/spawn-helper hint on spawn errors. |
| typescript/tests/integration/pty_integration.test.ts | Real-subprocess PTY integration scenarios (bash/cat/echo/sleep). |
| typescript/tests/config/cli.test.ts | Unit tests for CLI parsing and error handling. |
| Makefile | Adds make targets to run TS integration/performance/all test suites. |
| .github/workflows/ts-ci.yml | Extends TS CI to run integration and performance suites after unit tests. |
Comments suppressed due to low confidence (1)
typescript/src/interactive/pty_handler.ts:71
- createSession() can be called with an empty command array (e.g. from the tool schema), and when ENABLE_COMMAND_VALIDATION is false this will fall through to spawn(command[0]!) and throw a low-level error. It’s safer to always enforce non-empty commands before spawning.
const executable = command[0] ?? "";
try {
this.validateCommand(command);
const processEnv: Record<string, string> = {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
737ad8a to
30a893d
Compare
|
@kartikloops please rebase |
…and exclude settings
…m test_pty_integration.py
… and buffer overflow
30a893d to
4019f63
Compare
|
@luarss rebased and addressed review, please have a look |
| expect(fdDiff).toBeLessThanOrEqual(5); | ||
| }); | ||
|
|
||
| it.skip("stability simulation: 24-hour scaled run (enable manually, takes ~24s)", async () => { |
There was a problem hiding this comment.
why are you skipping this? walk me through the intuition of this test as well. why are we simulating 24 hours?
There was a problem hiding this comment.
It's a slow-leak soak test: one session, 24 simulated hours (each hour = 100 commands + a 1s settle, so ~24s wall-clock), asserting growth stays under 0.2 MB/hour. The point is to catch a slow per-command drip that the bulk-allocation tests miss, the kind of thing that only matters for a long-running daemon like this server. It measures a rate over 24 hours, which is far more sensitive to a slow leak than a single before/after delta.
Skipped because
(1) ~24s is too slow for a per-PR gate,
(2) RSS is noisy enough at a 0.2MB threshold that it'd flap as a hard CI assertion, and
(3) the session is mocked so it's really checking manager-side retention, not OpenROAD. It's kept as a documented manual probe you enable when investigating suspected growth. Happy to move it behind a RUN_SOAK=1 flag + nightly job if we'd rather it run on a schedule than never.
| const rssDiff = mon.rss("start", "end"); | ||
| console.log(` session creation leak: RSS diff ${rssDiff.toFixed(1)}MB`); | ||
| // RSS granularity is one OS page (typically 4MB); allow 12MB headroom | ||
| expect(rssDiff).toBeLessThan(12); |
There was a problem hiding this comment.
how can we ensure that we are measuring the memory from the app and not from the vitest/ts overheads?
There was a problem hiding this comment.
The measurement is a delta (end − start), so the fixed vitest/TS/V8 baseline cancels out. we're reading growth during the create/terminate cycles, not absolute process memory. GC is forced before each snapshot (global.gc() + --expose-gc in the perf config) so it's not counting uncollected garbage, and the perf suite runs in a forked pool so other test files don't share the heap. RSS is coarse by nature, hence the loose 12MB threshold and using heapUsed for the actual leak check. The one thing it deliberately doesn't measure is real OpenROAD RSS, the session is mocked, so this tracks manager-side memory only.
There was a problem hiding this comment.
so what are the memory measurements we get now? on average
There was a problem hiding this comment.
Here are the actual local numbers from macOS using the mocked manager. These measure manager-side memory only, not real OpenROAD memory. The results were stable across three runs.
| Test | Measured | Threshold |
|---|---|---|
| Session-creation leak, 10 cycles with 5 create/terminate operations each | ~5.4 MB RSS growth | < 12 MB |
| Long-running test, 1000 operations while active | ~3.4 MB RSS growth | < 25 MB |
| Long-running test, heap retained after cleanup | ~-1 MB | <= 5 MB |
| Concurrent test, 20 sessions | ~0.3 MB total, about ~0.02 MB per session | < 2 MB |
The main takeaway is that there is no leak signal. The create/terminate cycles settle around 5 MB, which looks like normal RSS noise rather than linear growth. The post-cleanup heap delta is negative, which means memory is being reclaimed after cleanup.
The per-session cost is also very small, around 0.02 MB per session. Since this test uses the mocked manager, that number reflects only the manager's own bookkeeping overhead, such as maps and session tracking. That is exactly what this check is meant to measure.
The thresholds are intentionally loose, about two to five times higher than the observed values. RSS is coarse and can move in page-sized jumps, and garbage collection timing adds some jitter, so the headroom keeps the test from being flaky.
One note: the file descriptor leak test is Linux-only, so it skips on macOS. Those numbers should appear in the Linux CI run.
Summary
Adds TypeScript integration and performance test suites (ported from the Python
tests/integration/andtests/performance/suites) so the TS server has the same real-subprocess and benchmark coverage as the Python implementation, and wires them into CI.__tests__/integration/pty_integration.test.ts) — 10 scenarios against real subprocesses (bash,cat,echo,sleep): basic exec, interactive I/O, multi-line output, lifecycle, error handling, concurrent read/write, env/cwd, large output, timeouts, sequential sessions.__tests__/performance/) — session creation/command latency, streaming throughput, concurrent scalability, memory profiling, buffer overflow, long-running stability, resource exhaustion, RSS/heap leak detection, FD leak detection (Linux), and token-budget/response-compactness checks.vitest.config.integration.ts/vitest.config.performance.ts(separate timeouts, excluded from the defaultnpm run testrun).npm run test:integration,test:performance,test:allscripts; matchingmake test-ts-integration,test-ts-performance,test-ts-alltargets.ts-ci.yml) runs unit → integration → performance on every push/PR touchingtypescript/**.