Skip to content

Conversation

@kfirstri
Copy link
Contributor

Summary

This pr adds a infra to write tests to test the cli - including running the CLI as a new subprocess and letting it connect to a mockServer we're running

@github-project-automation github-project-automation bot moved this to Backlog in Dev Jan 13, 2026
@github-actions
Copy link
Contributor

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/[email protected]

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/[email protected]"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/[email protected]"
  }
}

Preview published to npm registry — try new features instantly!

@claude
Copy link

claude bot commented Jan 13, 2026

Pull Request Review: CLI Test Infrastructure

Summary

This PR introduces a well-designed testing infrastructure for CLI commands with a testkit pattern. The implementation is solid overall, with clear patterns and good documentation in AGENTS.md.

Strengths

Excellent Architecture

  • Testkit Pattern: The Given/When/Then pattern is clean and makes tests highly readable
  • Isolation: Each test gets its own temp directory and clean state via beforeEach/afterEach
  • Shared Mock Server: Smart singleton pattern for the HTTP mock server reduces overhead
  • Good Separation: CLITestkit, MockServer, and CLIResultMatcher are well-separated concerns

Documentation

  • AGENTS.md is exceptionally thorough with clear examples, API reference, and patterns
  • Test code is self-documenting with clear comments
  • The naming convention is documented and consistent

Test Coverage

  • Good coverage of happy paths and error cases
  • Tests check both stdout/stderr content and exit codes
  • Auth file verification in login tests is thorough

Code Quality Issues

1. Error Handling: Silent Catch Block (tests/cli/testkit/CLITestkit.ts:110-111)
Silent catch blocks can hide real errors. If the file exists but contains invalid JSON, this will silently return null.

Recommendation: Differentiate between missing file and parse errors by checking error.code === 'ENOENT' before returning null, and re-throw other errors.

2. Error Handling: Route Handler Errors (tests/cli/testkit/MockServer.ts:60-63)
Converting errors to strings loses stack traces during debugging.

Recommendation: Log errors with console.error and preserve stack traces in the response.

3. Type Safety: Unsafe Type Assertion (tests/cli/testkit/MockServer.ts:52)
Assumes the HTTP method is valid without validation.

Recommendation: Add validation for HTTP methods before using them.

4. Mock Server Route Reset (tests/cli/testkit/MockServer.ts:68-73)
The resetRoutes() implementation may not properly clear old routes. Creating a new Polka instance and adding it as a new listener might not replace the old handler completely.

Recommendation: Validate this behavior works as expected with a test.

Performance Considerations

Build Performance (package.json:21)
The pretest script rebuilds the entire project before every test run, which can be slow during development.

Recommendation: Consider adding a test:dev script that skips the build for faster iterations.

Test Timeout - Good! 10 seconds is reasonable for integration tests that spawn processes.

Temp Directory Cleanup - Good use of afterEach to clean up per-test resources.

Security Concerns

All good here:

  • Environment variable isolation properly configured
  • Mock server binds to localhost with random port (secure)
  • No secrets in fixtures

Test Coverage Assessment

Current Coverage:

  • Login flow (device code, token exchange, userinfo)
  • Logout (both logged in and logged out states)
  • Whoami (success and failure cases)
  • Entities push (various error cases and success)
  • Show project (success, missing project, invalid config)
  • Create command (only help text - noted as limitation)

Missing Test Cases:

  1. Token expiry and refresh scenarios
  2. Login failure scenarios (device code timeout, user denial, network errors)
  3. Entities push edge cases (malformed entity files, network failures)
  4. Mock server edge cases (route parameter extraction, 404 scenarios)
  5. Error recovery (temp directory creation failures)

Dependencies Review

All new dependencies are appropriate:

Suggested Improvements

High Priority:

  1. Fix error handling in readAuthFile() to differentiate between missing file and parse errors
  2. Validate the resetRoutes() behavior works as expected
  3. Add error logging in mock server route handler

Medium Priority:

  1. Add more error scenario test coverage
  2. Improve error messages in CLIResultMatcher
  3. Consider adding test data builders

Low Priority:

  1. Update polka to a newer version
  2. Add JSDoc comments to public methods

Approval Recommendation

Overall Assessment: Approve with minor changes

This is excellent work! The testing infrastructure is well-designed, the documentation is thorough, and the patterns are clean. The issues identified are mostly minor and can be addressed in follow-up PRs.

Before Merging:

  1. Must Fix: Error handling in readAuthFile() (correctness)
  2. Should Fix: Add error logging in mock server route handler (debugging)
  3. Nice to Have: Validate resetRoutes() behavior

After Merging:

  • Add more error scenario coverage
  • Consider token refresh testing
  • Add test data builders for reusability

Great job on this infrastructure! It provides a solid foundation for comprehensive CLI testing.


Reviewed by: Claude (Sonnet 4.5)
Review Date: 2026-01-13

@kfirstri kfirstri moved this from Backlog to In progress in Dev Jan 15, 2026
@kfirstri kfirstri moved this from In progress to Ready in Dev Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

2 participants