Wegent/fix settings model e2e tests#742
Open
joyway1978 wants to merge 9 commits intowecode-ai:mainfrom
Open
Conversation
- Add data-testid to QuickAccessCards team cards - Update chat-flow.spec.ts to select model (公网:GLM-5) - Use data-testid for send button consistently - Simplify QuickAccessCards selection logic Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add data-testid support to ActionButton component - Add data-testid="clarification-toggle" to ClarificationToggle - Update chat-flow.spec.ts to use data-testid for finding clarification button - Simplify detection logic Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add data-testid to MessagesArea container - Add data-testid to ClarificationForm and its elements - Add data-testid to ClarificationQuestion radio/checkbox options - Add data-testid to AI message icon (Bot icon) - Add data-testid to ChatInputControls - Add data-testid to QuickAccessCards container - Update chat-flow.spec.ts to use data-testid exclusively Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Support both English and Chinese titles in settings-model.spec.ts - Update selectTestModel to use data-testid and handle all button states - Fix button text matching to include '选择模型' (Select Model) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix settings-model.spec.ts title selector to use 'Model Management' / '模型管理' - Add data-testid to model-id-name-input in ModelEditDialog - Update chat-image-browser-e2e.spec.ts selectTestModel with better detection - Increase timeouts and add error state detection
- settings-model.spec.ts: - Change waitUntil from 'domcontentloaded' to 'networkidle' - Add wait for settings content area to be visible - Add retry logic for flaky title loading - Add wait for dialog to open before checking form elements - Increase timeouts for form element visibility - chat-image-browser-e2e.spec.ts: - Add dismissOnboardingTour() calls before clicking elements - Fix driver-overlay blocking clicks in Strategy 2, 3, and 4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
settings-model.spec.ts: - Fix model creation test by targeting Model ID dropdown instead of provider dropdown - Use more specific selectors to avoid clicking wrong elements in dialog chat-image-browser-e2e.spec.ts: - Enhance dismissOnboardingTour() to forcefully remove driver.js elements from DOM - Increase wait time for model dropdown options to load (2s instead of 1s) - Filter out input elements when selecting model options Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix the failing e2e tests in settings-model.spec.ts: 1. Change wait strategy from 'networkidle' to 'domcontentloaded' to avoid timeout issues with ongoing network requests 2. Add explicit wait for model management title in beforeEach to ensure page is fully loaded before tests run 3. Increase dialog open timeout from 1000ms to 1500ms for better reliability 4. Increase element visibility timeouts from 10000ms to 15000ms 5. Remove redundant title checks in individual tests (already done in beforeEach) 6. Update visual regression test to wait for title instead of fixed timeout These changes improve test reliability by: - Using more reliable page load detection - Ensuring elements are visible before interacting with them - Providing adequate time for dialog animations to complete Refs: CI e2e test failures in settings-model.spec.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughThis pull request introduces data-testid attributes across frontend components and E2E tests to improve test reliability, updates E2E tests to use these new selectors instead of legacy ones, and refactors dialog-based UI handling in model management tests. Additionally, a port management utility script is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
stop_port.sh (1)
35-39: Prefer graceful termination before forcing SIGKILL.Line 38 uses unconditional
kill -9. A TERM-first approach is safer and still clears stuck processes when needed.🔧 Proposed safer termination flow
- kill -9 $PIDS + kill $PIDS 2>/dev/null + sleep 1 + REMAINING_PIDS=$(lsof -ti ":$port") + if [ -n "$REMAINING_PIDS" ]; then + kill -9 $REMAINING_PIDS + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stop_port.sh` around lines 35 - 39, The script stop_port.sh currently sends unconditional SIGKILL to PIDS (variable PIDS) which is unsafe; change the termination flow in the block that finds PIDS=$(lsof -ti ":$port") to send SIGTERM first (kill -TERM $PIDS), wait a short interval and re-check whether those PIDs still exist, and only then send SIGKILL (kill -9 $PIDS) for any remaining live PIDs; ensure you properly quote/iterate the PIDS variable when calling kill and use a small retry/wait loop (check with ps or lsof) so the code in the if [ -n "$PIDS" ]; then branch gracefully terminates before forcing kill.frontend/e2e/tests/settings-model.spec.ts (1)
48-56: Consider removing the fixedwaitForTimeout(1500)before checking dialog visibility.The 1.5-second delay before checking for the dialog is arbitrary. The subsequent
waitForSelectorwith a 15-second timeout should be sufficient to wait for the dialog to appear.♻️ Proposed simplification
await createButton.first().click() - // Wait for dialog to open (animation + rendering time) - await page.waitForTimeout(1500) - // Wait for dialog content to be visible await page.waitForSelector('[role="dialog"]', { state: 'visible', timeout: 15000 }) // Model edit is a dialog form - check for the model ID input const modelIdInput = page.locator('[data-testid="model-id-name-input"]') await expect(modelIdInput).toBeVisible({ timeout: 15000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/tests/settings-model.spec.ts` around lines 48 - 56, Remove the fixed 1.5s delay by deleting the call to page.waitForTimeout(1500) and rely on the existing await page.waitForSelector('[role="dialog"]', { state: 'visible', timeout: 15000 }) (and the subsequent locator checks like const modelIdInput = page.locator('[data-testid="model-id-name-input"]')) to wait for the dialog to appear; this removes the arbitrary pause and keeps the test deterministic using the selector timeouts.tests/specs/chat-flow.spec.ts (1)
171-177: Fragile check for enabled state relies on CSS class name.Checking
buttonClass?.includes('primary')to verify enabled state is brittle—it will break if the styling changes. Consider using a more explicit indicator.♻️ Proposed alternative: Check aria-pressed or data attribute
If the component supports it, prefer checking
aria-pressed="true"or adata-activeattribute rather than CSS class names:// Verify the button is now in enabled state (has primary/border-primary class) - const buttonClass = await clarificationToggle.getAttribute('class') - const isEnabled = buttonClass?.includes('primary') + // Check if button indicates active state via aria or styling + const isEnabled = await clarificationToggle.evaluate((el) => { + return el.getAttribute('aria-pressed') === 'true' || + el.classList.contains('border-primary') || + el.classList.contains('bg-primary') + })Alternatively, add a
data-testidordata-activeattribute to the component for explicit state indication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/specs/chat-flow.spec.ts` around lines 171 - 177, The check using buttonClass?.includes('primary') is brittle; update the enabled-state detection for the clarificationToggle element to use an explicit attribute instead (e.g., check clarificationToggle.getAttribute('aria-pressed') === 'true' or clarificationToggle.getAttribute('data-active') === 'true'), falling back to the class check only if those attributes are absent; adjust the conditional that calls test.skip() and the console message to reflect the new attribute-based check so the test reliably detects the toggle state.frontend/e2e/tests/tasks/chat-image-browser-e2e.spec.ts (1)
257-266: Redundant style assignment before element removal.Setting
style.display = 'none'is unnecessary sinceremove()immediately deletes the element from the DOM.♻️ Proposed simplification
console.warn('Overlay still visible, forcefully removing from DOM') await page.evaluate(() => { document.querySelectorAll('.driver-overlay, .driver-popover, .driver-popover-tip, [class*="driver-"]').forEach(el => { - ;(el as HTMLElement).style.display = 'none' el.remove() }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/tests/tasks/chat-image-browser-e2e.spec.ts` around lines 257 - 266, The code inside the recovery block for driverOverlay redundantly sets (el as HTMLElement).style.display = 'none' immediately before el.remove(); update the cleanup in the driverOverlay.isVisible check to simply remove matched elements without changing their style: locate the driverOverlay handling (use symbols driverOverlay, isVisible, page.evaluate and the selector '.driver-overlay, .driver-popover, .driver-popover-tip, [class*="driver-"]') and delete the style assignment so the evaluation only calls el.remove() for each element, keeping the waitForTimeout(500) afterwards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/e2e/tests/settings-model.spec.ts`:
- Around line 99-106: After clicking the submitButton in the test, replace the
blind wait with an explicit assertion that verifies the dialog actually closed
or a success notification appeared: wait for the ModelEditDialog to be
detached/hidden (use the dialog's root selector or the same locator used to open
it) or wait for the success toast/toast text that your app emits on successful
save, since ModelEditDialog calls onClose() only on success; update the test
around submitButton.click() to await that dialog-closed or success-toast
condition instead of page.waitForTimeout(2000).
In `@frontend/src/features/tasks/components/chat/QuickAccessCards.tsx`:
- Around line 221-223: The data-testid on the team card (in
QuickAccessCards.tsx) is currently using the mutable team.name which can change
and cause flaky selectors; update the test id to use a stable, immutable
identifier (e.g., team.id or team.teamId) instead of team.name in the element
where onClick={() => !isClicked && handleTeamClick(team)} is defined, and adjust
any tests that assert against the old `quick-access-team-{team.name}` string to
use the new `quick-access-team-{team.id}` format.
---
Nitpick comments:
In `@frontend/e2e/tests/settings-model.spec.ts`:
- Around line 48-56: Remove the fixed 1.5s delay by deleting the call to
page.waitForTimeout(1500) and rely on the existing await
page.waitForSelector('[role="dialog"]', { state: 'visible', timeout: 15000 })
(and the subsequent locator checks like const modelIdInput =
page.locator('[data-testid="model-id-name-input"]')) to wait for the dialog to
appear; this removes the arbitrary pause and keeps the test deterministic using
the selector timeouts.
In `@frontend/e2e/tests/tasks/chat-image-browser-e2e.spec.ts`:
- Around line 257-266: The code inside the recovery block for driverOverlay
redundantly sets (el as HTMLElement).style.display = 'none' immediately before
el.remove(); update the cleanup in the driverOverlay.isVisible check to simply
remove matched elements without changing their style: locate the driverOverlay
handling (use symbols driverOverlay, isVisible, page.evaluate and the selector
'.driver-overlay, .driver-popover, .driver-popover-tip, [class*="driver-"]') and
delete the style assignment so the evaluation only calls el.remove() for each
element, keeping the waitForTimeout(500) afterwards.
In `@stop_port.sh`:
- Around line 35-39: The script stop_port.sh currently sends unconditional
SIGKILL to PIDS (variable PIDS) which is unsafe; change the termination flow in
the block that finds PIDS=$(lsof -ti ":$port") to send SIGTERM first (kill -TERM
$PIDS), wait a short interval and re-check whether those PIDs still exist, and
only then send SIGKILL (kill -9 $PIDS) for any remaining live PIDs; ensure you
properly quote/iterate the PIDS variable when calling kill and use a small
retry/wait loop (check with ps or lsof) so the code in the if [ -n "$PIDS" ];
then branch gracefully terminates before forcing kill.
In `@tests/specs/chat-flow.spec.ts`:
- Around line 171-177: The check using buttonClass?.includes('primary') is
brittle; update the enabled-state detection for the clarificationToggle element
to use an explicit attribute instead (e.g., check
clarificationToggle.getAttribute('aria-pressed') === 'true' or
clarificationToggle.getAttribute('data-active') === 'true'), falling back to the
class check only if those attributes are absent; adjust the conditional that
calls test.skip() and the console message to reflect the new attribute-based
check so the test reliably detects the toggle state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68e9fb0e-424f-47bf-b0e3-bdb19c48684e
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
frontend/e2e/tests/settings-model.spec.tsfrontend/e2e/tests/tasks/chat-image-browser-e2e.spec.tsfrontend/e2e/tests/visual/settings-visual.spec.tsfrontend/src/components/ui/action-button.tsxfrontend/src/features/settings/components/ModelEditDialog.tsxfrontend/src/features/tasks/components/chat/QuickAccessCards.tsxfrontend/src/features/tasks/components/clarification/ClarificationForm.tsxfrontend/src/features/tasks/components/clarification/ClarificationQuestion.tsxfrontend/src/features/tasks/components/clarification/ClarificationToggle.tsxfrontend/src/features/tasks/components/input/ChatInputControls.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/components/message/MessagesArea.tsxstop_port.shtests/specs/chat-flow.spec.ts
Comment on lines
99
to
106
| // Submit form | ||
| const submitButton = page.locator('button:has-text("Save"), button:has-text("保存")').first() | ||
| if (await submitButton.isVisible({ timeout: 3000 }).catch(() => false)) { | ||
| if (await submitButton.isVisible({ timeout: 5000 }).catch(() => false)) { | ||
| await submitButton.click() | ||
|
|
||
| // Wait for navigation back to list or validation error | ||
| await page.waitForURL(/\/settings/, { timeout: 10000 }).catch(() => { | ||
| // May stay on form with validation errors | ||
| }) | ||
| // Wait for dialog to close or validation error | ||
| await page.waitForTimeout(2000) | ||
| } |
Contributor
There was a problem hiding this comment.
Test doesn't verify save success or failure.
After clicking submit, the test waits 2 seconds and ends without verifying the dialog closed or a success toast appeared. Per the context snippet from ModelEditDialog.tsx, onClose() is only called on success.
🔧 Proposed fix to verify dialog closure
const submitButton = page.locator('button:has-text("Save"), button:has-text("保存")').first()
if (await submitButton.isVisible({ timeout: 5000 }).catch(() => false)) {
await submitButton.click()
- // Wait for dialog to close or validation error
- await page.waitForTimeout(2000)
+ // Verify dialog closes on success (or remains open on validation error)
+ const dialogClosed = await page.locator('[role="dialog"]').isHidden({ timeout: 5000 }).catch(() => false)
+ if (!dialogClosed) {
+ console.log('Dialog still visible - may have validation errors')
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Submit form | |
| const submitButton = page.locator('button:has-text("Save"), button:has-text("保存")').first() | |
| if (await submitButton.isVisible({ timeout: 3000 }).catch(() => false)) { | |
| if (await submitButton.isVisible({ timeout: 5000 }).catch(() => false)) { | |
| await submitButton.click() | |
| // Wait for navigation back to list or validation error | |
| await page.waitForURL(/\/settings/, { timeout: 10000 }).catch(() => { | |
| // May stay on form with validation errors | |
| }) | |
| // Wait for dialog to close or validation error | |
| await page.waitForTimeout(2000) | |
| } | |
| // Submit form | |
| const submitButton = page.locator('button:has-text("Save"), button:has-text("保存")').first() | |
| if (await submitButton.isVisible({ timeout: 5000 }).catch(() => false)) { | |
| await submitButton.click() | |
| // Verify dialog closes on success (or remains open on validation error) | |
| const dialogClosed = await page.locator('[role="dialog"]').isHidden({ timeout: 5000 }).catch(() => false) | |
| if (!dialogClosed) { | |
| console.log('Dialog still visible - may have validation errors') | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/e2e/tests/settings-model.spec.ts` around lines 99 - 106, After
clicking the submitButton in the test, replace the blind wait with an explicit
assertion that verifies the dialog actually closed or a success notification
appeared: wait for the ModelEditDialog to be detached/hidden (use the dialog's
root selector or the same locator used to open it) or wait for the success
toast/toast text that your app emits on successful save, since ModelEditDialog
calls onClose() only on success; update the test around submitButton.click() to
await that dialog-closed or success-toast condition instead of
page.waitForTimeout(2000).
Comment on lines
221
to
223
| onClick={() => !isClicked && handleTeamClick(team)} | ||
| data-testid={`quick-access-team-${team.name}`} | ||
| className={` |
Contributor
There was a problem hiding this comment.
Use a stable key for team-card test IDs.
Line 222 uses team.name in data-testid, which can be duplicated/renamed and make selectors flaky. Prefer an immutable ID.
🔧 Proposed fix
- data-testid={`quick-access-team-${team.name}`}
+ data-testid={`quick-access-team-${team.id}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onClick={() => !isClicked && handleTeamClick(team)} | |
| data-testid={`quick-access-team-${team.name}`} | |
| className={` | |
| onClick={() => !isClicked && handleTeamClick(team)} | |
| data-testid={`quick-access-team-${team.id}`} | |
| className={` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/components/chat/QuickAccessCards.tsx` around
lines 221 - 223, The data-testid on the team card (in QuickAccessCards.tsx) is
currently using the mutable team.name which can change and cause flaky
selectors; update the test id to use a stable, immutable identifier (e.g.,
team.id or team.teamId) instead of team.name in the element where onClick={() =>
!isClicked && handleTeamClick(team)} is defined, and adjust any tests that
assert against the old `quick-access-team-{team.name}` string to use the new
`quick-access-team-{team.id}` format.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wegent/fix settings model e2e tests
Summary by CodeRabbit
Release Notes
Tests
Chores