Repository navigation
test(sdk): reuse API and session fixtures - #1229
Conversation
Repository fixtures and hydration tools duplicate Git process setup. Share the existing invocation patterns with their actual consumers.
SDK entrypoints and tests repeat runtime, authentication and option setup. Share the setup at existing owners and introduce internal helpers with their first consumers.
Progress, activity and cost paths repeat scalar and JSON parsing. Reuse internal helpers and simplify presentation without changing the rendered diagnostics or cost arithmetic.
# Conflicts: # sdk/typescript/src/security-policy.ts # sdk/typescript/tests-ts/api-policy.test.ts # sdk/typescript/tests-ts/api-post-scan.test.ts # sdk/typescript/tests-ts/component-scan.test.ts # sdk/typescript/tests-ts/security-policy.test.ts
# Conflicts: # sdk/typescript/tests-ts/support/shell.ts
|
@codex review the current head SHA. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The test-only refactoring preserves fixture values, overrides, isolation, and existing assertions.
Review effort: Balanced
Findings: None
What changed in this PR
Consolidates repeated SDK test setup into shared fixtures, building on #1209 without changing test behavior.
Changes:
- Shares runtime dependency and session-writing fixtures.
- Reuses worker-session and dashboard setup.
- Removes redundant test-client environment overrides.
| File | Description |
|---|---|
sdk/typescript/tests-ts/support/usage-rollout.ts |
Adds shared session fixtures. |
sdk/typescript/tests-ts/support/api-events.ts |
Adds shared runtime dependencies. |
sdk/typescript/tests-ts/scan-dashboard.test.ts |
Reuses dashboard setup. |
sdk/typescript/tests-ts/cost.test.ts |
Reuses session fixtures. |
sdk/typescript/tests-ts/api.test.ts |
Reuses runtime and usage fixtures. |
sdk/typescript/tests-ts/api-post-scan.test.ts |
Removes a redundant environment override. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Reviewed the expanded actual PR diff at 3b140c31451e8b9d4638be658aae39c91a475105, with base b96739d5ba184bc745f7ce97605781f97eaf0a66 and merge base 634532d7f7fb21ccca49b4bca9bb9fa1df9308d6, in three fresh independent HIGH full-diff passes plus root synthesis. All 20 changed paths and 10 test/helper paths were included. No actionable introduced issue was established.
The current three-dot diff includes earlier production refactors already present in the base; those were reviewed without attributing them as new own-base changes. Shared API and usage fixtures were checked statically. Unchanged imported helpers, callers, runtime compatibility and TestClient environment defaults were outside the touched-file scope.
This updates the review scope after the base changed and preserves the original approval submission. Tests, builds and product models were not run. Source approval does not establish CI readiness, whole-stack integration or deployment.
github-actions
Bot
added
the
skip-release-notes
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the actual PR contribution at e6589d72346fe7cca8d5bd8f06f1c50fe10fb006 against explicit base b96739d5ba184bc745f7ce97605781f97eaf0a66. The actual GitHub diff, ordered three-dot and own-base changes, and all 24 touched-file and instruction snapshots match the originally reviewed contribution exactly. The original three independent HIGH full-diff reports remain applicable, including all six changed test/helper paths and 1,468 diff lines. No repeat source pass was needed, and no supported introduced issue was identified.
The intermediate expanded contribution remains recorded with its separate three-pass review at the prior head and base. This approval covers the current reconciled test-helper and fixture contribution. Unchanged imports, callers and test-client defaults were outside the reviewed scope; runtime behavior was not independently verified.
Tests, builds and product models were not run locally. Source approval does not establish CI readiness, whole-stack integration or deployment.
|
@codex review Please review the current head |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the actual PR contribution at 62512244b68f3702170872d7690f4ca45c128827 against 89aae242136312467790947f3b122ca3f607614f in exactly three fresh independent HIGH full-diff passes plus root synthesis. All six changed test/helper paths and all 1,800 diff lines were included. No actionable introduced issue was established.
The shared runtime dependencies, session metadata options, worker fixtures and dashboard defaults were reviewed statically. Unchanged imports and TestClient defaults remain outside the touched-file scope.
Tests, builds and product models were not run. Source approval does not establish CI readiness, whole-stack integration or deployment.
Summary
Share repeated API, session, cost and dashboard fixtures while preserving each test's assertions. Import persistence tests reuse their selected Python executable so unrelated interpreter discovery does not interrupt import and reimport coverage on Windows.
Changes
Testing
Validated with Node 22.13.0 and Bun 1.3.14:
pnpm run types,pnpm run format,pnpm run build:ci, andpnpm run build:pluginpassed.Cross-platform validation runs in hosted CI.
Risk and rollout
Test-only change; production code and public CLI behavior are unchanged. Interpreter discovery remains covered by runtime tests, and import tests continue exercising real persistence and reimport behavior.
Public disclosure review