Repository navigation
test: add onboarding regression E2E coverage - #1105
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: test: add onboarding regression E2E coverage
Good addition of regression coverage for the recent onboarding fixes. The tests are well-structured and cover the specific bugs mentioned (#1085/#1100 and #1077/#1089).
Test structure: The regressions/ directory is the right home — these are bug-coverage tests, not snapshot or mock-LLM integration tests. Existing review comment on directory location noted but addressed below.
Minor suggestions:
🟡 Suggestion: The advanceToLlmStep function advances through steps 0→1→2 but the test for LLM defaults only validates the final step. The function could also benefit from a comment explaining which step indexes correspond to which UI flows, since step numbering was recently changed.
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: test: add onboarding regression E2E coverage
Good addition of regression coverage for the recent onboarding fixes. The tests are well-structured and cover the specific bugs mentioned (#1085/#1100 and #1077/#1089).
Test structure: The regressions/ directory is the right home — these are bug-coverage tests, not snapshot or mock-LLM integration tests. Existing review comment on directory location noted but addressed below.
Minor suggestions:
🟡 Suggestion: The advanceToLlmStep function advances through steps 0→1→2 but the test for LLM defaults only validates the final step. The function could also benefit from a comment explaining which step indexes correspond to which UI flows, since step numbering was recently changed.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed review feedback in 0a6566b: moved the onboarding regression spec into the mock-LLM suite, clarified onboarding step-order comments, and resolved the review threads. Ready for another look. This comment was generated by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: test: add onboarding regression E2E coverage
Good addition of regression coverage. The tests are well-structured and directly address the issues mentioned in the PR description.
Summary of Changes
New file: tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts
- Regression test for #1085/#1100: Modal stays open on backdrop click and Escape key press. Also verifies openhands-onboarded is not prematurely set.
- Regression test for #1077/#1089: LLM setup step defaults to Anthropic Claude Opus 4.8, not OpenHands provider.
Modified file: tests/e2e/snapshots/onboarding.snapshot.spec.ts
- Updated to reflect the new backend-first onboarding order (step 0 = backend check, step 1 = agent selection)
- Changed route from /conversations to / where OnboardingHost mounts
- Renamed snapshot files to match new step order
Observations
-
Step order documentation - The explicit step-order comments (lines 85-87) are helpful and make the test fail clearly if the onboarding flow changes again.
-
Clean localStorage handling - The test properly removes openhands-onboarded to ensure onboarding modal appears, and the skip button correctly sets it to 1.
-
Mock LLM catalog routing - The deterministic model/provider catalog routing ensures stable assertions.
-
Consistent with existing patterns - The test follows the same structure as other mock-LLM specs (serial mode, helper imports from ./utils/mock-llm-helpers).
-
Minor: local waitForStep helper - Previous review feedback noted this could be extracted. Keeping it local is fine given it is only used here, and the step-order comments mitigate the duplication concern.
Verdict
LGTM - The regression coverage is appropriate and well-implemented.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: test: add onboarding regression E2E coverage
Overview
Good regression test coverage for recent onboarding fixes. The tests directly address the issues mentioned in the PR description (#1085 and #1077). Here are my observations:
Strengths
-
Clear test structure: The regression tests follow a consistent pattern with
showOnboarding()helper andwaitForStep()assertions. -
Explicit step indexing: The comment in
advanceToLlmStep()explaining the onboarding order is helpful for future maintainability. Good documentation of the recent reordering. -
Robust assertions: Using
.poll()for localStorage checks ensures the tests wait for actual state changes rather than relying on timing. -
Snapshot test updates: The snapshot tests correctly reflect the new step ordering (backend check now at step 0).
Minor Suggestions
-
Code duplication in helpers:
showOnboarding(),routeOnboardingLlmCatalog(), andadvanceToLlmStep()are defined in both test files. Consider extracting these into a shared helper module (e.g.,tests/e2e/mock-llm/onboarding-helpers.ts) to avoid duplication and ensure consistency. -
Test isolation: The
advanceToLlmStep()function assumes the backend health probe resolves quickly. While the test does wait foronboarding-backend-connected, the 10s timeout should be sufficient, but adding a comment explaining this assumption would help future readers. -
Error messages: Consider adding more descriptive failure messages to the assertions, especially for the localStorage checks, to make debugging easier if tests fail in CI.
Testing Note
The label update-snapshots suggests the snapshots may need to be regenerated. Ensure the snapshots are updated before merging this PR. The visual snapshots should be validated to ensure they capture the correct UI state for each step.
Conclusion
These tests provide solid regression coverage for the onboarding fixes. The implementation is clean and well-documented. The suggestions above are minor improvements rather than blockers.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@OpenHands fix these please #1105 (review) and /iterate |
|
Since my last summary, there are no additional code changes or actions to report. Checklist:
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: test: add onboarding regression E2E coverage
Summary
This PR adds end-to-end regression test coverage for two recently-fixed onboarding bugs:
- #1085/#1100: Backdrop clicks and Escape key presses should not dismiss the onboarding modal or mark it as complete
- #1077/#1089: First-run LLM setup should default to Anthropic Claude Opus instead of OpenHands
The changes are focused and well-scoped, adding 252 lines across 3 files with no unrelated modifications.
Files Reviewed
tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts (+120 lines)
Strengths:
- Good use of
test.describe.configure({ mode: "serial" })to prevent test interference - The
showOnboarding()helper is well-structured and properly seeds all required localStorage keys - Test 1 properly verifies both the modal visibility and the
openhands-onboardedstate remains null after errant interactions - Test 2 correctly checks both the provider AND model defaults
Suggestions:
- The test for "keeps the modal open on backdrop click and Escape" could benefit from also testing backdrop click specifically (not just coordinate click at 8,8), but the current approach is reasonable for a regression test
- The model value check
claude-opus-4-8is correct for the regression, but consider adding a comment noting this is the Anthropic model ID format
tests/e2e/snapshots/onboarding.snapshot.spec.ts (+52/-68 lines)
Strengths:
- Good use of
networkidlewait state before snapshot capture - The step-by-step navigation pattern is clear and well-documented in comments
- Using
dispatchEvent("click")is appropriate for the clipped slide navigation issue
Suggestions:
- The LLM step wait could be more resilient by waiting for a specific element instead of
networkidlewhich can be timing-dependent
tests/e2e/support/onboarding-helpers.ts (+80 lines)
Strengths:
- Clean, well-documented helper functions
routeOnboardingLlmCatalog()properly mocks all three endpoints needed for the LLM step- Good timeout values (15s for step waits, 10s for element visibility)
Suggestions:
- The
advanceOnboardingToLlmStep()function is a nice addition for test clarity
Test Coverage Assessment
| Regression | Test Coverage | Status |
|---|---|---|
| #1085/#1100 - Backdrop/Escape dismissal | ✅ Covered | Test verifies modal stays open and openhands-onboarded is not set |
| #1077/#1089 - Anthropic default | ✅ Covered | Test verifies both provider (Anthropic) and model (claude-opus-4-8) defaults |
Quality Notes
- The PR description clearly explains the motivation and links the regressions
- All mocks are properly isolated to the mock-llm test environment
- The snapshot tests update is a good sign this PR has been properly vetted
Conclusion
This is a solid addition of regression test coverage. The tests are well-structured and properly isolate the tested behavior. No blocking issues identified.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the latest AI review suggestions in 95780be: added a note clarifying the Anthropic model ID assertion and replaced the onboarding snapshot tests' networkidle waits with a specific LLM settings readiness helper.\n\nLocal checks run again: targeted ESLint, npm run typecheck, mock-LLM onboarding regression spec, and onboarding snapshot spec.\n\n_This comment was generated by an AI agent (OpenHands) on behalf of the user._ |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Onboarding Regression E2E Coverage
Good addition of regression tests covering the recent onboarding fixes. The test helper structure is clean and reusable.
Summary
| File | Changes | Notes |
|---|---|---|
tests/e2e/support/onboarding-helpers.ts |
+91 lines | Shared helper functions for onboarding E2E tests |
tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts |
+122 lines | Regression tests for #1085, #1077 |
tests/e2e/snapshots/onboarding.snapshot.spec.ts |
+55/-72 | Updated step order and routes |
🟡 Suggestion: Consider extracting showOnboarding to helpers
The showOnboarding function in mock-llm-onboarding-regressions.spec.ts is 38 lines of localStorage setup + route configuration. This pattern will likely be needed by other test files. Moving it to onboarding-helpers.ts would improve reuse.
// In tests/e2e/support/onboarding-helpers.ts
export async function showOnboarding(page: Page, apiKey: string) {
await page.addInitScript(
({ apiKey }) => {
// ... localStorage setup
},
{ apiKey },
);
await routeSessionApiKey(page);
await routeOnboardingLlmCatalog(page);
await page.goto('/', { waitUntil: 'domcontentloaded' });
await expect(page.getByTestId('onboarding-modal')).toBeVisible({
timeout: 20_000,
});
}🟡 Suggestion: Add regression test for #1085 - Continue from previous state
The 'keeps the modal open on backdrop click and Escape' test covers dismissing, but consider adding a complementary test that verifies clicking outside before any step is interacted with also doesn't mark onboarding complete (the original bug manifested on very first load).
✅ Design Notes (acceptable)
- The step constants (
ONBOARDING_BACKEND_STEP, etc.) are 0-indexed integers matchingdata-current-step, which is consistent - Using
dispatchEvent('click')instead of.click()is the right call for absolutely-positioned slides - The
networkidlewait has been replaced withwaitForOnboardingLlmSettingsReady- this is more deterministic
Minor: Route change comment
The snapshot tests now go to '/' instead of '/conversations'. This is intentional for the new onboarding flow, but worth confirming this doesn't affect other test suites that may assume the /conversations entry point.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the follow-up AI review in de37952: moved the onboarding modal setup into the shared onboarding helper, and clarified that the dismissal regression test exercises the first-load path before any step interaction.\n\nLocal checks run again: targeted ESLint, npm run typecheck, mock-LLM onboarding regression spec, and onboarding snapshot spec.\n\n_This comment was generated by an AI agent (OpenHands) on behalf of the user._ |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
PR Review: test: add onboarding regression E2E coverage
Overview
This PR adds mock-LLM browser-based E2E coverage for two onboarding regressions:
- #1085/#1100: Backdrop click or Escape must not dismiss onboarding or mark it complete
- #1077/#1089: First-run LLM setup should default to Anthropic Claude Opus (not OpenHands provider)
Files changed:
tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts(new, 95 lines)tests/e2e/snapshots/onboarding.snapshot.spec.ts(modified, +55/-72 lines)tests/e2e/support/onboarding-helpers.ts(new, 135 lines)
Code Review
Strengths
-
Clear regression documentation: Both tests include inline comments referencing the GitHub issues/PRs they cover, making the intent self-documenting.
-
Meaningful assertions: Tests use descriptive messages in
expect()calls (e.g., "onboarding modal should ignore backdrop clicks and Escape"), which helps debugging when tests fail. -
Proper test isolation: Using
localStorage.removeItem("openhands-onboarded")inshowOnboarding()ensures each test starts fresh. -
Deterministic mocking: The
routeOnboardingLlmCatalog()helper provides predictable LLM provider/model responses, avoiding flakiness. -
Appropriate test mode: Using
test.describe.configure({ mode: "serial" })is the right choice for onboarding flow tests that share localStorage state. -
Good helper abstractions: Moving common helpers to
onboarding-helpers.tsreduces duplication and centralizes onboarding-specific logic. -
Step reordering is well-integrated: The snapshot spec update correctly reflects the backend-first onboarding order.
Minor Suggestions
-
Potential race in
advanceOnboardingToLlmStep(): The function advances through steps without waiting for the backend health probe to settle. The caller in the regression test does wait separately viaadvanceOnboardingToLlmStep(page)followed bywaitForOnboardingBackendConnected(page)beforeadvanceOnboardingToLlmStep(page). However, the internalwaitForOnboardingBackendConnected()call should be sufficient — just verifying the pattern is consistent. -
Snapshot file naming: The new snapshot
onboarding-step-0-check-backend.pngfollows the existing naming convention. Good consistency. -
Timeout values: All timeout values (10s, 15s) are reasonable for E2E tests running in CI environments.
Test Plan Verification
The PR includes the following test commands:
npm run typecheck
npx eslint tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts tests/e2e/snapshots/onboarding.snapshot.spec.ts
MOCK_LLM_SESSION_API_KEY=mock-llm-onboarding-key MOCK_LLM_PYTHON=.mock-llm-venv/bin/python3 npx playwright test --config=playwright.mock-llm.config.ts tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts
npx playwright test tests/e2e/snapshots/onboarding.snapshot.spec.ts --project=chromium --update-snapshots --reporter=listThese commands are comprehensive and cover the new tests and updated snapshots.
Conclusion
This PR is well-structured and adds valuable regression coverage. The tests are clear, well-documented, and follow the project's testing patterns. No blocking issues identified.
Recommendation: Approve
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
🚀 Released in v1.2.0. |
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ No baseline found on main — all 73 snapshots are new and will become the baseline once this PR merges.
🆕 New snapshots (73)These snapshots have no baseline on main and will become the new baseline once this PR merges.
|









































































Why
Recent issue/PR triage over the last 48 hours turned up several regressions that should stay covered by E2E tests. The static/public auth regressions already have mock-LLM E2E coverage, so this PR adds mock-LLM browser coverage for onboarding regressions that were only covered at component level:
Summary
tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.tswith two Playwright regression tests for the onboarding dismissal and LLM default flows.OnboardingHostmounts.Test Plan
npm run typechecknpx eslint tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.ts tests/e2e/snapshots/onboarding.snapshot.spec.tsMOCK_LLM_SESSION_API_KEY=mock-llm-onboarding-key MOCK_LLM_PYTHON=.mock-llm-venv/bin/python3 npx playwright test --config=playwright.mock-llm.config.ts tests/e2e/mock-llm/mock-llm-onboarding-regressions.spec.tsnpx playwright test tests/e2e/snapshots/onboarding.snapshot.spec.ts --project=chromium --update-snapshots --reporter=listThis PR was created by an AI agent (OpenHands) on behalf of the user.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a6de379529d6444ebae0588c7997ff575920c3ea47Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-de37952Run
All tags pushed for this build
About Multi-Architecture Support
sha-de37952) is a multi-arch manifest supporting both amd64 and arm64sha-de37952-amd64) are also available if needed