[chore]: configure test coverage with env var#1730
[chore]: configure test coverage with env var#1730seanmcguire12 wants to merge 3 commits intomainfrom
Conversation
|
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 9 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant CI as CI / Developer
participant Script as Test Script (Core/E2E/Server)
participant Utils as NEW: test-artifacts.ts
participant FS as File System
participant Runner as Test Runner (Vitest/Playwright/Node)
CI->>Script: Run test command (e.g. pnpm test:core)
Script->>Utils: isTestCoverageEnabled()
Note over Utils: Checks TEST_COVERAGE env var
Utils-->>Script: boolean
alt TEST_COVERAGE is enabled (CI or Local Opt-in)
Script->>Utils: NEW: initCoverageDir(path)
Utils->>FS: CHANGED: mkdirSync (recursive)
Utils-->>Script: coveragePath
Script->>Utils: NEW: initJunitPath(path)
Utils->>FS: CHANGED: mkdirSync (parent dir)
Utils-->>Script: junitPath
Script->>Utils: NEW: withCoverageEnv(env, coveragePath)
Utils-->>Script: Updated Env (NODE_V8_COVERAGE)
Script->>Runner: spawnSync (with reporter + coverage flags)
Runner->>FS: Write JUnit XML & Coverage data
Runner-->>Script: Exit Code
Script->>Utils: NEW: maybeWriteCtrf(junitPath, writer)
Utils->>FS: Convert JUnit to CTRF JSON
else TEST_COVERAGE is disabled (Default Local)
Script->>Utils: NEW: initCoverageDir(path)
Utils-->>Script: null
Script->>Utils: NEW: initJunitPath(path)
Utils-->>Script: null
Script->>Runner: CHANGED: spawnSync (minimal flags, no coverage env)
Note right of Runner: No artifacts generated
Runner-->>Script: Exit Code
end
Script-->>CI: Return Exit Status
Greptile SummaryThis PR makes test coverage and artifact generation opt-in via the Key changes:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 0d969af |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Confidence score: 4/5
- There is a medium-severity issue where
NODE_V8_COVERAGE/CTRF_JUNIT_PATHcan leak into the child process even whenTEST_COVERAGEis off, potentially bypassing the opt-in gate inpackages/core/scripts/test-e2e.ts. - Impact seems limited to test environment configuration rather than production behavior, so overall risk looks low-to-moderate and likely safe to merge with awareness.
- Pay close attention to
packages/core/scripts/test-e2e.ts- ensure coverage-related env vars are only set when explicitly enabled.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/scripts/test-e2e.ts">
<violation number="1" location="packages/core/scripts/test-e2e.ts:162">
P2: Environment variables `NODE_V8_COVERAGE` and `CTRF_JUNIT_PATH` leak from the parent process when `TEST_COVERAGE` is not enabled, bypassing the opt-in gate. When `coverageDir`/`ctrfPath` are `null`, `withCoverageEnv`/`withCtrfEnv` return the env unchanged, so any pre-existing values from `process.env` are passed to the child. Consider explicitly deleting these keys when coverage is disabled, e.g. `{ ...env, NODE_V8_COVERAGE: undefined }`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as CI / Developer
participant Script as Test Script (Core/E2E/Server)
participant Utils as NEW: test-artifacts.ts
participant FS as File System
participant Runner as Test Runner (Vitest/Playwright/Node)
CI->>Script: Run test command (e.g. pnpm test:core)
Script->>Utils: isTestCoverageEnabled()
Note over Utils: Checks TEST_COVERAGE env var
Utils-->>Script: boolean
alt TEST_COVERAGE is enabled (CI or Local Opt-in)
Script->>Utils: NEW: initCoverageDir(path)
Utils->>FS: CHANGED: mkdirSync (recursive)
Utils-->>Script: coveragePath
Script->>Utils: NEW: initJunitPath(path)
Utils->>FS: CHANGED: mkdirSync (parent dir)
Utils-->>Script: junitPath
Script->>Utils: NEW: withCoverageEnv(env, coveragePath)
Utils-->>Script: Updated Env (NODE_V8_COVERAGE)
Script->>Runner: spawnSync (with reporter + coverage flags)
Runner->>FS: Write JUnit XML & Coverage data
Runner-->>Script: Exit Code
Script->>Utils: NEW: maybeWriteCtrf(junitPath, writer)
Utils->>FS: Convert JUnit to CTRF JSON
else TEST_COVERAGE is disabled (Default Local)
Script->>Utils: NEW: initCoverageDir(path)
Utils-->>Script: null
Script->>Utils: NEW: initJunitPath(path)
Utils-->>Script: null
Script->>Runner: CHANGED: spawnSync (minimal flags, no coverage env)
Note right of Runner: No artifacts generated
Runner-->>Script: Exit Code
end
Script-->>CI: Return Exit Status
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| "include": ["**/*.ts", "lib/v3/cli.js"], | ||
| "exclude": ["node_modules", "dist"] | ||
| "exclude": ["node_modules", "dist", "scripts"] |
There was a problem hiding this comment.
the scripts all use tsx
I dont think this is a good idea, it significantly complicates the build because it introduces a cross-repo dependency at the tsx level, it's not worth it for simple helpers. |
|
here this is a simpler way to accomplish the same thing without branching in all the test scripts: #1733 |
yeah i was intentionally shooting for "don't write in the first place" behaviour, instead of "write, then remove" |
2 participants
why
what changed
packages/core/scripts/test-utils.tsto th workspace root levelscripts/test-utils.tsso that utils can be shared across other test scripts (test-core.ts,test-e2e.ts,test-server.ts,test-evals.ts)scripts/test-artifacts.tswhich handles conditionally writing test artifacts based on theTEST_COVERAGEenv varSummary by cubic
Make test coverage and JUnit/CTRF artifact generation opt-in via the TEST_COVERAGE env var to avoid writing artifacts during local runs. CI sets TEST_COVERAGE=1 to keep coverage and reports in pipelines.
New Features
Migration
Written for commit 9852ae8. Summary will update on new commits. Review in cubic