Skip to content

test: focus existing tests on observable behavior - #311

Open
Waishnav wants to merge 3 commits into
mainfrom
codex/test-cleanup-stack
Open

test: focus existing tests on observable behavior#311
Waishnav wants to merge 3 commits into
mainfrom
codex/test-cleanup-stack

Conversation

@Waishnav

@Waishnav Waishnav commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Several tests passed without proving the behavior they described: review expansion had no empty case, runtime recovery only counted another attempt, and a configuration helper was collected as a passing test. The suite now distinguishes those outcomes, exposes runtime lifecycle scenarios by name, and drops assertions tied only to fixture configuration or an opaque ID format.

Patch fixtures and process sessions are cleaned up when assertions fail, and the repository guidance asks new tests to identify the observable contract and plausible failure they protect. This is the first PR in a two-part stack; packaged MCP workflow coverage follows in #312, which targets this branch. Merge this cleanup first.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: bc4f24f9-d3ac-42e8-bff2-ba7acd01d3ed

📥 Commits

Reviewing files that changed from the base of the PR and between fb5e2eb and 204ec79.

📒 Files selected for processing (17)
  • AGENTS.md
  • src/apply-patch.test.ts
  • src/bin-launcher.test.ts
  • src/cli-show-changes.test.ts
  • src/cli.test.ts
  • src/local-agent-profiles.test.ts
  • src/local-agent-runtime.test.ts
  • src/local-agent-store.test.ts
  • src/process-sessions.test.ts
  • src/server.test.ts
  • src/skills.test.ts
  • src/test-support/config.ts
  • src/ui/card-types.test.ts
  • src/workspace-conversation.test.ts
  • src/workspaces.test.ts
  • test/package-install-smoke.test.ts
  • tsconfig.build.json
💤 Files with no reviewable changes (1)
  • src/local-agent-store.test.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The changes improve test isolation and cleanup, add a shared configuration helper, convert local runtime scenarios to named tests, refine UI coverage, and update test guidance.

Changes

Test hygiene and support

Layer / File(s) Summary
Shared test configuration wiring
src/test-support/config.ts, src/*test.ts, test/package-install-smoke.test.ts, tsconfig.build.json
Adds writeTestDevspaceConfig, updates test imports, and excludes test-support sources from the build.
Fixture and process cleanup
src/apply-patch.test.ts, src/process-sessions.test.ts
Uses shared temporary roots and cleanup blocks for patch and process-session tests.
Local agent runtime test isolation
src/local-agent-runtime.test.ts
Registers runtime scenarios as separate node:test cases with cleanup hooks and explicit result assertions.
Focused assertions and test guidance
AGENTS.md, src/ui/card-types.test.ts, src/local-agent-store.test.ts, src/cli.test.ts
Adds test-design guidance, parameterizes card coverage, and removes assertions that no longer represent required behavior.

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

Merge Risk: ⚪ Minimal · up to 204ec

The test hygiene changes preserve existing behavior while improving cleanup and scenario isolation; no current merge-blocking risk remains.

Poem

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 14 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: updating existing tests to focus on observable behavior.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 14 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/test-cleanup-stack

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.

❤️ Share

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

@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown

Greptile Summary

This PR focuses existing tests on observable contracts and improves failure-safe cleanup.

  • Renames the shared configuration fixture so it is no longer discovered as a test and excludes test support from production builds.
  • Splits local-agent runtime lifecycle coverage into named scenarios with fresh state and registered cleanup.
  • Adds explicit empty and populated review-card expansion cases.
  • Removes assertions tied to fixture setup or opaque identifier formatting.
  • Ensures patch fixtures and process sessions are cleaned up after assertion failures.
  • Documents repository-wide guidance for behavior-oriented tests.

Confidence Score: 5/5

The PR appears safe to merge; the test refactors preserve coverage while making assertions and cleanup more robust.

No actionable failures remain: renamed helper imports resolve from source during tests, production builds do not require the helper, removed incidental assertions retain focused coverage elsewhere, and the new lifecycle cleanup follows the underlying APIs’ contracts.

Important Files Changed

Filename Overview
src/local-agent-runtime.test.ts Names and isolates runtime lifecycle scenarios, strengthens recovery assertions, and registers idempotent cleanup.
src/apply-patch.test.ts Places temporary patch fixtures under one root and removes that root in a finally block.
src/process-sessions.test.ts Widens process-manager cleanup to cover failures from every session scenario.
src/test-support/config.ts Renames a test-only configuration helper so test discovery no longer treats it as a passing test.
tsconfig.build.json Excludes test-support sources from production compilation without affecting tsx-based test resolution.
src/ui/card-types.test.ts Covers both empty and populated review-card expansion behavior.
AGENTS.md Adds guidance emphasizing observable contracts, plausible failures, fresh state, and prompt cleanup.

Reviews (1): Last reviewed commit: "test: clean up owned resources even when..." | Re-trigger Greptile

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.

1 participant