Skip to content

feat(config): read ~/.testsprite/config for persistent defaults (endpoint_url, output, project_id)#179

Open
Andy00L wants to merge 2 commits into
TestSprite:mainfrom
Andy00L:feat/config-file
Open

feat(config): read ~/.testsprite/config for persistent defaults (endpoint_url, output, project_id)#179
Andy00L wants to merge 2 commits into
TestSprite:mainfrom
Andy00L:feat/config-file

Conversation

@Andy00L

@Andy00L Andy00L commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

defaultConfigPath() reserved ~/.testsprite/config but loadConfig never
read it, so every non-credential preference had to be repeated per command.
This implements the aws-cli credentials-vs-config split: profile-sectioned INI
settings (endpoint_url, output, project_id) read via a new
readConfigFileSettings(profile). Precedence is exactly as accepted in
triage — flag > env > config file > built-in default:

  • endpoint_url slots into the loadConfig cascade just above the built-in
    default (below --endpoint-url and TESTSPRITE_API_URL);
  • output becomes the default of the global --output flag (flag still wins);
  • project_id backs --project on the project-scoped test commands, with the
    validation error now naming both options.
    The file is optional by design: missing/unreadable file or section resolves to
    {} and falls through. TESTSPRITE_CONFIG_FILE overrides the path.

Related issue

Fixes #100

Type of change

  • New feature (non-breaking change that adds functionality)

Checklist

  • PR targets the main branch.
  • Commits follow Conventional Commits.
  • npm run lint and npm run format:check pass.
  • npm run typecheck passes.
  • npm test passes and coverage stays at or above the 80% gate.
  • New behavior is covered by unit tests (mock-based; no network).
  • No secrets, API keys, internal endpoints, or personal data are included.

Notes for reviewers

The INI walk is extracted from parseCredentials into a shared parseIniFile
(null-prototype accumulator + __proto__/constructor/prototype section and
key guards); parseCredentials delegates to it, byte-for-byte same behavior —
including the recent CR/LF hardening, which is untouched. No secrets ever live
in the config file (the key stays in credentials). Env-var default for
--project (TESTSPRITE_PROJECT_ID) is intentionally NOT included here — that
is issue #76; per its triage, env will slot above the config file.

Summary by CodeRabbit

  • New Features
    • Added support for reading default project, output, and endpoint settings from a user config file, including a smarter default --output value.
    • When no project is provided, the app now uses the saved project id automatically.
  • Bug Fixes
    • Improved config resolution and resilience: missing config files or unknown keys no longer break startup.
    • Hardened INI parsing to safely ignore unsafe entries.
  • Tests
    • Expanded test coverage for config-file loading, profile selection, and precedence ordering.

@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 55081f8e-e57f-41fc-b8f8-f1e353a86ff8

📥 Commits

Reviewing files that changed from the base of the PR and between 681cb57 and 55a739a.

📒 Files selected for processing (1)
  • src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/index.ts

Walkthrough

Adds a shared INI parser, reads ~/.testsprite/config for defaults, and wires config-file values into loadConfig(), the global --output default, and project id resolution in test commands.

Changes

Config file settings support

Layer / File(s) Summary
Hardened generic INI parser and credential parsing refactor
src/lib/credentials.ts
Adds parseIniFile with null-prototype accumulation and guards for dangerous section/key names, then refactors parseCredentials to delegate parsing to it.
Config file settings reader and loadConfig precedence
src/lib/config.ts, src/lib/config.test.ts
Adds ConfigFileSettings, ReadConfigFileOptions, and readConfigFileSettings(); extends LoadConfigOptions with configPath; inserts config-file endpointUrl into loadConfig() precedence; adds tests for profile parsing, missing files, env path override, and apiUrl precedence.
Output flag default from config
src/index.ts
Adds configFileOutputDefault() and uses it as the default for the global --output option.
Project id resolution from config
src/commands/test.ts
Updates requireProjectId to accept optional projectId and profile, fall back to config-file project_id, and updates runList, runCreate, and runTestRunAll to use the resolved value.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant loadConfig
  participant readConfigFileSettings
  participant ConfigFile

  CLI->>loadConfig: resolve config defaults
  loadConfig->>readConfigFileSettings: read profile settings
  readConfigFileSettings->>ConfigFile: read ~/.testsprite/config
  ConfigFile-->>readConfigFileSettings: INI values
  readConfigFileSettings-->>loadConfig: endpointUrl, output, projectId
  loadConfig-->>CLI: resolved settings
