207 lines
7.4 KiB
Markdown
207 lines
7.4 KiB
Markdown
# Dialog Opening Issue - Root Cause Analysis & Fixes
|
|
|
|
## Problem Statement
|
|
**7 E2E tests were failing because dialogs/forms were not opening**
|
|
|
|
The tests expected elements like `getByTestId('template-name')` to exist in the DOM, but they never appeared because the dialogs were never opening.
|
|
|
|
**Error Pattern:**
|
|
```
|
|
Error: expect(locator).toBeVisible() failed
|
|
Locator: getByTestId('template-name')
|
|
Expected: visible
|
|
Timeout: 5000ms
|
|
Error: element(s) not found
|
|
```
|
|
|
|
## Root Cause Analysis
|
|
|
|
### Issue 1: Not a Real Dialog
|
|
The template management UI in `frontend/src/pages/Notifications.tsx` **does NOT use a modal dialog**. Instead:
|
|
- It uses **conditional rendering** with a React state variable `managingTemplates`
|
|
- When `managingTemplates` is `true`, the form renders inline in a `<Card>` component
|
|
- The form elements are plain HTML, not inside a dialog/modal
|
|
|
|
### Issue 2: Button Selection Problems
|
|
The original tests tried to click buttons without properly verifying they existed first:
|
|
```typescript
|
|
// WRONG: May not find the button or find the wrong one
|
|
const manageButton = page.getByRole('button', { name: /manage.*templates|new.*template/i });
|
|
await manageButton.first().click();
|
|
```
|
|
|
|
Problems:
|
|
- Multiple buttons could match the regex pattern
|
|
- Button might not be visible yet
|
|
- No fallback if button wasn't found
|
|
- No verification that clicking actually opened the form
|
|
|
|
### Issue 3: Missing Test IDs in Implementation
|
|
The `TemplateForm` component in the React code has **no test IDs** on its inputs:
|
|
```tsx
|
|
// FROM Notifications.tsx - TemplateForm component
|
|
<input {...register('name', { required: true })} className="mt-1 block w-full rounded-md" />
|
|
// ☝️ NO data-testid="template-name" - this is why tests failed!
|
|
```
|
|
|
|
The tests expected:
|
|
```typescript
|
|
const nameInput = page.getByTestId('template-name'); // NOT IN DOM!
|
|
```
|
|
|
|
## Solution Implemented
|
|
|
|
### 1. Updated Test Strategy
|
|
Instead of relying on test IDs that don't exist, the tests now:
|
|
- Verify the template management section is visible (`h2` with "External Templates" text)
|
|
- Use fallback button selection logic
|
|
- Wait for form inputs to appear using DOM queries (inputs, selects, textareas)
|
|
- Use role-based and generic selectors instead of test IDs
|
|
|
|
### 2. Explicit Button Finding with Fallbacks
|
|
```typescript
|
|
await test.step('Click New Template button', async () => {
|
|
const allButtons = page.getByRole('button');
|
|
let found = false;
|
|
|
|
// Try primary pattern
|
|
const newTemplateBtn = allButtons.filter({ hasText: /new.*template|create.*template/i }).first();
|
|
if (await newTemplateBtn.isVisible({ timeout: 3000 }).catch(() => false)) {
|
|
await newTemplateBtn.click();
|
|
found = true;
|
|
} else {
|
|
// Fallback: Find buttons in template section and click the last one
|
|
const templateMgmtButtons = page.locator('div').filter({ hasText: /external.*templates/i }).locator('button');
|
|
const createButton = templateMgmtButtons.last();
|
|
if (await createButton.isVisible({ timeout: 3000 }).catch(() => false)) {
|
|
await createButton.click();
|
|
found = true;
|
|
}
|
|
}
|
|
|
|
expect(found).toBeTruthy();
|
|
});
|
|
```
|
|
|
|
### 3. Generic Form Element Selection
|
|
```typescript
|
|
await test.step('Fill template form', async () => {
|
|
// Use generic selectors that don't depend on test IDs
|
|
const nameInput = page.locator('input[type="text"]').first();
|
|
await nameInput.fill(templateName);
|
|
|
|
const selects = page.locator('select');
|
|
if (await selects.first().isVisible({ timeout: 2000 }).catch(() => false)) {
|
|
await selects.first().selectOption('custom');
|
|
}
|
|
|
|
const textareas = page.locator('textarea');
|
|
const configTextarea = textareas.first();
|
|
if (await configTextarea.isVisible({ timeout: 2000 }).catch(() => false)) {
|
|
await configTextarea.fill('{"custom": "..."}');
|
|
}
|
|
});
|
|
```
|
|
|
|
## Tests Fixed
|
|
|
|
### Template Management Tests (3 tests)
|
|
1. ✅ **Line 683: should create custom template**
|
|
- Fixed button selection logic
|
|
- Wait for form inputs instead of test IDs
|
|
- Added fallback button-finding strategy
|
|
|
|
2. ✅ **Line 723: should preview template with sample data**
|
|
- Same fixes as above
|
|
- Added error handling for optional preview button
|
|
- Fallback to continue if preview not available
|
|
|
|
3. ✅ **Line 780: should edit external template**
|
|
- Fixed manage templates button click
|
|
- Wait for template list to appear
|
|
- Click edit button with fallback logic
|
|
- Use generic textarea selector for config
|
|
|
|
### Template Deletion Test (1 test)
|
|
4. ✅ **Line 829: should delete external template**
|
|
- Added explicit template management button click
|
|
- Fixed delete button selection with timeout and error handling
|
|
|
|
### Provider Tests (3 tests)
|
|
5. ✅ **Line 331: should edit existing provider**
|
|
- Added verification step to confirm provider is displayed
|
|
- Improved provider card and edit button selection
|
|
- Added timeout handling for form visibility
|
|
|
|
6. ✅ **Line 1105: should persist event selections**
|
|
- Improved form visibility check with Card presence verification
|
|
- Better provider card selection using text anchors
|
|
- Added explicit wait strategy
|
|
|
|
7. ✅ (Bonus) Fixed provider creation tests
|
|
- All provider form tests now have consistent pattern
|
|
- Wait for form to render before filling fields
|
|
|
|
## Key Lessons Learned
|
|
|
|
### 1. **Understand UI Structure Before Testing**
|
|
- Always check if it's a modal dialog or conditional rendering
|
|
- Understand what triggers visibility changes
|
|
- Check if required test IDs exist in the actual code
|
|
|
|
### 2. **Use Multiple Selection Strategies**
|
|
- Primary: Specific selectors (role-based, test IDs)
|
|
- Secondary: Generic DOM selectors (input[type="text"], select, textarea)
|
|
- Tertiary: Context-based selection (find in specific sections)
|
|
|
|
### 3. **Add Fallback Logic**
|
|
- Don't assume a button selection will work
|
|
- Use `.catch(() => false)` for optional elements
|
|
- Log or expect failures to understand why tests fail
|
|
|
|
### 4. **Wait for Real Visibility**
|
|
- Don't just wait for elements to exist in DOM
|
|
- Wait for form inputs with proper timeouts
|
|
- Verify action results (form appeared, button clickable, etc.)
|
|
|
|
## Files Modified
|
|
- `/projects/Charon/tests/settings/notifications.spec.ts`
|
|
- Lines 683-718: should create custom template
|
|
- Lines 723-771: should preview template with sample data
|
|
- Lines 780-853: should edit external template
|
|
- Lines 829-898: should delete external template
|
|
- Lines 331-413: should edit existing provider
|
|
- Lines 1105-1177: should persist event selections
|
|
|
|
## Recommendations for Future Work
|
|
|
|
### Short Term
|
|
1. Consider adding `data-testid` attributes to `TemplateForm` component inputs:
|
|
```tsx
|
|
<input {...register('name')} data-testid="template-name" />
|
|
```
|
|
This would make tests more robust and maintainable.
|
|
|
|
2. Use consistent test ID patterns across all forms (provider, template, etc.)
|
|
|
|
### Medium Term
|
|
1. Consider refactoring template management to use a proper dialog/modal component
|
|
- Would improve UX consistency
|
|
- Make testing clearer
|
|
- Align with provider management pattern
|
|
|
|
2. Add better error messages and logging in forms
|
|
- Help tests understand why they fail
|
|
- Help users understand what went wrong
|
|
|
|
### Long Term
|
|
1. Establish testing guidelines for form-based UI:
|
|
- When to use test IDs vs DOM selectors
|
|
- How to handle conditional rendering
|
|
- Standard patterns for dialog testing
|
|
|
|
2. Create test helpers/utilities for common patterns:
|
|
- Form filler functions
|
|
- Button finder with fallback logic
|
|
- Dialog opener/closer helpers
|