test(quality): remove vacuous and duplicate tests, resurrect dead suites, add discriminating coverage#288
Conversation
github-actions
Bot
added
area:ci
📝 WalkthroughWalkthroughThe PR updates runtime and adversarial test gating, adjusts bridge timeout and recovery expectations, adds codec and frame coverage to CI, verifies CLI artifacts, expands generator tests, and removes or narrows obsolete contract and quality tests. ChangesBridge and adversarial behavior
Codec and generated output coverage
Runtime and contract test maintenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
area:codec
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…arial suite, add discriminating coverage Tests: 94 deleted, 2 consolidated into generator.test.ts, 2 new discriminating tests added.\nNet test-source lines: -2,596.\n\nEnables adversarial coverage with Python availability gating and wires Python codec/frame tests into CI.
…y for worker restart The isolation test's sequential warmups both landed on worker 1 (it frees between calls), leaving worker 2 to pay its first-serialize import cost (#285) inside the timed fast call; the post-timeout recovery call likewise paid a worker restart + cold import inside a 1s budget. Warm concurrently (concurrency 1 forces distinct workers) and use 5s timeout / 8s sleep so isolation, timeout, and recovery assertions keep their meaning with realistic budgets on current main.
Error-surfacing tests assert loudness, not latency; 2s defaults could not absorb worker cold-start/import costs (#285) under full-suite load and flaked as timeouts. Timeout-behavior tests keep their explicit small budgets.
…ronments The two it.fails timeout tests were misdiagnosed: NodeBridge DOES recover after a request timeout. They failed locally only because the worker's first-serialize import of the scientific stack (#285) consumed the 200ms budget before the fixture ever ran — while CI runners without numpy passed them, tripping vitest's 'Expect test to fail' on the .fails marker. Drop .fails and retune so both environments agree: a 4s request budget absorbs the cold-import cost (~2s under load) but not the 6s fixture sleep, so the timeout still fires deterministically and the recovery call fits its own budget.
8b26f21 to
17274c4
Compare
The out_of_order_bridge fixture only responds once two requests are in flight, but the pool now defaults to one in-flight request per worker (#284) — the second call was never sent and the first deadlocked into its 2s timeout. Pass maxConcurrentPerProcess: 2 so the reordering path is actually exercised.
coderabbitai
Bot
removed
enhancement
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 @.github/workflows/ci.yml:
- Around line 101-104: Pin the pytest dependency used by the “Run Python codec
and frame tests” step to a compatible version range, preferably by adding
pytest>=8,<9 to test/python/requirements-suite-core.txt and removing the
unpinned inline installation while preserving the existing test command.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 65004eb4-c471-4b0a-aef8-b1c55728e495
📒 Files selected for processing (16)
.github/workflows/ci.ymltest/adversarial_playground.test.tstest/arch-stories.test.tstest/bridge-codec.test.tstest/cli.test.tstest/code_generation_quality.test.tstest/frame-codec.test.tstest/generator.test.tstest/rpc-client.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_bun.test.tstest/runtime_config.test.tstest/runtime_deno.test.tstest/transport-pool.test.tstest/transport.test.tstest/type_mapping_edge_cases.test.ts
💤 Files with no reviewable changes (5)
- test/code_generation_quality.test.ts
- test/type_mapping_edge_cases.test.ts
- test/runtime_config.test.ts
- test/transport-pool.test.ts
- test/arch-stories.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: test (20, 3.11)
- GitHub Check: test (20, 3.12)
- GitHub Check: test (22, 3.12)
- GitHub Check: os (macos-latest)
- GitHub Check: data-plane-perf
- GitHub Check: test (20, 3.10)
- GitHub Check: codec-suite
- GitHub Check: os (windows-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript strict mode and avoid
any; preferunknownwith type guards.
Files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/bridge-codec.test.tstest/generator.test.tstest/frame-codec.test.tstest/transport.test.tstest/rpc-client.test.tstest/runtime_bun.test.tstest/cli.test.tstest/adversarial_playground.test.ts
test/runtime_*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place corresponding runtime bridge tests in
test/runtime_*.test.ts.
test/runtime_*.test.ts: Place corresponding runtime bridge tests intest/runtime_*.test.ts.
Include tests for new runtime behavior, following existing runtime test patterns.
Files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/runtime_bun.test.ts
🧠 Learnings (4)
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/bridge-codec.test.tstest/generator.test.tstest/frame-codec.test.tstest/transport.test.tstest/rpc-client.test.tstest/runtime_bun.test.tstest/cli.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/bridge-codec.test.tstest/generator.test.tstest/frame-codec.test.tstest/transport.test.tstest/rpc-client.test.tstest/runtime_bun.test.tstest/cli.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/bridge-codec.test.tstest/generator.test.tstest/frame-codec.test.tstest/transport.test.tstest/rpc-client.test.tstest/runtime_bun.test.tstest/cli.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/runtime_deno.test.tstest/bridge-codec.test.tstest/generator.test.tstest/frame-codec.test.tstest/transport.test.tstest/rpc-client.test.tstest/runtime_bun.test.tstest/cli.test.tstest/adversarial_playground.test.ts
🪛 ESLint
test/bridge-codec.test.ts
[error] 115-115: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 128-128: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 168-168: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 169-169: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 176-176: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 183-183: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 192-192: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 193-193: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 199-199: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 200-200: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 232-232: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
test/generator.test.ts
[error] 1430-1430: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 1431-1431: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 1454-1454: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
test/adversarial_playground.test.ts
[error] 132-132: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 265-265: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 283-283: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 465-465: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 466-466: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
🪛 OpenGrep (1.23.0)
test/runtime_bun.test.ts
[ERROR] 77-77: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 85-88: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 409-412: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 417-417: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🪛 zizmor (1.26.1)
.github/workflows/ci.yml
[warning] 81-104: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🔇 Additional comments (15)
test/rpc-client.test.ts (1)
298-316: LGTM!The test correctly verifies that
sendOnbypasses lifecycle initialization for both the held and explicit transports, asserts the encoded message contains the expected RPC method, and disposes the client infinally. This aligns with the upstreamsendOncontract that deliberately skipsthis.execute()and auto-init to avoid deadlock during warmup.test/runtime_bridge_fixtures.test.ts (1)
238-238: LGTM!The load-sensitivity comment is helpful context, and the
15_000numeric separator formatting is a clean readability improvement with no behavioral change.Also applies to: 261-261
test/runtime_bun.test.ts (1)
23-27: 📐 Maintainability & Code Quality | ⚡ Quick winConflict with established learning on skip vs. early-return pattern.
A prior learning from this repository (PR 127) established that fixture/availability-based tests should prefer early returns when Python or runtime capabilities are unavailable, treating a silent pass as intentional rather than relying on Vitest skip APIs. This PR explicitly reverses that pattern by replacing silent guards with
it.skipIf(...).If this is a deliberate policy change, consider updating the learning to reflect the new convention so future reviews don't flag it. Otherwise, verify that visible skips are preferred over silent passes for Bun runtime tests specifically.
The
it.skipIfgating logic itself is correct — particularly the compound guard at line 405 (!processAvailable || (!shellPath && !falsePath)) which safely ensuresfalsePath!at line 417 is non-null when the shell branch is not taken.The static analysis command-injection hints on lines 77, 85–88, 409–412, and 417 are false positives: all executable paths originate from
Bun.which, not user input, and this is test code.Based on learnings: "In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures."
Also applies to: 68-104, 135-151, 160-168, 405-420
Sources: Learnings, Linters/SAST tools
test/runtime_deno.test.ts (1)
4-6: LGTM!Clear and accurate CI documentation. Noting that this file requires a native Deno runner and is intentionally excluded from current CI jobs helps future contributors understand why these tests don't appear in CI output.
test/adversarial_playground.test.ts (4)
8-16: LGTM!Also applies to: 74-77, 88-92, 105-109
123-136: LGTM!Also applies to: 265-283, 399-416, 453-469
587-592: LGTM!
691-698: LGTM!Also applies to: 725-746, 814-839, 1000-1005
test/bridge-codec.test.ts (2)
110-130: LGTM!Also applies to: 162-200, 228-232, 334-335
719-722: LGTM!test/frame-codec.test.ts (1)
162-185: LGTM!.github/workflows/ci.yml (1)
81-104: 🩺 Stability & AvailabilityAdd an explicit
permissions:block. This workflow doesn’t set one, so the token scope depends on repository defaults;contents: readis sufficient for these test jobs.test/cli.test.ts (1)
8-16: LGTM!Also applies to: 346-346, 363-363, 381-400, 441-472
test/generator.test.ts (1)
1412-1432: LGTM!Also applies to: 1434-1456
test/transport.test.ts (1)
454-454: LGTM!Also applies to: 629-658
| - name: Run Python codec and frame tests | ||
| run: | | ||
| python -m pip install pytest | ||
| python -m pytest test/python/test_bridge_codec.py test/python/test_frame_codec.py |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Pin pytest version for reproducible CI.
python -m pip install pytest installs the latest version without constraints. A breaking or compromised pytest release could fail CI or introduce supply-chain risk. Consider pinning to a compatible version range or adding pytest to test/python/requirements-suite-core.txt alongside the other dependencies.
♻️ Suggested fix
- name: Run Python codec and frame tests
run: |
- python -m pip install pytest
+ python -m pip install 'pytest>=8,<9'
python -m pytest test/python/test_bridge_codec.py test/python/test_frame_codec.pyAlternatively, add pytest>=8,<9 to test/python/requirements-suite-core.txt and remove the inline install.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run Python codec and frame tests | |
| run: | | |
| python -m pip install pytest | |
| python -m pytest test/python/test_bridge_codec.py test/python/test_frame_codec.py | |
| - name: Run Python codec and frame tests | |
| run: | | |
| python -m pip install 'pytest>=8,<9' | |
| python -m pytest test/python/test_bridge_codec.py test/python/test_frame_codec.py |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 81-104: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for 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.
In @.github/workflows/ci.yml around lines 101 - 104, Pin the pytest dependency
used by the “Run Python codec and frame tests” step to a compatible version
range, preferably by adding pytest>=8,<9 to
test/python/requirements-suite-core.txt and removing the unpinned inline
installation while preserving the existing test command.
1 participant
Final PR of the v0.9.0 cleanup wave. From a three-agent test-quality audit of the full suite (~34k lines): remove tests that verify nothing, resurrect tests that were never running, consolidate overlap into single owners, and add the discriminating tests the audit found missing. Net −2,596 lines with strictly higher fault detection.
Deleted (vacuous/tautological/duplicate):
code_generation_quality.test.ts(2,034 lines): duplicatedgenerator.test.tswith weaker substring assertions plus "readable code"/"bundle size" heuristics that pass for incorrect output; unique fixtures moved togenerator.test.tswith exact-shape assertionsruntime_config.test.ts: tests that built a local object literal and asserted on that same literal — no runtime import at alltoBeInstanceOf/toBeDefinedconstructor stubs acrosstransport/transport-pool/rpc-clienttests; one circularcapabilities()assertion; validator re-tests inarch-stories; TypeVar duplicates intype_mapping_edge_casesResurrected:
adversarial_playground.test.ts(~40 tests) was gated onTYWRAP_ADVERSARIAL=1, which nothing has ever set — it has never run in CI. Now runs on python availability. Its first-ever execution immediately caught both product bugs fixed elsewhere in this wave: the serial-worker pool default (fix(runtime)!: serial-worker pool default and strict codec rejection of Map/Set #284 — its isolation tests fail on main without an explicitmaxConcurrentPerProcess: 1) and the first-serialize import tax (perf(runtime): type-first codec dispatch — stop cold-importing the scientific stack for plain values #285 — cold workers/restarts blow 1–2s call budgets). Tests now warm workers concurrently and use suite-level budgets for error-surfacing assertions (loudness, not latency); genuine timeout-behavior tests keep explicit small budgets. Three latent limitations markedtest.failswith FIXMEs.test/python/test_bridge_codec.py+test_frame_codec.pywere invoked by no CI job — now wired intopython-suite-coreruntime_bun.test.tssilent in-bodyreturnguards → visibleskipIf;runtime_deno.test.tsdocumented as executed by no CI job (needs a native Deno runner — tracked for the docs-accuracy wave)Added (audit's top missing discriminating tests):
RpcClient.sendOn()lifecycle-bypass contract (assertsinitis NOT triggered)generator.test.tsnpm run check:allgreen: 1,332 passed, 3 expected-fail, 0 failures.