Skip to content

test: clarify repo-level test layout#170

Merged
benvinegar merged 3 commits into
mainfrom
test/clarify-test-layout
Apr 6, 2026
Merged

test: clarify repo-level test layout#170
benvinegar merged 3 commits into
mainfrom
test/clarify-test-layout

Conversation

@benvinegar

Copy link
Copy Markdown
Member

Summary

  • organize repo-level tests by intent under test/cli, test/session, test/pty, and test/smoke
  • rename files to describe the product surface under test instead of repeating folder names or leaking harness details
  • document the test layout and update scripts/docs to point at the renamed PTY and TTY suites

Why

Top-level test/ files had grown into a mix of CLI, session, PTY, and smoke coverage, but the layout and filenames did not make that intent obvious. This makes the repo-level test tree easier to scan and keeps PTY integration and TTY smoke coverage distinct.

Validation

  • bun test
  • bun run test:integration
  • bun run test:tty-smoke

@greptile-apps

greptile-apps Bot commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR reorganizes the repo-level test/ directory by intent — splitting files into test/cli/, test/session/, test/pty/, and test/smoke/ — and renames test files to reflect the product surface they cover rather than the harness they use. It also extracts the PTY test harness into a standalone test/pty/harness.ts module, adds test/README.md to document the layout, and updates package.json, AGENTS.md, and CONTRIBUTING.md to point at the new paths.

  • Reorganization is clean and consistently applied across all 11 changed files
  • package.json scripts correctly updated: test:integration./test/pty/ui-integration.test.ts, test:tty-smoketest/smoke/tty.test.ts
  • test/README.md clearly explains the intent of each subdirectory and naming conventions
  • test/pty/harness.ts extracts the PTY fixture factory cleanly, enabling ui-integration.test.ts to be smaller and purpose-focused
  • CONTRIBUTING.md validation section is missing bun run test:integration (present in AGENTS.md) — minor inconsistency that could cause contributors to skip PTY coverage
  • test/session/cli.test.ts uses hardcoded ports (48961, 48962, 48963) unlike its sibling files which call reserveLoopbackPort()

Confidence Score: 5/5

Safe to merge — pure reorganization with no logic changes; all findings are minor style/consistency issues.

Both findings are P2: one is a doc inconsistency (missing command in CONTRIBUTING.md) and the other is a pre-existing pattern not harmonized during the move (hardcoded ports). Neither affects runtime behavior or test correctness.

CONTRIBUTING.md (missing test:integration in validation snippet) and test/session/cli.test.ts (hardcoded ports vs dynamic reservation in sibling files)

Important Files Changed

Filename Overview
AGENTS.md Updated testing section with new subdirectory layout and corrected PTY test path — accurate and consistent with the new structure
CONTRIBUTING.md Added Test layout section and reference to test/README.md; validation commands snippet omits bun run test:integration
package.json Scripts correctly updated: test:integration points to test/pty/ui-integration.test.ts, test:tty-smoke to test/smoke/tty.test.ts
test/README.md New file; clearly documents intent of each test subdirectory and naming conventions
test/cli/entrypoint.test.ts CLI contract tests moved/renamed; covers help, version, pager fallback, and friendly error output — no logic changes
test/pty/harness.ts PTY harness factory cleanly extracted into its own module; exports fixture builders, launchHunk, and waitForSnapshot
test/pty/ui-integration.test.ts PTY integration tests updated to import from the new harness module; comprehensive coverage of resize, scroll, navigation, and layout
test/session/broker-e2e.test.ts Broker E2E tests moved here; correctly uses reserveLoopbackPort() for dynamic port allocation
test/session/cli.test.ts Session CLI tests moved here; uses hardcoded ports 48961/48962/48963 unlike sibling files which use dynamic port reservation
test/session/daemon.test.ts Daemon lifecycle test moved here; correctly uses reserveLoopbackPort() for dynamic port allocation
test/smoke/tty.test.ts TTY smoke tests moved/renamed; opt-in via HUNK_RUN_TTY_SMOKE=1, no logic changes

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    T[test/]
    T --> H[helpers/ shared unit-test fixtures]
    T --> C[cli/ black-box CLI contracts]
    T --> S[session/ daemon broker session-CLI]
    T --> P[pty/ live PTY-driven UI integration]
    T --> SM[smoke/ opt-in TTY transcript checks]

    C --> CE[entrypoint.test.ts]
    S --> SE[broker-e2e.test.ts]
    S --> SC[cli.test.ts]
    S --> SD[daemon.test.ts]
    P --> PH[harness.ts]
    P --> PI[ui-integration.test.ts]
    SM --> ST[tty.test.ts]
Loading

Comments Outside Diff (2)

  1. CONTRIBUTING.md, line 29-34 (link)

    P2 Missing test:integration in validation commands

    The "Validate a typical change" snippet omits bun run test:integration, which runs the PTY integration suite in test/pty/ui-integration.test.ts. AGENTS.md (and CLAUDE.md) explicitly lists it for rendering and interaction changes:

    For rendering changes: run bun run typecheck, bun test, bun run test:integration, bun run test:tty-smoke, and do one real TTY smoke run on an actual diff.

    Contributors following only CONTRIBUTING.md would skip PTY coverage. Since this PR is specifically about surfacing test intent, it's a good moment to align the two docs.

  2. test/session/cli.test.ts, line 137 (link)

    P2 Hardcoded port inconsistent with sibling files

    This file uses fixed ports 48961, 48962, and 48963 (lines 137, 196, 269), while the two sibling files added in this same PR — broker-e2e.test.ts and daemon.test.ts — both call reserveLoopbackPort() to dynamically pick a free port. Hardcoded ports can cause flaky CI failures when those ports happen to be already bound on the test machine. Now that all three files share a directory, it's a good moment to harmonize the pattern.

    Example fix (same pattern used in daemon.test.ts):

    const port = await reserveLoopbackPort();

    Note: cli.test.ts would need to import or inline a reserveLoopbackPort helper and make each test async if it isn't already.

Reviews (1): Last reviewed commit: "test: run grouped repo-level suites by f..." | Re-trigger Greptile

@benvinegar benvinegar merged commit c636903 into main Apr 6, 2026
3 checks passed
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