test: expand and stabilize settings e2e coverage#2024
Merged
senamakel merged 8 commits intoMay 17, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds broad settings E2E coverage and element helpers, fixes Composio routing label interactions, adds test-isolation guards and timing adjustments, and updates build/test tooling (configurable cargo install cache, Appium resolution, Rust test stack sizing). ChangesSettings UI & E2E Test Coverage
Build System & Test Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 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: 6
🧹 Nitpick comments (2)
app/test/e2e/specs/settings-advanced-config.spec.ts (1)
156-168: ⚡ Quick winRemove fixed sleep before persistence assertion.
browser.pause(1000)is brittle under load variance. Prefer relying only onwaitUntilwith the persistence predicate.🤖 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/settings-advanced-config.spec.ts` around lines 156 - 168, Remove the brittle fixed sleep by deleting the browser.pause(1000) call and rely solely on the existing browser.waitUntil predicate that reads local storage via readLocalStorageJson; keep or adjust the timeout/interval passed to browser.waitUntil if needed to account for slower environments so the predicate checking payload?.modelOverride === 'gpt-4.1-mini' && payload?.temperature === '0.2' is the only synchronization mechanism.app/test/e2e/specs/settings-feature-preferences.spec.ts (1)
35-40: ⚡ Quick winReplace fixed reload delay with readiness-based wait.
The 3s hard pause can still race on slow CI and adds unnecessary latency on fast runs. Wait for a deterministic app-ready marker/hash condition instead.
🤖 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/settings-feature-preferences.spec.ts` around lines 35 - 40, In reloadAndReturnTo, replace the fixed browser.pause(3000) after window.location.reload() with a deterministic readiness wait: after calling window.location.reload(), wait for a concrete app-ready condition (e.g., a specific DOM marker or URL/hash change) instead of sleeping, then call navigateViaHash(route) and waitForText(markerText, 15_000) as before; update the function (reloadAndReturnTo) to use the existing wait helper(s) or implement a short waitFor existence of the readiness selector or for location.hash to change rather than using browser.pause.
🤖 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/settings-account-preferences.spec.ts`:
- Around line 20-29: The test helper clickFirstSwitch is fragile because it
clicks switches[0]; change it to target the analytics toggle via a stable
selector (e.g., data-testid or aria-label) instead of index-based selection:
update the clickFirstSwitch function (or add a new clickSwitchBySelector) to
accept and query a selector string and click the matched element (use
'[data-testid="analytics-toggle"]' or the specific aria-label), and replace the
other call sites (including the occurrences referenced around the other switch
usage) to pass that stable selector so the test targets the correct toggle
deterministically.
- Around line 20-51: The two custom DOM helpers clickFirstSwitch and
clickBySelector directly use browser.execute and must be replaced to use the
shared E2E helpers from element-helpers.ts (the toggle/button click helpers) for
cross-platform stability; import the appropriate helper functions from
element-helpers.ts (e.g., the toggle click helper for switches and the
button/click helper for generic selectors), update clickFirstSwitch to call the
toggle helper for the first switch and update clickBySelector to call the shared
click helper (ensuring both await the helper and return a boolean), and remove
the direct browser.execute logic so specs use the centralized helpers.
In `@app/test/e2e/specs/settings-advanced-config.spec.ts`:
- Around line 13-30: The inline page-execution helpers clickSelector and
clickLabelContaining should be removed and replaced with the shared E2E
interaction helpers from element-helpers.ts; import and call the appropriate
exported functions (the project's cross-platform click primitives) instead of
using browser.execute in the test, update usages in
settings-advanced-config.spec.ts to call those helpers, and delete clickSelector
and clickLabelContaining so tests rely on the centralized element-helpers API
for resilient, consistent interactions.
In `@app/test/e2e/specs/settings-feature-preferences.spec.ts`:
- Around line 146-158: The test toggles both 'Toggle Do Not Disturb' and 'Toggle
Messages notifications' but only asserts the Messages state after reload; update
the spec to also assert the Do Not Disturb control persists by calling
switchState('Toggle Do Not Disturb') after
reloadAndReturnTo('/settings/notifications', 'Do Not Disturb') and checking it
equals the expected value (matching the toggle done earlier). Locate this logic
around the existing clickSelector and switchState calls in
settings-feature-preferences.spec.ts and add the additional post-reload expect
for 'Toggle Do Not Disturb'.
- Around line 13-33: The spec defines bespoke UI helpers setSelectValue and
clickSelector; remove these local functions and replace their usage with the
shared helpers from element-helpers.ts to keep cross-platform behavior
consistent — import the appropriate helpers (e.g., the shared setSelectValue and
click/clickElement helper names used in element-helpers.ts) at the top of
settings-feature-preferences.spec.ts, update all calls to use those helper
functions instead of setSelectValue/clickSelector, and delete the local function
declarations (setSelectValue and clickSelector) so the spec relies exclusively
on the centralized element-helpers API.
In `@src/openhuman/channels/tests/runtime_dispatch.rs`:
- Around line 89-90: The test's assertion "elapsed < Duration::from_millis(700)"
no longer guarantees parallelism because 700ms can be reached by near-sequential
runs; update the test in runtime_dispatch.rs (the runtime_dispatch test) to use
a stricter threshold or, better, measure a single-message baseline inside the
test (call the same dispatch path once to get baseline_elapsed) and then assert
elapsed < baseline_elapsed * 1.3 (or a similarly tight multiplier) to prove
concurrency; adjust the Duration comparison or implement the baseline
measurement around the same dispatch functions used in the test to make the
assertion robust.
---
Nitpick comments:
In `@app/test/e2e/specs/settings-advanced-config.spec.ts`:
- Around line 156-168: Remove the brittle fixed sleep by deleting the
browser.pause(1000) call and rely solely on the existing browser.waitUntil
predicate that reads local storage via readLocalStorageJson; keep or adjust the
timeout/interval passed to browser.waitUntil if needed to account for slower
environments so the predicate checking payload?.modelOverride === 'gpt-4.1-mini'
&& payload?.temperature === '0.2' is the only synchronization mechanism.
In `@app/test/e2e/specs/settings-feature-preferences.spec.ts`:
- Around line 35-40: In reloadAndReturnTo, replace the fixed browser.pause(3000)
after window.location.reload() with a deterministic readiness wait: after
calling window.location.reload(), wait for a concrete app-ready condition (e.g.,
a specific DOM marker or URL/hash change) instead of sleeping, then call
navigateViaHash(route) and waitForText(markerText, 15_000) as before; update the
function (reloadAndReturnTo) to use the existing wait helper(s) or implement a
short waitFor existence of the readiness selector or for location.hash to change
rather than using browser.pause.
🪄 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: e563314e-0a32-4c9d-9404-5dfffbf74449
📒 Files selected for processing (13)
.gitignoreapp/src/components/settings/panels/ComposioPanel.tsxapp/test/e2e/specs/settings-account-preferences.spec.tsapp/test/e2e/specs/settings-advanced-config.spec.tsapp/test/e2e/specs/settings-ai-skills.spec.tsapp/test/e2e/specs/settings-feature-preferences.spec.tse2e/docker-local-bootstrap.shscripts/ensure-tauri-cli.shscripts/test-rust-with-mock.shsrc/openhuman/channels/tests/runtime_dispatch.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/providers/types.rssrc/openhuman/wallet/execution.rs
Contributor
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/helpers/element-helpers.ts`:
- Around line 387-395: The helper setSelectValueByTestId can return true even
when assigning an invalid option value leaves the select unchanged; update the
browser.execute payload (the anonymous function) to verify the assignment
actually took effect by comparing el.value to the requested next value (and if
it doesn't match, try to find an option with value === next and set its selected
property before dispatching the change), then return true only when the select's
value equals next and false otherwise; reference the existing anonymous function
that queries `[data-testid="${id}"]` and the passed parameters ({ id, next }) to
locate where to add this verification and conditional dispatch logic.
🪄 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: 7bcf2aac-6081-46ea-ba43-d84f65439f31
📒 Files selected for processing (6)
app/src/components/settings/panels/PrivacyPanel.tsxapp/test/e2e/helpers/element-helpers.tsapp/test/e2e/specs/settings-account-preferences.spec.tsapp/test/e2e/specs/settings-advanced-config.spec.tsapp/test/e2e/specs/settings-feature-preferences.spec.tssrc/openhuman/channels/tests/runtime_dispatch.rs
✅ Files skipped from review due to trivial changes (1)
- app/src/components/settings/panels/PrivacyPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/test/e2e/specs/settings-advanced-config.spec.ts
- app/test/e2e/specs/settings-account-preferences.spec.ts
- app/test/e2e/specs/settings-feature-preferences.spec.ts
Comment on lines
+387
to
+395
| return await browser.execute( | ||
| ({ id, next }) => { | ||
| const el = document.querySelector<HTMLSelectElement>(`[data-testid="${id}"]`); | ||
| if (!el) return false; | ||
| el.value = next; | ||
| el.dispatchEvent(new Event('change', { bubbles: true })); | ||
| return true; | ||
| }, | ||
| { id: testId, next: value } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/test/e2e/helpers/element-helpers.ts | sed -n '380,400p'Repository: tinyhumansai/openhuman
Length of output: 870
🏁 Script executed:
rg -n "setSelectValueByTestId" app/test/e2e --type tsRepository: tinyhumansai/openhuman
Length of output: 576
🏁 Script executed:
cat -n app/test/e2e/specs/settings-feature-preferences.spec.ts | sed -n '150,170p'Repository: tinyhumansai/openhuman
Length of output: 1218
setSelectValueByTestId can return success when no value was actually applied.
The function always returns true after setting el.value = next and dispatching a change event, even if next is not a valid option value. HTML select elements don't throw errors when assigned invalid values; the assignment appears to succeed syntactically but the element's actual value may not change, creating false-positive E2E test results that mask broken test flows.
Suggested fix
return await browser.execute(
({ id, next }) => {
- const el = document.querySelector<HTMLSelectElement>(`[data-testid="${id}"]`);
+ const el = Array.from(
+ document.querySelectorAll<HTMLSelectElement>('select[data-testid]')
+ ).find((node) => node.getAttribute('data-testid') === id);
if (!el) return false;
+ if (!Array.from(el.options).some((opt) => opt.value === next)) return false;
el.value = next;
+ if (el.value !== next) return false;
el.dispatchEvent(new Event('change', { bubbles: true }));
return true;
},
{ id: testId, next: value }
);📝 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
| return await browser.execute( | |
| ({ id, next }) => { | |
| const el = document.querySelector<HTMLSelectElement>(`[data-testid="${id}"]`); | |
| if (!el) return false; | |
| el.value = next; | |
| el.dispatchEvent(new Event('change', { bubbles: true })); | |
| return true; | |
| }, | |
| { id: testId, next: value } | |
| return await browser.execute( | |
| ({ id, next }) => { | |
| const el = Array.from( | |
| document.querySelectorAll<HTMLSelectElement>('select[data-testid]') | |
| ).find((node) => node.getAttribute('data-testid') === id); | |
| if (!el) return false; | |
| if (!Array.from(el.options).some((opt) => opt.value === next)) return false; | |
| el.value = next; | |
| if (el.value !== next) return false; | |
| el.dispatchEvent(new Event('change', { bubbles: true })); | |
| return true; | |
| }, | |
| { id: testId, next: value } | |
| ); |
🤖 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/helpers/element-helpers.ts` around lines 387 - 395, The helper
setSelectValueByTestId can return true even when assigning an invalid option
value leaves the select unchanged; update the browser.execute payload (the
anonymous function) to verify the assignment actually took effect by comparing
el.value to the requested next value (and if it doesn't match, try to find an
option with value === next and set its selected property before dispatching the
change), then return true only when the select's value equals next and false
otherwise; reference the existing anonymous function that queries
`[data-testid="${id}"]` and the passed parameters ({ id, next }) to locate where
to add this verification and conditional dispatch logic.
Member
Author
learn what knowledge? :D now you can just claude or codex max the crap out of things |
This was referenced May 20, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
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.
Summary
cargo-tauriinstallation.Problem
Solution
cargo-tauriinstall reliably inside Docker.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change. This PR adds tests and harness fixes but does not add or rename matrix feature rows.## Related. No matrix row changes or new feature IDs were introduced here.docs/RELEASE-MANUAL-SMOKE.md). This change is coverage/harness oriented and does not add a release-cut manual smoke path.Closes #NNNin the## Relatedsection. No upstream tracking issue was linked for this work.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/e2e-settings-flow-coveragedd66a134Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckpnpm debug unitpnpm debug rustapp/test/e2e/specs/settings-account-preferences.spec.tsapp/test/e2e/specs/settings-feature-preferences.spec.tsapp/test/e2e/specs/settings-advanced-config.spec.tscargo fmt --checkandcargo checkgates passedcargo fmt --manifest-path app/src-tauri/Cargo.toml --checkandcargo check --manifest-path app/src-tauri/Cargo.tomlgates passedValidation Blocked
command:pnpm test:coverageerror:Not run in this shipping pass; I validated targeted/new flows plus full unit and Rust suites instead.impact:Diff coverage is still enforced in CI and remains part of the babysit loop.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests & Infrastructure