test(e2e): use stable cron test ids#2301
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds tauri-only E2E helpers ( ChangesStable E2E Test Selectors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/test/e2e/specs/cron-jobs-flow.spec.ts`:
- Line 31: The spec currently imports and calls tauri-driver-only helpers
waitForTestId and clickTestId (used in cron-jobs-flow.spec.ts) unguarded,
causing hard failures on non-tauri sessions (macOS/Appium); update the test to
detect the driver/session type or use the cross-platform element helpers (e.g.,
textExists, waitForText or the provided crossPlatformClick/wait helpers) before
invoking waitForTestId/clickTestId, wrapping all uses of waitForTestId and
clickTestId (including the occurrences referenced by the reviewer) in a
conditional that falls back to the tauri-agnostic helper so the spec can run on
both tauri and Appium.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba3d11ed-0310-44fa-9acb-8db2d81050a2
📒 Files selected for processing (4)
app/src/components/settings/panels/CronJobsPanel.tsxapp/test/e2e/helpers/element-helpers.tsapp/test/e2e/specs/cron-jobs-flow.spec.tsgitbooks/developing/e2e-testing.md
aqilaziz
force-pushed
the
codex/1861-cron-e2e-testids
branch
from
28eca40 to
c3e4004
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/test/e2e/specs/cron-jobs-flow.spec.ts (1)
31-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard tauri-only test-id helpers with cross-platform fallbacks.
waitForTestId/clickTestIdare still called unguarded (Line 81, Line 98, Line 125, Line 153, Line 156, Line 158, Line 176). This can hard-fail on Mac2/Appium runs.Suggested minimal fix
-import { clickTestId, textExists, waitForTestId, waitForText } from '../helpers/element-helpers'; +import { + clickNativeButton, + clickTestId, + textExists, + waitForTestId, + waitForText, +} from '../helpers/element-helpers'; +import { isTauriDriver } from '../helpers/platform'; +async function waitForCronPanel(timeout = 5_000): Promise<void> { + if (isTauriDriver()) { + await waitForTestId('cron-jobs-panel', timeout); + } else { + await waitForText('Cron Jobs', timeout); + } +} + +async function clickCronRefresh(): Promise<void> { + if (isTauriDriver()) { + await clickTestId('cron-refresh'); + } else { + await clickNativeButton('Refresh Cron Jobs'); + } +}As per coding guidelines: “Ensure each spec is runnable in isolation… Use
clickNativeButton(),waitForWebView(),clickToggle()for cross-platform element interaction.”Also applies to: 81-82, 98-99, 125-126, 153-159, 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/test/e2e/specs/cron-jobs-flow.spec.ts` at line 31, The spec calls Tauri-only helpers waitForTestId and clickTestId directly (in cron-jobs-flow.spec.ts) which can fail on non-Tauri runners; update the call sites (the uses at the locations flagged) to use cross-platform fallbacks: either replace with clickNativeButton / clickToggle / waitForWebView as appropriate, or wrap the existing waitForTestId/clickTestId calls in a small runtime guard that detects the platform and falls back to web-friendly helpers (e.g., try waitForTestId and on failure call waitForWebView, or use clickNativeButton instead of clickTestId for buttons). Ensure you update every occurrence of waitForTestId and clickTestId in this spec so the spec runs on Mac2/Appium and web runners.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/test/e2e/specs/cron-jobs-flow.spec.ts`:
- Line 31: The spec calls Tauri-only helpers waitForTestId and clickTestId
directly (in cron-jobs-flow.spec.ts) which can fail on non-Tauri runners; update
the call sites (the uses at the locations flagged) to use cross-platform
fallbacks: either replace with clickNativeButton / clickToggle / waitForWebView
as appropriate, or wrap the existing waitForTestId/clickTestId calls in a small
runtime guard that detects the platform and falls back to web-friendly helpers
(e.g., try waitForTestId and on failure call waitForWebView, or use
clickNativeButton instead of clickTestId for buttons). Ensure you update every
occurrence of waitForTestId and clickTestId in this spec so the spec runs on
Mac2/Appium and web runners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4bfe317-6be1-43ff-a15a-d584b7a3bb3b
📒 Files selected for processing (4)
app/src/components/settings/panels/CronJobsPanel.tsxapp/test/e2e/helpers/element-helpers.tsapp/test/e2e/specs/cron-jobs-flow.spec.tsgitbooks/developing/e2e-testing.md
✅ Files skipped from review due to trivial changes (2)
- gitbooks/developing/e2e-testing.md
- app/src/components/settings/panels/CronJobsPanel.tsx
aqilaziz
force-pushed
the
codex/1861-cron-e2e-testids
branch
from
c3e4004 to
c11dca9
Compare
aqilaziz
force-pushed
the
codex/1861-cron-e2e-testids
branch
from
c11dca9 to
29d5446
Compare
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid work migrating the cron E2E flow off brittle Tailwind class selectors to stable data-testid hooks. The fallback pattern (try testid → catch → text/button helper) is well-structured and keeps the spec runnable on non-tauri drivers. Helpers are clean, taxonomy doc is useful.
CI is fully green including the Linux/Appium E2E run, and the scoped slice of #1861 is delivered as described.
| Area | Files | Changes |
|---|---|---|
| Frontend | CronJobsPanel.tsx |
+2 data-testid attributes (cron-jobs-panel, cron-refresh) |
| E2E helpers | element-helpers.ts |
+waitForTestId, clickTestId (tauri-driver guarded) |
| E2E spec | cron-jobs-flow.spec.ts |
Replaced DOM-walking with testid selectors + cross-platform fallbacks |
| Docs | e2e-testing.md |
Testid naming taxonomy |
One minor note below — otherwise LGTM. Nice incremental improvement to the E2E foundation.
| - `skill-row-<skillId>`, `skill-install-<skillId>`, `skill-uninstall-<skillId>` | ||
| - `thread-row-<threadId>`, `new-thread-button`, `send-message-button` | ||
| - `onboarding-next-button` | ||
|
|
There was a problem hiding this comment.
[minor] The taxonomy section documents test IDs for surfaces not yet implemented (settings-nav-<routeId>, skill-row-<skillId>, thread-row-<threadId>, etc.). This is fine as a forward-looking convention guide, but consider adding a note like "IDs marked with ★ are planned but not yet wired" — saves someone from writing waitForTestId('skill-row-...') and wondering why it times out.
|
huge thanks @aqilaziz, locking in stable test ids and ditching the tailwind/dom walking is gonna make these e2e flows so much less flaky 🙌 love that you also documented the testid taxonomy in the guide, future contributors will thank you. cheers for another solid one 🚀 |
3 participants
Summary
waitForTestIdandclickTestIdE2E helpers for stable DOM hooks.cron-jobs-panelandcron-refreshhooks to the Cron Jobs settings panel.data-testidnaming taxonomy in the E2E guide.Problem
cron-jobs-flow.spec.tsas the concrete example of brittle selectors: it foundmorning_briefingvia Tailwind class strings andclosest('div.p-4').Solution
CoreJobListcron row/action hooks directly from the spec.element-helpers.tsinstead of keeping local DOM snippets in specs.Submission Checklist
## Related- N/A: no matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) - N/A: no release-cut surface.Closes #NNNin the## Relatedsection - N/A: this is a scoped slice that references Add data-testid hooks to UI affordances exercised by E2E specs #1861 without closing the broader audit issue.Impact
data-testidattributes on the Cron Jobs panel.waitForTestIdis documented as tauri-driver/DOM-backed; Mac2 specs should keep accessibility/text helpers unless IDs are mirrored into accessible labels.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/1861-cron-e2e-testids28eca40a3d2e3a75b61105887e4faf58d4c0866eValidation Run
pnpm --filter openhuman-app exec prettier --check src/components/settings/panels/CronJobsPanel.tsx test/e2e/helpers/element-helpers.ts test/e2e/specs/cron-jobs-flow.spec.ts- passedpnpm typecheck- passedcargo fmt --all --check- passed;git diff --check- passedValidation Blocked
command:pnpm --filter openhuman-app exec tsc -p test/tsconfig.e2e.json --noEmiterror:local command could not findtsc/@wdio/globals/typesvia that invocationimpact:app-widepnpm typecheckpasses; remote E2E/TS jobs remain authoritative for the E2E tsconfigBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Tests
Documentation
Maintenance