test fixes and doc work
Some checks failed
Deploy to Test Environment / deploy-to-test (push) Failing after 2m50s
Some checks failed
Deploy to Test Environment / deploy-to-test (push) Failing after 2m50s
This commit is contained in:
367
docs/adr/0057-test-remediation-post-api-versioning.md
Normal file
367
docs/adr/0057-test-remediation-post-api-versioning.md
Normal file
@@ -0,0 +1,367 @@
|
||||
# ADR-057: Test Remediation Post-API Versioning and Frontend Rework
|
||||
|
||||
**Date**: 2026-01-28
|
||||
|
||||
**Status**: Accepted
|
||||
|
||||
**Context**: Major test remediation effort completed after ADR-008 API versioning implementation and frontend style rework
|
||||
|
||||
## Context
|
||||
|
||||
Following the completion of ADR-008 Phase 2 (API Versioning Strategy) and a concurrent frontend style/design rework, the test suite experienced 105 test failures across unit tests and E2E tests. This ADR documents the systematic remediation effort, root cause analysis, and lessons learned to prevent similar issues in future migrations.
|
||||
|
||||
### Scope of Failures
|
||||
|
||||
| Test Type | Failures | Total Tests | Pass Rate After Fix |
|
||||
| ---------- | -------- | ----------- | ------------------- |
|
||||
| Unit Tests | 69 | 3,392 | 100% |
|
||||
| E2E Tests | 36 | 36 | 100% |
|
||||
| **Total** | **105** | **3,428** | **100%** |
|
||||
|
||||
### Root Causes Identified
|
||||
|
||||
The failures were categorized into six distinct categories:
|
||||
|
||||
1. **API Versioning Path Mismatches** (71 failures)
|
||||
- Test files using `/api/` instead of `/api/v1/`
|
||||
- Environment variables not set for API base URL
|
||||
- Integration and E2E tests calling unversioned endpoints
|
||||
|
||||
2. **Dark Mode Class Assertion Failures** (8 failures)
|
||||
- Frontend rework changed Tailwind dark mode utility classes
|
||||
- Test assertions checking for outdated class names
|
||||
|
||||
3. **Selected Item Styling Changes** (6 failures)
|
||||
- Component styling refactored to new design tokens
|
||||
- Test assertions expecting old CSS class combinations
|
||||
|
||||
4. **Admin-Only Component Visibility** (12 failures)
|
||||
- MainLayout tests not properly mocking admin role
|
||||
- ActivityLog component visibility tied to role-based access
|
||||
|
||||
5. **Mock Hoisting Issues** (5 failures)
|
||||
- Queue mocks not available during module initialization
|
||||
- Vitest's module hoisting order causing mock setup failures
|
||||
|
||||
6. **Error Log Path Hardcoding** (3 failures)
|
||||
- Route handlers logging hardcoded paths like `/api/flyers`
|
||||
- Test assertions expecting versioned paths `/api/v1/flyers`
|
||||
|
||||
## Decision
|
||||
|
||||
We implemented a systematic remediation approach addressing each failure category with targeted fixes while establishing patterns to prevent regression.
|
||||
|
||||
### 1. API Versioning Configuration Updates
|
||||
|
||||
**Files Modified**:
|
||||
|
||||
- `vite.config.ts`
|
||||
- `vitest.config.e2e.ts`
|
||||
- `vitest.config.integration.ts`
|
||||
|
||||
**Pattern Applied**: Centralize API base URL in Vitest environment variables
|
||||
|
||||
```typescript
|
||||
// vite.config.ts - Unit test configuration
|
||||
test: {
|
||||
env: {
|
||||
// ADR-008: Ensure API versioning is correctly set for unit tests
|
||||
VITE_API_BASE_URL: '/api/v1',
|
||||
},
|
||||
// ...
|
||||
}
|
||||
|
||||
// vitest.config.e2e.ts - E2E test configuration
|
||||
test: {
|
||||
env: {
|
||||
// ADR-008: API versioning - all routes use /api/v1 prefix
|
||||
VITE_API_BASE_URL: 'http://localhost:3098/api/v1',
|
||||
},
|
||||
// ...
|
||||
}
|
||||
|
||||
// vitest.config.integration.ts - Integration test configuration
|
||||
test: {
|
||||
env: {
|
||||
// ADR-008: API versioning - all routes use /api/v1 prefix
|
||||
VITE_API_BASE_URL: 'http://localhost:3099/api/v1',
|
||||
},
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
### 2. E2E Test URL Path Updates
|
||||
|
||||
**Files Modified** (7 files, 31 URL occurrences):
|
||||
|
||||
- `src/tests/e2e/budget-journey.e2e.test.ts`
|
||||
- `src/tests/e2e/deals-journey.e2e.test.ts`
|
||||
- `src/tests/e2e/flyer-upload.e2e.test.ts`
|
||||
- `src/tests/e2e/inventory-journey.e2e.test.ts`
|
||||
- `src/tests/e2e/receipt-journey.e2e.test.ts`
|
||||
- `src/tests/e2e/upc-journey.e2e.test.ts`
|
||||
- `src/tests/e2e/user-journey.e2e.test.ts`
|
||||
|
||||
**Pattern Applied**: Update all hardcoded API paths to versioned endpoints
|
||||
|
||||
```typescript
|
||||
// Before
|
||||
const response = await getRequest().post('/api/auth/register').send({...});
|
||||
|
||||
// After
|
||||
const response = await getRequest().post('/api/v1/auth/register').send({...});
|
||||
```
|
||||
|
||||
### 3. Unit Test Assertion Updates for UI Changes
|
||||
|
||||
**Files Modified**:
|
||||
|
||||
- `src/features/flyer/FlyerDisplay.test.tsx`
|
||||
- `src/features/flyer/FlyerList.test.tsx`
|
||||
|
||||
**Pattern Applied**: Update CSS class assertions to match new design system
|
||||
|
||||
```typescript
|
||||
// FlyerDisplay.test.tsx - Dark mode class update
|
||||
// Before
|
||||
expect(image).toHaveClass('dark:brightness-75');
|
||||
// After
|
||||
expect(image).toHaveClass('dark:brightness-90');
|
||||
|
||||
// FlyerList.test.tsx - Selected item styling update
|
||||
// Before
|
||||
expect(selectedItem).toHaveClass('ring-2', 'ring-brand-primary');
|
||||
// After
|
||||
expect(selectedItem).toHaveClass('border-brand-primary', 'bg-teal-50/50', 'dark:bg-teal-900/10');
|
||||
```
|
||||
|
||||
### 4. Admin-Only Component Test Separation
|
||||
|
||||
**File Modified**: `src/layouts/MainLayout.test.tsx`
|
||||
|
||||
**Pattern Applied**: Separate test cases for admin vs. regular user visibility
|
||||
|
||||
```typescript
|
||||
describe('for authenticated users', () => {
|
||||
beforeEach(() => {
|
||||
mockedUseAuth.mockReturnValue({
|
||||
...defaultUseAuthReturn,
|
||||
authStatus: 'AUTHENTICATED',
|
||||
userProfile: createMockUserProfile({ user: mockUser }),
|
||||
});
|
||||
});
|
||||
|
||||
it('renders auth-gated components for regular users (PriceHistoryChart, Leaderboard)', () => {
|
||||
renderWithRouter(<MainLayout {...defaultProps} />);
|
||||
expect(screen.getByTestId('price-history-chart')).toBeInTheDocument();
|
||||
expect(screen.getByTestId('leaderboard')).toBeInTheDocument();
|
||||
// ActivityLog is admin-only, should NOT be present for regular users
|
||||
expect(screen.queryByTestId('activity-log')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('renders ActivityLog for admin users', () => {
|
||||
mockedUseAuth.mockReturnValue({
|
||||
...defaultUseAuthReturn,
|
||||
authStatus: 'AUTHENTICATED',
|
||||
userProfile: createMockUserProfile({ user: mockUser, role: 'admin' }),
|
||||
});
|
||||
renderWithRouter(<MainLayout {...defaultProps} />);
|
||||
expect(screen.getByTestId('activity-log')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
### 5. vi.hoisted() Pattern for Queue Mocks
|
||||
|
||||
**File Modified**: `src/routes/health.routes.test.ts`
|
||||
|
||||
**Pattern Applied**: Use `vi.hoisted()` to ensure mocks are available during module hoisting
|
||||
|
||||
```typescript
|
||||
// Use vi.hoisted to create mock queue objects that are available during vi.mock hoisting.
|
||||
// This ensures the mock objects exist when the factory function runs.
|
||||
const { mockQueuesModule } = vi.hoisted(() => {
|
||||
// Helper function to create a mock queue object with vi.fn()
|
||||
const createMockQueue = () => ({
|
||||
getJobCounts: vi.fn().mockResolvedValue({
|
||||
waiting: 0,
|
||||
active: 0,
|
||||
failed: 0,
|
||||
delayed: 0,
|
||||
}),
|
||||
});
|
||||
|
||||
return {
|
||||
mockQueuesModule: {
|
||||
flyerQueue: createMockQueue(),
|
||||
emailQueue: createMockQueue(),
|
||||
// ... additional queues
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
// Mock the queues.server module BEFORE the health router imports it.
|
||||
vi.mock('../services/queues.server', () => mockQueuesModule);
|
||||
|
||||
// Import the router AFTER all mocks are defined.
|
||||
import healthRouter from './health.routes';
|
||||
```
|
||||
|
||||
### 6. Dynamic Error Log Paths
|
||||
|
||||
**Pattern Applied**: Use `req.originalUrl` instead of hardcoded paths in error handlers
|
||||
|
||||
```typescript
|
||||
// Before (INCORRECT - hardcoded path)
|
||||
req.log.error({ error }, 'Error in /api/flyers/:id:');
|
||||
|
||||
// After (CORRECT - dynamic path)
|
||||
req.log.error({ error }, `Error in ${req.originalUrl.split('?')[0]}:`);
|
||||
```
|
||||
|
||||
## Implementation Summary
|
||||
|
||||
### Files Modified (14 total)
|
||||
|
||||
| Category | Files | Changes |
|
||||
| -------------------- | ----- | ------------------------------------------------- |
|
||||
| Vitest Configuration | 3 | Added `VITE_API_BASE_URL` environment variables |
|
||||
| E2E Tests | 7 | Updated 31 API endpoint URLs |
|
||||
| Unit Tests | 4 | Updated assertions for UI, mocks, and admin roles |
|
||||
|
||||
### Verification Results
|
||||
|
||||
After remediation, all tests pass in the dev container environment:
|
||||
|
||||
```text
|
||||
Unit Tests: 3,392 passing
|
||||
E2E Tests: 36 passing
|
||||
Integration: 345/348 passing (3 known issues, unrelated)
|
||||
Type Check: Passing
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
|
||||
1. **Test Suite Stability**: All tests now pass consistently in the dev container
|
||||
2. **API Versioning Compliance**: Tests enforce the `/api/v1/` path requirement
|
||||
3. **Pattern Documentation**: Clear patterns established for future test maintenance
|
||||
4. **Separation of Concerns**: Admin vs. user test cases properly separated
|
||||
5. **Mock Reliability**: `vi.hoisted()` pattern prevents mock timing issues
|
||||
|
||||
### Negative
|
||||
|
||||
1. **Maintenance Overhead**: Future API version changes will require test updates
|
||||
2. **Manual Migration**: No automated tool to update test paths during versioning
|
||||
|
||||
### Neutral
|
||||
|
||||
1. **Test Execution Time**: No significant impact on test execution duration
|
||||
2. **Coverage Metrics**: Coverage percentages unchanged
|
||||
|
||||
## Best Practices Established
|
||||
|
||||
### 1. API Versioning in Tests
|
||||
|
||||
**Always use versioned API paths in tests**:
|
||||
|
||||
```typescript
|
||||
// Good
|
||||
const response = await request.get('/api/v1/users/profile');
|
||||
|
||||
// Bad
|
||||
const response = await request.get('/api/users/profile');
|
||||
```
|
||||
|
||||
**Configure environment variables centrally in Vitest configs** rather than in individual test files.
|
||||
|
||||
### 2. vi.hoisted() for Module-Level Mocks
|
||||
|
||||
When mocking modules that are imported at the top level of other modules:
|
||||
|
||||
```typescript
|
||||
// Pattern: Define mocks with vi.hoisted() BEFORE vi.mock() calls
|
||||
const { mockModule } = vi.hoisted(() => ({
|
||||
mockModule: {
|
||||
someFunction: vi.fn(),
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock('./some-module', () => mockModule);
|
||||
|
||||
// Import AFTER mocks
|
||||
import { something } from './module-that-imports-some-module';
|
||||
```
|
||||
|
||||
### 3. Testing Conditional Component Rendering
|
||||
|
||||
When testing components that render differently based on user role:
|
||||
|
||||
1. Create separate `describe` blocks for each role
|
||||
2. Set up role-specific mocks in `beforeEach`
|
||||
3. Explicitly test both presence AND absence of role-gated components
|
||||
|
||||
### 4. CSS Class Assertions After UI Refactors
|
||||
|
||||
After frontend style changes:
|
||||
|
||||
1. Review component implementation for new class names
|
||||
2. Update test assertions to match actual CSS classes
|
||||
3. Consider using partial matching for complex class combinations:
|
||||
|
||||
```typescript
|
||||
// Flexible matching for Tailwind classes
|
||||
expect(element).toHaveClass('border-brand-primary');
|
||||
// vs exact matching
|
||||
expect(element).toHaveClass('border-brand-primary', 'bg-teal-50/50', 'dark:bg-teal-900/10');
|
||||
```
|
||||
|
||||
### 5. Error Logging Paths
|
||||
|
||||
**Always use dynamic paths in error logs**:
|
||||
|
||||
```typescript
|
||||
// Pattern: Use req.originalUrl for request path logging
|
||||
req.log.error({ error }, `Error in ${req.originalUrl.split('?')[0]}:`);
|
||||
```
|
||||
|
||||
This ensures error logs reflect the actual request URL including version prefixes.
|
||||
|
||||
## Migration Checklist for Future API Version Changes
|
||||
|
||||
When implementing a new API version (e.g., v2), follow this checklist:
|
||||
|
||||
- [ ] Update `vite.config.ts` test environment `VITE_API_BASE_URL`
|
||||
- [ ] Update `vitest.config.e2e.ts` test environment `VITE_API_BASE_URL`
|
||||
- [ ] Update `vitest.config.integration.ts` test environment `VITE_API_BASE_URL`
|
||||
- [ ] Search and replace `/api/v1/` with `/api/v2/` in E2E test files
|
||||
- [ ] Search and replace `/api/v1/` with `/api/v2/` in integration test files
|
||||
- [ ] Verify route handler error logs use `req.originalUrl`
|
||||
- [ ] Run full test suite in dev container to verify
|
||||
|
||||
**Search command for finding hardcoded paths**:
|
||||
|
||||
```bash
|
||||
grep -r "/api/v1/" src/tests/
|
||||
grep -r "'/api/" src/routes/*.ts
|
||||
```
|
||||
|
||||
## Related ADRs
|
||||
|
||||
- [ADR-008](./0008-api-versioning-strategy.md) - API Versioning Strategy
|
||||
- [ADR-010](./0010-testing-strategy-and-standards.md) - Testing Strategy and Standards
|
||||
- [ADR-014](./0014-containerization-and-deployment-strategy.md) - Platform: Linux Only
|
||||
- [ADR-040](./0040-testing-economics-and-priorities.md) - Testing Economics and Priorities
|
||||
- [ADR-012](./0012-frontend-component-library-and-design-system.md) - Frontend Component Library
|
||||
|
||||
## Key Files
|
||||
|
||||
| File | Purpose |
|
||||
| ------------------------------ | -------------------------------------------- |
|
||||
| `vite.config.ts` | Unit test environment configuration |
|
||||
| `vitest.config.e2e.ts` | E2E test environment configuration |
|
||||
| `vitest.config.integration.ts` | Integration test environment configuration |
|
||||
| `src/tests/e2e/*.e2e.test.ts` | E2E test files with versioned API paths |
|
||||
| `src/routes/*.routes.test.ts` | Route test files with `vi.hoisted()` pattern |
|
||||
| `docs/development/TESTING.md` | Testing guide with best practices |
|
||||
@@ -71,6 +71,7 @@ This directory contains a log of the architectural decisions made for the Flyer
|
||||
**[ADR-040](./0040-testing-economics-and-priorities.md)**: Testing Economics and Priorities (Accepted)
|
||||
**[ADR-045](./0045-test-data-factories-and-fixtures.md)**: Test Data Factories and Fixtures (Accepted)
|
||||
**[ADR-047](./0047-project-file-and-folder-organization.md)**: Project File and Folder Organization (Proposed)
|
||||
**[ADR-057](./0057-test-remediation-post-api-versioning.md)**: Test Remediation Post-API Versioning (Accepted)
|
||||
|
||||
## 9. Architecture Patterns
|
||||
|
||||
|
||||
Reference in New Issue
Block a user