feat(test): GitHub-native CI output for "test run --all" (--gh-output, --summary-file)#264
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe batch test command now supports GitHub Actions summaries and annotations plus reduced machine-readable summary files. New helpers normalize accepted test results, render Markdown, emit annotations, and validate CI and off-CI behavior. ChangesBatch CI outputs
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runTestRunAll
participant summarizeAcceptedPayload
participant emitGithubOutputs
participant GitHubActions
CLI->>runTestRunAll: batch options
runTestRunAll->>summarizeAcceptedPayload: captured JSON payload
summarizeAcceptedPayload-->>runTestRunAll: reduced CiSummary
runTestRunAll->>emitGithubOutputs: summary and environment
emitGithubOutputs->>GitHubActions: step summary and error annotations
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/commands/ci.ts`:
- Around line 93-143: Update renderJobSummaryMarkdown to escape Markdown
table-cell values for testId, runId, and dashboardUrl, including pipe characters
and newlines. Update emitGithubOutputs to percent-encode workflow-command
parameters and data for testId, error, and dashboardUrl before writing ::error
messages. Add regression coverage exercising pipe, percent, and newline inputs
across both output formats.
- Around line 196-266: Update createCiCommand so --project either uses the
existing project-id resolution flow for TESTSPRITE_PROJECT_ID and config
project_id, or remove those fallback claims from its help text. Replace the
hardcoded max-concurrency upper bound and corresponding help text value with the
shared MAX_BATCH_CONCURRENCY constant from test.ts, keeping validation and
documentation aligned.
- Around line 268-273: Remove the local parseRequestTimeoutFlag helper and
update the ci command’s --request-timeout handling to call the shared
request-timeout parser used by other commands. Preserve the central parser’s
validation-error behavior for invalid or non-positive values instead of silently
falling back to the default timeout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1463b67-81e6-4117-b323-bda179b6b972
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (3)
src/commands/ci.test.tssrc/commands/ci.tssrc/index.ts
| export function renderJobSummaryMarkdown(summary: CiSummary): string { | ||
| return [ | ||
| '## TestSprite results', | ||
| '', | ||
| `**${summary.passed}/${summary.total} passed** (${summary.failed} failed, ${summary.timedOut} timed out)`, | ||
| '', | ||
| '| Test | Status | Run |', | ||
| '| --- | --- | --- |', | ||
| ...summary.runs.map( | ||
| row => | ||
| `| ${row.testId} | ${row.status} | ${ | ||
| row.dashboardUrl ? `[dashboard](${row.dashboardUrl})` : (row.runId ?? '') | ||
| } |`, | ||
| ), | ||
| '', | ||
| ].join('\n'); | ||
| } | ||
|
|
||
| /** | ||
| * Emit the GitHub-native surfaces. Self-gating on the standard env vars: | ||
| * `$GITHUB_STEP_SUMMARY` (a file path Actions provides) receives the Markdown | ||
| * table; `GITHUB_ACTIONS=true` enables one `::error::` workflow command per | ||
| * non-passed run on stdout (Actions parses workflow commands from stdout). | ||
| * Both writes are best-effort: a broken summary file must not mask the gate. | ||
| */ | ||
| export function emitGithubOutputs( | ||
| summary: CiSummary, | ||
| env: NodeJS.ProcessEnv, | ||
| sinks: { | ||
| stdout: (line: string) => void; | ||
| stderr: (line: string) => void; | ||
| appendFile: (path: string, content: string) => void; | ||
| }, | ||
| ): void { | ||
| const summaryPath = env.GITHUB_STEP_SUMMARY; | ||
| if (typeof summaryPath === 'string' && summaryPath.length > 0) { | ||
| try { | ||
| sinks.appendFile(summaryPath, renderJobSummaryMarkdown(summary)); | ||
| } catch { | ||
| sinks.stderr('[ci] could not append to GITHUB_STEP_SUMMARY; continuing'); | ||
| } | ||
| } | ||
| if (env.GITHUB_ACTIONS === 'true') { | ||
| for (const row of summary.runs) { | ||
| if (row.status === 'passed') continue; | ||
| const detail = row.error !== undefined ? ` ${row.error}` : ''; | ||
| const link = row.dashboardUrl !== undefined ? ` ${row.dashboardUrl}` : ''; | ||
| sinks.stdout(`::error title=TestSprite ${row.testId}::status=${row.status}${detail}${link}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'File list:\n'
git ls-files | rg '^src/commands/ci\.ts$|^src/commands/ci\.test\.ts$|^DOCUMENTATION\.md$'
printf '\nOutline for src/commands/ci.ts:\n'
ast-grep outline src/commands/ci.ts --view expanded || true
printf '\nRelevant excerpts from src/commands/ci.ts:\n'
sed -n '1,240p' src/commands/ci.ts
printf '\nRelevant excerpts from src/commands/ci.test.ts:\n'
if [ -f src/commands/ci.test.ts ]; then
sed -n '1,260p' src/commands/ci.test.ts
else
echo 'No src/commands/ci.test.ts found'
fi
printf '\nSearch for escaping helpers / workflow command handling:\n'
rg -n "escapeWorkflow|GITHUB_STEP_SUMMARY|::error|markdown|dashboardUrl|testId|runId" src/commands src/lib || true
printf '\nRelevant docs snippets:\n'
rg -n "Signals & pipes|Output & scripting|workflow commands|step summary|GITHUB_STEP_SUMMARY|GITHUB_ACTIONS" DOCUMENTATION.md || trueRepository: TestSprite/testsprite-cli
Length of output: 50382
Escape CI summary and workflow-command values
testId, runId, and dashboardUrl go straight into the Markdown table, and testId, error, and dashboardUrl go straight into ::error. Escape table cells and encode workflow-command data/parameters before emitting them; add a regression test for |, %, and newline inputs.
🤖 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 `@src/commands/ci.ts` around lines 93 - 143, Update renderJobSummaryMarkdown to
escape Markdown table-cell values for testId, runId, and dashboardUrl, including
pipe characters and newlines. Update emitGithubOutputs to percent-encode
workflow-command parameters and data for testId, error, and dashboardUrl before
writing ::error messages. Add regression coverage exercising pipe, percent, and
newline inputs across both output formats.
| /** Default overall deadline for the CI gate, in seconds (mirrors run --all). */ | ||
| const CI_DEFAULT_TIMEOUT_SECONDS = 600; | ||
| /** Default poll fan-out bound (mirrors run --all's --max-concurrency). */ | ||
| const CI_DEFAULT_MAX_CONCURRENCY = 50; | ||
|
|
||
| export function createCiCommand(deps: CiDeps = {}): Command { | ||
| return new Command('ci') | ||
| .description( | ||
| 'One-shot CI gate: run the project suite, gate on the exit code, and emit CI-native output (machine summary; GitHub job summary + ::error:: PR annotations when on Actions)', | ||
| ) | ||
| .option('--project <id>', 'project id (or TESTSPRITE_PROJECT_ID / config project_id)') | ||
| .option( | ||
| '--timeout <s>', | ||
| `overall deadline in seconds (1-3600, default ${CI_DEFAULT_TIMEOUT_SECONDS})`, | ||
| ) | ||
| .option( | ||
| '--max-concurrency <n>', | ||
| `max concurrent run polls (1-100, default ${CI_DEFAULT_MAX_CONCURRENCY})`, | ||
| ) | ||
| .option('--summary-file <path>', 'also write the machine summary JSON to this file') | ||
| .addHelpText( | ||
| 'after', | ||
| '\nExamples:\n' + | ||
| ' testsprite ci --project proj_123 # gate a pipeline step\n' + | ||
| ' testsprite ci --summary-file testsprite.json # keep the summary as a build artifact', | ||
| ) | ||
| .addHelpText('after', GLOBAL_OPTS_HINT) | ||
| .action( | ||
| async ( | ||
| cmdOpts: { | ||
| project?: string; | ||
| timeout?: string; | ||
| maxConcurrency?: string; | ||
| summaryFile?: string; | ||
| }, | ||
| command: Command, | ||
| ) => { | ||
| const globals = command.optsWithGlobals() as Partial<FactoryCommonOptions> & { | ||
| requestTimeout?: string; | ||
| }; | ||
| const timeoutSeconds = | ||
| cmdOpts.timeout === undefined ? CI_DEFAULT_TIMEOUT_SECONDS : Number(cmdOpts.timeout); | ||
| if (!Number.isInteger(timeoutSeconds) || timeoutSeconds < 1 || timeoutSeconds > 3600) { | ||
| throw localValidationError('timeout', 'must be an integer between 1 and 3600'); | ||
| } | ||
| const maxConcurrency = | ||
| cmdOpts.maxConcurrency === undefined | ||
| ? CI_DEFAULT_MAX_CONCURRENCY | ||
| : Number(cmdOpts.maxConcurrency); | ||
| if (!Number.isInteger(maxConcurrency) || maxConcurrency < 1 || maxConcurrency > 100) { | ||
| throw localValidationError('max-concurrency', 'must be an integer between 1 and 100'); | ||
| } | ||
| await runCi( | ||
| { | ||
| profile: globals.profile ?? 'default', | ||
| output: globals.output ?? 'text', | ||
| endpointUrl: globals.endpointUrl, | ||
| debug: globals.debug ?? false, | ||
| verbose: globals.verbose ?? false, | ||
| dryRun: globals.dryRun ?? false, | ||
| requestTimeoutMs: parseRequestTimeoutFlag(globals.requestTimeout), | ||
| projectId: cmdOpts.project, | ||
| timeoutSeconds, | ||
| maxConcurrency, | ||
| summaryFile: cmdOpts.summaryFile, | ||
| }, | ||
| deps, | ||
| ); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate how --project resolves TESTSPRITE_PROJECT_ID / config elsewhere.
rg -n "TESTSPRITE_PROJECT_ID" -C3 src
rg -nP "\-\-project\s*<" -C5 src/commands/test.tsRepository: TestSprite/testsprite-cli
Length of output: 14872
🏁 Script executed:
#!/bin/bash
rg -n "MAX_BATCH_CONCURRENCY" srcRepository: TestSprite/testsprite-cli
Length of output: 482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the CI command and the batch runner it calls.
sed -n '1,260p' src/commands/ci.ts
printf '\n---\n'
sed -n '6300,7090p' src/commands/test.ts
# Find any shared project-id resolution helpers used by ci.
printf '\n=== project-id resolution search ===\n'
rg -n "requireProjectId|projectId.*env|TESTSPRITE_PROJECT_ID|config project_id|default.*project" src/commands src/lib src -g '!**/*.map'Repository: TestSprite/testsprite-cli
Length of output: 47303
--project fallback and concurrency bound drift
--projecthere doesn’t resolveTESTSPRITE_PROJECT_IDor config; either wire in the same project-id resolution used elsewhere or drop the fallback from the help text.- Reuse
MAX_BATCH_CONCURRENCYfromsrc/commands/test.tsinstead of hardcoding100so the--max-concurrencyhelp text and validation stay aligned.
🤖 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 `@src/commands/ci.ts` around lines 196 - 266, Update createCiCommand so
--project either uses the existing project-id resolution flow for
TESTSPRITE_PROJECT_ID and config project_id, or remove those fallback claims
from its help text. Replace the hardcoded max-concurrency upper bound and
corresponding help text value with the shared MAX_BATCH_CONCURRENCY constant
from test.ts, keeping validation and documentation aligned.
| function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { | ||
| if (raw === undefined) return undefined; | ||
| const seconds = Number(raw); | ||
| if (!Number.isFinite(seconds) || seconds <= 0) return undefined; | ||
| return Math.round(seconds * 1000); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether a shared --request-timeout parser/clamp already exists.
rg -n "requestTimeout" -C3 src/lib/client-factory.ts
rg -n "REQUEST_TIMEOUT_MIN_MS|REQUEST_TIMEOUT_MAX_MS" srcRepository: TestSprite/testsprite-cli
Length of output: 3563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/commands/ci.ts (timeout-related lines) ---'
sed -n '240,320p' src/commands/ci.ts
echo
echo '--- src/lib/client-factory.ts (resolveRequestTimeoutMs and clamp) ---'
sed -n '110,170p' src/lib/client-factory.ts
echo
echo '--- search for parseRequestTimeoutFlag and requestTimeoutMs flow ---'
rg -n "parseRequestTimeoutFlag|requestTimeoutMs|request-timeout|REQUEST_TIMEOUT_MIN_MS|REQUEST_TIMEOUT_MAX_MS" src/commands src/lib -C2Repository: TestSprite/testsprite-cli
Length of output: 41988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- request-timeout handling in ci.ts ---'
grep -n -C4 "parseRequestTimeoutFlag" src/commands/ci.ts || true
echo
echo '--- comparable flag parsers in the CLI ---'
rg -n "function parse.*Timeout|parse.*Concurrency|Invalid.*timeout|must be a positive" src/commands src/lib -C2Repository: TestSprite/testsprite-cli
Length of output: 6288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/commands/ci.ts | nl -ba | sed -n '250,310p'
echo
sed -n '110,170p' src/lib/client-factory.ts | nl -ba
echo
rg -n "parseTimeout|timeout.*validate|max-concurrency|positive integer|clamp" src/commands src/lib -C2Repository: TestSprite/testsprite-cli
Length of output: 203
Use the shared request-timeout parser here instead of the local helper. Invalid or non-positive --request-timeout values are currently ignored and fall back to the default timeout, while the central parser raises a validation error; that makes ci behave differently from the other commands.
🤖 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 `@src/commands/ci.ts` around lines 268 - 273, Remove the local
parseRequestTimeoutFlag helper and update the ci command’s --request-timeout
handling to call the shared request-timeout parser used by other commands.
Preserve the central parser’s validation-error behavior for invalid or
non-positive values instead of silently falling back to the default timeout.
|
Maintainer verdict: the engineering is good, but a new top-level Context, restated from #99 (07-18): the CI close-out ships as an official GitHub Action (in progress internally), not a new CLI subcommand. A permanent top-level command is a long-term maintenance commitment (the concern raised in #99's original triage), and What we do want from this PR — the CI-native output, which is the genuinely missing half:
Reshape: attach those to the existing One defect to fix in the same pass: the JSON summary writes to stdout unconditionally, ignoring the global Happy to re-review fast — the test discipline here is exactly what we want to merge. |
…output, --summary-file)
Andy00L
changed the title
|
Reshaped as requested: the top-level command is gone; the reducer/renderer/emitter moved to src/lib/gh-output.ts and hook into |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/commands/test.ts`:
- Around line 9221-9234: Extend the validation around the existing ghOutput and
summaryFile checks to reject both flags when --wait is not enabled, using
localValidationError and the existing validation exit code. Apply this to the
--all path while preserving the current single-id restrictions, so CI-result
artifacts are only accepted for test run --all --wait.
In `@src/lib/gh-output.ts`:
- Around line 114-119: Keep GitHub annotations off JSON stdout: update the
annotation loop in gh-output.ts to use an output-mode-aware auxiliary sink,
revise gh-output.test.ts so annotations are not treated as JSON-command stdout,
and update test.ts to avoid wiring annotations to process.stdout when --output
json is active. Add or update the integration assertion ensuring JSON stdout
remains parseable.
- Around line 40-60: Update summarizeAcceptedPayload to validate the JSON.parse
result is a non-null record before accessing accepted, and filter or safely skip
accepted entries that are null or not records before reading row.error or other
fields. Preserve valid-row mapping behavior and add regression coverage for JSON
null and accepted arrays containing null entries.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ecc7b5ec-6909-43a3-9826-dc987e24dbdd
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (3)
src/commands/test.tssrc/lib/gh-output.test.tssrc/lib/gh-output.ts
|
CI red root-caused: the three failing run-spec tests inherited the runner's real GITHUB_ACTIONS=true, which (correctly) activated the new gh-output layer and appended ::error:: lines to the stdout they JSON.parse. Fixed by injecting an empty env into those deps; verified locally under GITHUB_ACTIONS=true (full suite 2023 green). |
… harden the payload reducer
|
All three review points addressed:
|
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
Re-reviewed after the reshape — this is exactly what was asked: no new top-level command, --gh-output/--summary-file attached to test run --all --wait with GITHUB_ACTIONS auto-enable, artifact still written on a failing gate, stdout contract kept clean. Fast turnaround appreciated.
2 participants
Reshaped per the maintainer review: the top-level
cicommand is gone; the CI-native output layer now attaches to the existingtest run --allpath.src/lib/gh-output.tskeeps the reducer (summarizeAcceptedPayload), the$GITHUB_STEP_SUMMARYMarkdown renderer, and the::error::annotations emitter, with their unit tests moved tosrc/lib/gh-output.test.ts.test run --allgains--gh-output(force the GitHub-native surfaces even off-Actions) and--summary-file <path>(write the reduced{total, passed, failed, timedOut, runs[]}JSON artifact). Auto-enabled whenGITHUB_ACTIONS=true, so the official Action gets all of it for free by invokingtest run --all --wait.--outputcontract defect fixed by construction: nothing extra is written to stdout (the envelope owns it; workflow commands are only emitted under Actions or explicit--gh-output); the summary file is a machine artifact written in both output modes.--all-only and rejected loudly otherwise (same rule as--filter).Closes #99 CLI-side per the direction note (the one-shot entrypoint ships as the official GitHub Action).
Discord: interferon0
Summary by CodeRabbit
testsprite test run --all --wait, including workflow error annotations and a$GITHUB_STEP_SUMMARYjob summary.--gh-outputand--summary-file(with--summary-filewriting a reduced JSON summary).