163 lines
5.0 KiB
Markdown
163 lines
5.0 KiB
Markdown
# [Test Quality] Fix weak assertions in certificates.spec.ts
|
|
|
|
**Created:** February 3, 2026
|
|
**Status:** Open
|
|
**Priority:** Low
|
|
**Labels:** test-quality, technical-debt, low-priority
|
|
**Milestone:** Post-Phase 2 cleanup
|
|
|
|
---
|
|
|
|
## Description
|
|
|
|
Two tests in `certificates.spec.ts` have weak assertions that always pass, which were identified during Phase 2.1 Supervisor code review (PR #1 checkpoint feedback).
|
|
|
|
### Affected Tests
|
|
|
|
#### 1. **"should display empty state when no certificates exist"** (line 93-106)
|
|
- **Current:** `expect(hasEmptyMessage || hasTable).toBeTruthy()` (always passes)
|
|
- **Issue:** Assertion is a logical OR that will pass if either condition is true, making it impossible to fail
|
|
- **Fix:** Explicit assertions with database cleanup in beforeEach
|
|
|
|
#### 2. **"should show loading spinner while fetching data"** (line 108-122)
|
|
- **Current:** `expect(hasTable || hasEmpty).toBeTruthy()` (always passes)
|
|
- **Issue:** Same logical OR pattern that cannot fail
|
|
- **Fix:** Test isolation and explicit state checks
|
|
|
|
---
|
|
|
|
## Root Cause
|
|
|
|
**Database State Dependency:** Tests assume a clean database or pre-populated state that may not exist in CI or after other tests run.
|
|
|
|
**Weak Assertion Pattern:** Using `||` (OR) with `.toBeTruthy()` creates assertions that always pass as long as one condition is met, even if the actual test intent is not validated.
|
|
|
|
---
|
|
|
|
## Action Items
|
|
|
|
- [ ] **Add database cleanup in beforeEach hook**
|
|
- Clear certificates table before each test
|
|
- Ensure known starting state
|
|
|
|
- [ ] **Replace `.toBeTruthy()` with explicit state checks**
|
|
- Empty state test: `expect(emptyMessage).toBeVisible()` AND `expect(table).not.toBeVisible()`
|
|
- Loading test: `expect(spinner).toBeVisible()` followed by `expect(spinner).not.toBeVisible()`
|
|
|
|
- [ ] **Use `test.skip()` or mark as flaky until fixed**
|
|
- Document why tests are skipped
|
|
- Track in this issue
|
|
|
|
- [ ] **Audit PR 2/3 files for similar patterns**
|
|
- Search for `.toBeTruthy()` usage in:
|
|
- `proxy-hosts.spec.ts` (PR #2)
|
|
- `access-lists-crud.spec.ts` (PR #3)
|
|
- `authentication.spec.ts` (PR #3)
|
|
- Document any additional weak assertions found
|
|
|
|
---
|
|
|
|
## Example Fix
|
|
|
|
**Before (Weak):**
|
|
```typescript
|
|
test('should display empty state when no certificates exist', async ({ page }) => {
|
|
await test.step('Check for empty state or existing certificates', async () => {
|
|
const emptyMessage = page.getByText(/no certificates/i);
|
|
const table = page.getByRole('table');
|
|
|
|
const hasEmptyMessage = await emptyMessage.isVisible().catch(() => false);
|
|
const hasTable = await table.isVisible().catch(() => false);
|
|
|
|
expect(hasEmptyMessage || hasTable).toBeTruthy(); // ❌ Always passes
|
|
});
|
|
});
|
|
```
|
|
|
|
**After (Strong):**
|
|
```typescript
|
|
test.describe('Empty State Tests', () => {
|
|
test.beforeEach(async ({ request }) => {
|
|
// Clear certificates from database
|
|
await request.delete('/api/v1/certificates/all');
|
|
});
|
|
|
|
test('should display empty state when no certificates exist', async ({ page }) => {
|
|
await page.goto('/certificates');
|
|
await waitForLoadingComplete(page);
|
|
|
|
const emptyMessage = page.getByText(/no certificates/i);
|
|
const table = page.getByRole('table');
|
|
|
|
// ✅ Explicit assertions
|
|
await expect(emptyMessage).toBeVisible();
|
|
await expect(table).not.toBeVisible();
|
|
});
|
|
});
|
|
```
|
|
|
|
---
|
|
|
|
## E2E Test Failures (Phase 2.4 Validation)
|
|
|
|
These tests failed during full browser suite execution:
|
|
|
|
**Chromium:**
|
|
- ❌ `certificates.spec.ts:93` - empty state test
|
|
- ❌ `certificates.spec.ts:108` - loading spinner test
|
|
|
|
**Firefox:**
|
|
- ❌ `certificates.spec.ts:93` - empty state test
|
|
- ❌ `certificates.spec.ts:108` - loading spinner test
|
|
|
|
**Error Message:**
|
|
```
|
|
Error: expect(received).toBeTruthy()
|
|
Received: false
|
|
```
|
|
|
|
---
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] Both tests pass consistently in all 3 browsers (Chromium, Firefox, WebKit)
|
|
- [ ] Tests fail when expected conditions are not met (e.g., database has certificates)
|
|
- [ ] Database cleanup is documented and runs before each test
|
|
- [ ] Similar weak assertion patterns audited in PR 2/3 files
|
|
- [ ] Tests are no longer marked as skipped/flaky
|
|
|
|
---
|
|
|
|
## Priority Rationale
|
|
|
|
**Low Priority:** These tests are not blocking Phase 2 completion or causing CI failures. They are documentation/technical debt issues that should be addressed in post-Phase 2 cleanup.
|
|
|
|
**Impact:**
|
|
- Tests currently pass but do not validate actual behavior
|
|
- False sense of security (tests can't fail even when functionality is broken)
|
|
- Future maintenance challenges if assumptions change
|
|
|
|
---
|
|
|
|
## Related
|
|
|
|
- **Phase 2 Triage:** `docs/plans/browser_alignment_triage.md`
|
|
- **Supervisor Feedback:** PR #1 code review checkpoint
|
|
- **Test Files:**
|
|
- `tests/core/certificates.spec.ts` (lines 93-122)
|
|
- `tests/core/proxy-hosts.spec.ts` (to be audited)
|
|
- `tests/core/access-lists-crud.spec.ts` (to be audited)
|
|
- `tests/core/authentication.spec.ts` (to be audited)
|
|
|
|
---
|
|
|
|
## Timeline
|
|
|
|
**Estimated Effort:** 2-3 hours
|
|
- Investigation: 30 minutes
|
|
- Fix implementation: 1 hour
|
|
- Testing and validation: 1 hour
|
|
- Audit PR 2/3 files: 30 minutes
|
|
|
|
**Target Completion:** TBD (post-Phase 2)
|