Loading

Possibly related PRs

Suggested reviewers: ruili-testsprite, zeshi-du

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The config-file support and tests are in scope, but the requested documentation table updates are missing from the shown changes. Add the DOCUMENTATION.md configuration tables describing the new precedence and supported config keys, alongside the existing code and tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is specific and matches the main change: reading ~/.testsprite/config for persistent defaults.
Out of Scope Changes check ✅ Passed The changes remain focused on config defaults, INI parsing, and related tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/config.test.ts (1)

92-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good coverage for the default profile; missing a non-default-profile case.

None of these tests exercise readConfigFileSettings/loadConfig with a profile other than default against a [profile x]-style section header — the exact scenario flagged in config.ts's readConfigFileSettings review comment. Adding a case like writeConfigFile('[profile dev]\nendpoint_url = ...\n') + readConfigFileSettings('dev', { path }) would have caught the section-naming mismatch.

🤖 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/lib/config.test.ts` around lines 92 - 140, Add a test case in the
`readConfigFileSettings + the config-file layer of loadConfig` suite that
exercises a non-default profile with a `[profile ...]` section header, since the
current coverage only verifies `[default]`. Use `writeConfigFile` and
`readConfigFileSettings` to confirm the profile-specific keys are read when the
requested profile is something like `dev`, and optionally assert `loadConfig`
respects that profile-based config path too. This should target the
`readConfigFileSettings` behavior around section-name handling so the
`default`-only assumption is covered.
🤖 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/index.ts`:
- Around line 22-32: The fallback logic in configFileOutputDefault() is using
process.env.TESTSPRITE_PROFILE too early, so it can miss an explicit --profile
value and read the wrong config section. Update the output default resolution in
src/index.ts so it runs after argv parsing or receives the active profile from
the parsed CLI state, and ensure the lookup still uses readConfigFileSettings()
plus isOutputMode() with the resolved profile name.

---

Nitpick comments:
In `@src/lib/config.test.ts`:
- Around line 92-140: Add a test case in the `readConfigFileSettings + the
config-file layer of loadConfig` suite that exercises a non-default profile with
a `[profile ...]` section header, since the current coverage only verifies
`[default]`. Use `writeConfigFile` and `readConfigFileSettings` to confirm the
profile-specific keys are read when the requested profile is something like
`dev`, and optionally assert `loadConfig` respects that profile-based config
path too. This should target the `readConfigFileSettings` behavior around
section-name handling so the `default`-only assumption is covered.
🪄 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: 6c91dc19-aa0d-4143-93e3-adf7fd100b10

📥 Commits

Reviewing files that changed from the base of the PR and between e53257d and 681cb57.

📒 Files selected for processing (5)
  • src/commands/test.ts
  • src/index.ts
  • src/lib/config.test.ts
  • src/lib/config.ts
  • src/lib/credentials.ts

Comment thread src/index.ts
@Andy00L

Andy00L commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the review finding: the config-file --output default now honors the profile selected by --profile (argv is peeked for both '--profile ' and '--profile=' spellings before Commander parses), then TESTSPRITE_PROFILE, then 'default' — matching the documented profile precedence.

@zeshi-du

zeshi-du commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Approved in design — the aws-cli credentials-vs-config split, the parseIniFile extraction with the null-prototype/proto-key hardening, and config sitting below the credentials file in the apiUrl cascade are all right. Two things before merge:

  1. Rebase over feat: support TESTSPRITE_PROJECT_ID default #144 (approved, landing first): it makes --project optional with a TESTSPRITE_PROJECT_ID env fallback via a resolveProjectId(opts, deps) helper. Fold your config lookup into that helper so the chain reads: --project flag > TESTSPRITE_PROJECT_ID env > config-file project_id. That also removes the opts.projectId = requireProjectId(...) mutation pattern.
  2. Today's wave (feat(test): JUnit XML report export for batch --wait runs #96/fix(rerun): preserve exit code and escalate auth errors in batch rerun #33/fix(steps): surface run-scoped per-step error text and stepType #167/feat(test): add "test diff <run-a> <run-b>" to isolate run-to-run regressions #168 etc.) moved test.ts, so expect mechanical conflicts there too.

The profileForDefaults() argv peek is acceptable given Commander's default-evaluation timing — keep the graceful fallback. No other changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hackathon] Read ~/.testsprite/config for persistent defaults (output, endpoint, project)

2 participants