feat: Unit test POC for API and Expo layers with coverage infrastructure#1914
feat: Unit test POC for API and Expo layers with coverage infrastructure#1914andrew-bierman merged 8 commits intomainfrom
Conversation
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
The PR is complete. Here's a quick summary of what's in place: 13 test files / 142 passing tests across two layers:
New scripts:
The CI |
|
Could you clarify what you'd like me to do? Happy to make changes, answer questions, or expand the test coverage further. |
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
There was a problem hiding this comment.
Pull request overview
This PR establishes a unit test foundation for the PackRat monorepo, adding 142 passing unit tests across the API (packages/api) and Expo (apps/expo) layers. It introduces separate Vitest configurations so pure utility tests can run in a standard Node.js environment without requiring Docker or Cloudflare Workers infrastructure.
Changes:
- New Vitest unit test configs (
packages/api/vitest.unit.config.ts,apps/expo/vitest.config.ts) targeting Node.js environments, isolated from the existing Docker-dependent integration tests. - New test scripts added to
packages/api/package.json,apps/expo/package.json, and the monorepo rootpackage.json, plus@vitest/coverage-v8as a new dev dependency. - 11 new test files covering weight utilities,
computePackWeights, AI response formatting, type assertion helpers, chat context helpers, pack service, catalog service, and relative time calculation.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/api/vitest.unit.config.ts |
New standalone Vitest config for API unit tests (Node.js env, no Docker) |
apps/expo/vitest.config.ts |
New Vitest config for Expo utility unit tests |
packages/api/package.json |
Adds test:unit, test:unit:coverage scripts and @vitest/coverage-v8 dev dependency |
apps/expo/package.json |
Adds test, test:coverage scripts and @vitest/coverage-v8 + vitest dev dependencies |
package.json |
Adds monorepo-level test:api:unit and test:expo scripts |
packages/api/src/utils/__tests__/weight.test.ts |
Tests for weight conversion, base/total weight calculations |
packages/api/src/utils/__tests__/compute-pack.test.ts |
Tests for computePackWeights and computePacksWeights |
packages/api/src/utils/__tests__/format-ai-response.test.ts |
Tests for markdown stripping and text formatting |
packages/api/src/utils/__tests__/typeAssertions.test.ts |
Tests for assertDefined and assertAllDefined |
packages/api/src/utils/__tests__/chatContextHelpers.test.ts |
Tests for chat context prompt/suggestions/greeting helpers |
packages/api/src/services/__tests__/packService.test.ts |
Tests for PackService.getPackDetails and generatePacks validation |
packages/api/src/services/__tests__/catalogService.test.ts |
Tests for CatalogService.getCatalogItems input validation |
apps/expo/utils/__tests__/weight.test.ts |
Expo-flavored weight utility tests |
apps/expo/utils/__tests__/typeAssertions.test.ts |
Tests for Expo assertion helpers |
apps/expo/utils/__tests__/format-ai-response.test.ts |
Tests for Expo AI response formatter |
apps/expo/utils/__tests__/chatContextHelpers.test.ts |
Tests for Expo chat context helpers (missing getContextualGreeting) |
apps/expo/lib/utils/__tests__/compute-pack.test.ts |
Expo-flavored computePackWeights tests |
apps/expo/lib/utils/__tests__/getRelativeTime.test.ts |
Relative time bucketing tests using fake timers |
You can also share your feedback on Copilot code review. Take the survey.
| it('calls findFirst with the correct packId and userId', async () => { | ||
| mockFindFirst.mockResolvedValue(undefined); | ||
| await service.getPackDetails('pack-42'); | ||
| expect(mockFindFirst).toHaveBeenCalledOnce(); | ||
| }); |
There was a problem hiding this comment.
The test is named "calls findFirst with the correct packId and userId" but only asserts toHaveBeenCalledOnce(), verifying call count rather than the actual arguments. The test name implies checking the query arguments (packId and userId), but no such assertion is made. This should either use toHaveBeenCalledWith(...) to verify the arguments, or the test name should be updated to reflect what is actually being checked.
packages/api/vitest.unit.config.ts
Outdated
| include: [resolve(__dirname, 'src/**/*.ts')], | ||
| exclude: [ | ||
| resolve(__dirname, 'src/**/*.test.ts'), | ||
| resolve(__dirname, 'src/**/*.spec.ts'), | ||
| resolve(__dirname, 'src/index.ts'), | ||
| ], |
There was a problem hiding this comment.
Both coverage.include and coverage.exclude arrays use resolve(__dirname, 'src/**/*.ts') which produces absolute path strings like /path/to/packages/api/src/**/*.ts. Vitest's coverage fields expect relative glob patterns (e.g., 'src/**/*.ts'), not absolute paths. While this may incidentally work on some platforms (micromatch can sometimes handle absolute paths), it is non-standard and may fail on Windows or in certain CI environments. The same issue exists in apps/expo/vitest.config.ts. Use relative glob patterns instead.
apps/expo/vitest.config.ts
Outdated
| include: [resolve(__dirname, 'utils/**/*.ts'), resolve(__dirname, 'lib/utils/**/*.ts')], | ||
| exclude: [ | ||
| resolve(__dirname, 'utils/**/*.test.ts'), | ||
| resolve(__dirname, 'lib/utils/**/*.test.ts'), | ||
| ], |
There was a problem hiding this comment.
Same issue as in packages/api/vitest.unit.config.ts: coverage.include and coverage.exclude arrays use resolve(__dirname, ...) to produce absolute path strings, but Vitest coverage fields expect relative glob patterns. Use relative glob strings like 'utils/**/*.ts' and 'lib/utils/**/*.ts' instead.
| @@ -0,0 +1,57 @@ | |||
| import { describe, expect, it } from 'vitest'; | |||
| import { generatePromptWithContext, getContextualSuggestions } from '../chatContextHelpers'; | |||
There was a problem hiding this comment.
The getContextualGreeting function is exported from apps/expo/utils/chatContextHelpers.ts but is not imported or tested in this file. The corresponding API test file (packages/api/src/utils/__tests__/chatContextHelpers.test.ts) includes full coverage of getContextualGreeting. This creates an inconsistency and leaves an exported function untested.
🧩 Analysis chain🏁 Script executed: Length of output: 817 🏁 Script executed: Length of output: 55693
🔴 High Priority1.
|
| Missing scenario | Why it matters |
|---|---|
| Future timestamp (diff < 0) | Should not return negative "ago" strings |
| Exactly 30 days (week/month boundary) | Off-by-one in bucket selection |
| > 1 year ago | What does the function return? |
8. vi.useFakeTimers() called inside every test body
File: apps/expo/lib/utils/__tests__/getRelativeTime.test.ts
Calling vi.useFakeTimers() inside the test body (after vi.setSystemTime) is unusual and could be fragile if a test throws before the afterEach teardown. The conventional pattern is:
beforeEach(() => { vi.useFakeTimers(); });
afterEach(() => { vi.useRealTimers(); });
it('returns "1 hour ago"', () => {
vi.setSystemTime(new Date('2024-01-01T13:00:00Z'));
expect(getRelativeTime('2024-01-01T12:00:00Z')).toBe('1 hour ago');
});9. Module-level NOW constant in Expo compute-pack.test.ts
File: apps/expo/lib/utils/__tests__/compute-pack.test.ts, Line 9
const NOW = new Date().toISOString();This is captured once when the test module loads. It works fine for the current test data (timestamps are only used as required fields, not asserted on). Adding a brief comment clarifies intent:
// Arbitrary fixed timestamp used only as a required field value, not asserted on
const NOW = new Date().toISOString();10. packService mock returns numeric userId — verify type alignment
File: packages/api/src/services/__tests__/packService.test.ts, Line 76
get: vi.fn((key: string) => {
if (key === 'user') return { userId: 1 }; // ← number
return undefined;
}),If userId is typed as string elsewhere in the service (common in UUID-based schemas), this mock will silently pass but exercise wrong-type paths. Confirm the userId field type in the actual Hono context user object and align the mock accordingly.
✅ What's working well
- Mock isolation is solid — DB, OpenAI, and AI SDK are all mocked at the module boundary in service tests; no real I/O.
afterEach(() => vi.useRealTimers())is always present in time-sensitive tests, preventing timer leaks.- Factory helpers (
makePackItem,makePack,makeItem) are well-structured and make test data creation readable. - Parallel configs are clean — the unit config intentionally avoids the Cloudflare Workers pool for speed, which is the right call.
- Coverage reporters (
lcov+html) are CI-ready out of the box.
🧠 Learnings used
Learnt from: CR
Repo: PackRat-AI/PackRat PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-01T10:08:42.658Z
Learning: Applies to packages/api/**/*.test.{ts,tsx} : Run API tests using cd packages/api && bun test; NEVER CANCEL as it takes <1 second but may have environment errors; tests expect environment variables to be configured
Learnt from: CR
Repo: PackRat-AI/PackRat PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-01T10:08:42.658Z
Learning: Configure required CI/CD environment variables: PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN for GitHub package access, Cloudflare API tokens for API deployment, and Expo credentials for mobile builds
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@copilot CodeRabbit has provided review feedback on this PR. Please address the comments and suggestions. Once you've made the necessary changes, I'll ask CodeRabbit to re-review. |
- Use relative glob patterns in coverage.include/exclude (not resolve(__dirname,...)) in both packages/api/vitest.unit.config.ts and apps/expo/vitest.config.ts - Rename misleading packService test name to match actual toHaveBeenCalledOnce assertion - Add getContextualGreeting import and 5 tests to Expo chatContextHelpers test file Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
All four review points addressed in commit
|
|
@copilot CodeRabbit has reviewed this PR and provided feedback. Please address the review comments and push updates. Once you've made the changes, the monitor will ask CodeRabbit to re-review. |
- Scope vitest.unit.config.ts include to __tests__/ dirs only (avoids future integration test pickup in Node.js runner) - Add src/**/*.d.ts and src/db/migrations/** to coverage excludes - Move vi.useFakeTimers() to beforeEach in getRelativeTime.test.ts - Add edge-case tests: future timestamps + >1 year old timestamps - Add clarifying comment to NOW constant in compute-pack.test.ts - Add file-origin comments to both format-ai-response.test.ts files Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
Could you clarify what you'd like me to do? Happy to make further changes, answer questions, or expand test coverage. |
|
@copilot failing actions need to fix |
Deploying packrat-landing with
|
| Latest commit: |
5fc57ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3bce2ed8.packrat-landing.pages.dev |
| Branch Preview URL: | https://copilot-add-unit-test-covera.packrat-landing.pages.dev |
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Fixed in commit
|
|
@copilot check types action still failing |
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Fixed in commit |
coverage.include/coverage.excluderelative globs (previous session)getContextualGreetingtests to Expo chatContextHelpers (previous session)test.includeinvitest.unit.config.tstosrc/**/__tests__/**/*.test.tsdb/migrations/**and**/*.d.tsto coverage excludesvi.useFakeTimers()tobeforeEachingetRelativeTime.test.tsgetRelativeTime.test.tsNOWconstant in Expocompute-pack.test.tsformat-ai-response.test.tsfilespackages/api/src/utils/__tests__/weight.test.ts— alignmakeItemwith ZodPackItemtypeapps/expo/utils/__tests__/weight.test.ts— remove duplicateweight/weightUnitprops (TS2783)apps/expo/lib/utils/__tests__/compute-pack.test.ts— TS2783 + non-null assertionspackages/api/src/utils/__tests__/compute-pack.test.ts— TS2783 + non-null assertionspackages/api/src/services/__tests__/packService.test.ts—ConstructorParameters+ castspackages/api/src/services/__tests__/catalogService.test.ts—as unknown as ConstructorParameters(TS2352)Original prompt
This section details on the original issue you should resolve
<issue_title>Feature: Comprehensive Unit Test Coverage for API and Expo Layers</issue_title>
<issue_description>## Feature Request: Comprehensive Unit Test Coverage for API and Expo Layers
Problem Statement
PackRat currently has significant gaps in unit test coverage across both the API (backend) and Expo (mobile app) layers. Many core business logic paths are untested, leading to regressions and reduced confidence in deployments.
Current State
Proposed Solution
Implement comprehensive unit test suite achieving 80%+ coverage for critical paths in both layers.
API Layer Tests
Target Coverage: 80%+
Core Modules to Cover:
Services - Business logic, data transformation, external API calls
Controllers - Request/response handling, validation
Middleware - Auth, logging, rate limiting
Utilities - Helpers, formatters, validators
Database/Models - Schema validation, relationships
Testing Stack:
Sample Test Pattern:
Expo Layer Tests
Target Coverage: 75%+
Core Areas to Cover:
Hooks - Custom React hooks
Components - Reusable UI components
Screens - Page-level components
Utils/Helpers - Pure functions
State Management - Store logic
Testing Stack:
Sample Test Pattern: