🧪 Improve test coverage for low-coverage modules

[Robdel12]

Summary

Adds comprehensive tests for modules that had low test coverage:

• auth/client.js: 48.57% → ~89% line coverage

  • Tests for createAuthClient factory function
  • Tests for request and authenticatedRequest methods
  • Error handling tests for 401, 429, and 500 status codes

• server-manager/index.js: 52.69% → 81.72% line coverage

  • Tests for createServerManager factory function
  • Tests for start(), stop(), getTddResults() methods
  • Tests for server getter interface

• project/operations.js: 64.83% → 100% line coverage

  • Tests for mapping CRUD operations
  • Tests for OAuth and API token-based project operations
  • Tests for fallback behavior between auth methods
  • Tests for token management operations

• api/client.js: 59.51% → ~72% line coverage

  • Tests for request method HTTP calls
  • Tests for fetch options passthrough
  • Error handling tests for various HTTP status codes

Overall coverage improved from 89.43% to 91.37%

Test plan

• All 1,722 tests pass
• No regressions in existing tests
• Coverage thresholds maintained (75% lines/functions, 70% branches)

[claude]

Code Review

I've reviewed PR #165 and the test coverage improvements look excellent! Here's my detailed feedback:

Strengths

1. Comprehensive Test Coverage

• Coverage improved from 89.43% to 91.37% - great progress toward the 92% goal
• Tests cover critical authentication and project operations paths
• Good balance of happy path and error handling tests

2. Well-Structured Tests

• Clean use of Node.js test mocking with beforeEach/afterEach for globalThis.fetch
• Proper test isolation - each test has clear setup and teardown
• Good use of helper functions (e.g., createMockMappingStore, createMockApiClient)
• Descriptive test names that clearly state what's being tested

3. Error Handling Coverage

• Tests cover various HTTP status codes (401, 403, 404, 429, 500)
• Tests verify correct error types (AuthError, VizzlyError)
• Edge cases like response body parse errors are tested

4. Follows Repository Conventions

• Tests mirror the structure of source files
• Uses existing test patterns from the codebase
• Proper use of Node.js built-in test runner

Suggestions for Improvement

1. Mock Implementation Concerns (tests/auth/client.test.js:26, tests/api/client.test.js:132)

The mock fetch implementation uses new Map() for headers, but then checks headers with .get():

headers: new Map([['content-type', 'application/json']])

However, the actual implementation in parseResponseBody does:

let contentType = response.headers.get('content-type');

This should work, but for better accuracy, consider using a more realistic Headers mock that matches the fetch API spec. Some tests use the simpler object notation which may not have a .get() method:

headers: {
  get: () => 'application/json',
}

This approach (used in tests/auth/client.test.js:157) is actually better as it's more explicit about the interface being tested.

2. Missing Test Coverage Areas

While the coverage is great, a few areas could be enhanced:

• Token refresh flow: In api/client.js, the attemptTokenRefresh function is complex but may not be fully tested through the current test suite
• Concurrent requests: The auth client doesn't test behavior with multiple simultaneous requests
• Edge case in project operations: listProjectsWithOAuth silently skips failed orgs (line 152-154), which is tested but could benefit from verification that successful orgs still return even when some fail

3. Test Helper Consistency (tests/project/operations.test.js)

The test helpers like createMockMappingStore, createMockOAuthClient, and createMockApiClient are defined within the test file. Consider:

• Documenting their interfaces at the top of the file or as JSDoc comments
• Ensuring they match the actual service interfaces closely
• Could these helpers be extracted to a shared test utilities file if they'll be reused?

4. Server Manager Tests

The createServerManagerWithDeps helper (tests/server-manager/index.test.js:272) is excellent for dependency injection testing. However:

• The helper duplicates some logic from the actual implementation
• Consider if this could be simplified by using the actual startServer/stopServer operations directly (which you already do!)

Minor Issues

1. Potential Race Condition (tests/project/operations.test.js:196)

The test for "silently skips failed org requests" uses a callCount variable to track which call fails. While this works, it's slightly fragile. Consider making the mock more explicit about which org slug should fail:

authenticatedRequest: async (endpoint) => {
  if (endpoint.includes('org1')) {
    throw new Error('Failed');
  }
  return { projects: [{ id: 'p2', name: 'Project 2' }] };
}

2. Test Assertion Specificity (tests/auth/client.test.js:91)

Several tests check error names/codes but don't verify the error message:

await assert.rejects(
  () => client.request('/api/login'),
  error => {
    assert.strictEqual(error.name, 'AuthError');
    return true;
  }
);

Consider also asserting on the error message to ensure users get helpful feedback.

Security Considerations

No security concerns identified. The tests properly:

• Don't expose real tokens or credentials
• Use mock implementations for all external calls
• Test authentication error handling correctly

Performance Considerations

• Mock implementations are lightweight and shouldn't impact test performance
• Good use of parallel test execution (no artificial dependencies between tests)
• No resource leaks detected (proper cleanup in afterEach)

Overall Assessment

This is a high-quality PR that significantly improves test coverage for critical authentication and project management code. The tests are well-written, follow best practices, and will help prevent regressions. The minor suggestions above are just opportunities for polish, not blockers.

Recommendation: Approve with minor suggestions ✅

Great work on pushing coverage from 89.43% to 91.37%! 🎉