mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 16:08:23 +08:00
test(snapshot): changes tab diff viewer + backend management UI (6 tests) (#450)
* test(snapshot): changes tab diff viewer + backend management UI (6 tests) Pre-seed MOCK_GIT_CHANGES with M/A/D entries (using AgentServerGitChangeStatus values: UPDATED/ADDED/DELETED) so changes-tab tests can exercise the file list, Monaco diff viewer, and deleted-file placeholder without per-test MSW manipulation. Expose window.__setMockGitChanges__ so the empty-state test can clear the list after boot and trigger a React Query refetch via __TEST_INVALIDATE_QUERIES__, avoiding a full page reload that would reinitialise module state. Backend management tests exercise the selector dropdown, add-backend modal, and manage-backends modal — all driven by localStorage seeding via addInitScript. Co-authored-by: openhands <openhands@all-hands.dev> * chore: update baseline snapshots [skip ci] * ci: trigger re-run against CI-generated baselines * fix(snapshot-tests): mask Monaco editor for stable CI screenshots; fix unit test - changes-tab spec: mask data-testid=editor-container so Monaco's sub-pixel font hinting (which varies per OS) doesn't cause false pixel-diff failures - mock-conversation-handlers test: update assertion to match the new pre-seeded MOCK_GIT_CHANGES (3 M/A/D entries) instead of the previous empty array Co-authored-by: openhands <openhands@all-hands.dev> * chore: update baseline snapshots [skip ci] * ci: trigger re-run against CI-regenerated baselines (Monaco mask + unit test fix) * fix(snapshot-tests): normalize RandomTip height via addStyleTag for stable empty-state screenshot RandomTip renders a randomly-chosen tip whose line-count varies, causing the flex-1 container above it to have different heights across runs. Fix by injecting a CSS rule via page.addStyleTag() that pins .text-m.bg-tertiary.p-4 to 80px (visibility:hidden so the variable text is invisible) — layout is now deterministic. Switch back to screenshotting the full files-tab panel since dimensions are stable. Co-authored-by: openhands <openhands@all-hands.dev> * chore: update baseline snapshots [skip ci] * ci: trigger re-run against baselines (empty-state RandomTip height fix) * fix(snapshot-tests): use inner content div for empty-state screenshot to avoid left-strip artefact Screenshot files-tab's last direct div child (the flex-1 content wrapper) instead of the outer main element. During CI baseline generation the outer main's bounding box occasionally captured a ~30px left-panel overlay artefact that made the baseline permanently diverge from subsequent verification runs. Targeting the inner wrapper excludes the outer-element overflow while still showing the full empty-state (icon + 'no changes yet' text + hidden tip area). Co-authored-by: openhands <openhands@all-hands.dev> * chore: update baseline snapshots [skip ci] * ci: validate against fresh inner-div empty-state baseline * test(snapshot): extended backend UI flows — 12 tests, 19 screenshots Add backends-extended.snapshot.spec.ts covering 8 behaviour flows with iterative screenshot captures at each state transition: Flow 1a Blank add form — Save disabled until name+host filled Flow 1b Local backend — Save enabled with name+host, no API key needed Flow 1c Cloud backend — Save disabled without API key, enabled with it Flow 2a Host auto-infers Local kind; OAuth section disappears Flow 2b Cloud-domain URL keeps Cloud kind; OAuth section shows Flow 2c Manual kind selection locks type (touchedKind=true) even when a cloud URL is later typed into the Host field Flow 3 OAuth Login button disabled while host is empty; enabled once filled Flow 4 Remove backend: shows ConfirmationModal → Cancel keeps row → Confirm removes it from the list (4 screenshots) Flow 5 Edit modal pre-populates name/host/key from stored backend Flow 6 Switch active backend: environment-switch overlay captured via page-level screenshot + animation override so the card is opaque at frame-0; after-switch state verified via selector label Flow 7 Whitespace-only host keeps Save disabled; syntactically invalid URL is accepted by the frontend (no URL-format validation) Flow 8 Cancel add form: dismisses modal, Manage Backends confirms no phantom entry was saved Notable decisions: - Uses body[data-environment-switching="true"] as the early DOM signal before React paints the portal div for the switch overlay - Adds inline style-tag override before the overlay screenshot because .environment-switch-overlay > div has opacity:0 at animation frame 0; Playwright's animations:"disabled" pauses there, making the card invisible without the override - Backends seeded via page.addInitScript localStorage injection so tests are fully self-contained with no MSW state dependency Co-authored-by: openhands <openhands@all-hands.dev> * chore: update baseline snapshots [skip ci] * ci: validate extended backend snapshot tests against CI baselines * ci: always post snapshot PR comment even when test generation step fails The 'Post snapshot report to PR' step was skipped whenever 'Generate current PR snapshots' exited non-zero (e.g. a test crash like a hidden element, not just a snapshot diff). GitHub Actions skips steps without an always() guard when a prior step fails. Add always() so the comment is posted regardless — showing diffs or the test failure output — which was the intended behaviour. Co-authored-by: openhands <openhands@all-hands.dev> * ci: fix snapshot comment - remove tracked screenshots, add crash reporting Three fixes: 1. Remove 28 git-tracked snapshot PNGs from this branch. These were committed by the old baseline-in-git workflow before #482 migrated to artifact storage. Because they stayed tracked (gitignore doesn't untrack already-indexed files), every CI checkout put them in tests/e2e/__snapshots__/ BEFORE the baseline artifact was downloaded. The Save step then copied them into /tmp/main-baselines, making the new tests appear as 'Unchanged' instead of 'New' in the PR comment. 2. Add 'Clear snapshot directory before downloading baselines' step. Wipes tests/e2e/__snapshots__/ before the artifact download so any future accidentally-tracked files can never contaminate the baseline. 3. Surface test crashes in the PR comment. - Generate step gets continue-on-error + an id so subsequent steps can read its outcome. - GENERATE_OUTCOME is passed to the comment script. - If outcome == 'failure', a GitHub-flavoured WARNING callout is prepended to the comment with a direct link to the CI run logs. - A dedicated 'Fail if snapshot generation had test crashes' step restores the job failure that continue-on-error absorbed. Co-authored-by: openhands <openhands@all-hands.dev> * ci: use PR number in snapshot concurrency group for cleaner cancellation The previous group used github.ref which resolves to refs/pull/{N}/merge for PR events — correct but opaque. Using github.event.pull_request.number makes the grouping explicit and human-readable (snapshot-tests-450), and falls back to github.ref for main pushes and workflow_dispatch. cancel-in-progress: true was already set, so new commits already cancelled prior runs. This just makes the intent clearer. Co-authored-by: openhands <openhands@all-hands.dev> * fix: syntax error in post-snapshot-comment.mjs (] vs ) in lines.push) lines.push(...) was accidentally closed with ]; instead of ); after splitting the original lines = [...] array literal into a push call. Caused a SyntaxError at startup, preventing any comment from being posted. Co-authored-by: openhands <openhands@all-hands.dev> * fix: snapshot test disabled states, changes-tab crash, and CI false-failures Three fixes: 1. BrandButton disabled visual styling (brand-button.tsx) disabled:opacity-30 pseudo-class was not applying in Vite dev mode (Tailwind v4 + postcss-prefix-selector interaction), making disabled and enabled buttons visually identical in snapshot screenshots. Fix: add isDisabled conditional class directly ('opacity-30 cursor-not-allowed pointer-events-none') so the disabled appearance is applied regardless of whether :disabled pseudo-class works. 2. changes-tab test crash (changes-tab.snapshot.spec.ts) Test waited for data-testid='files-tab' but the right panel always starts CLOSED (isRightPanelShown = false is session-only Zustand state; sanitizeStoredState strips any persisted rightPanelShown key). Fix: click data-testid='right-panel-toggle' after navigation to open the panel before waiting for files-tab. Also remove the no-op rightPanelShown: true from the localStorage seed. 3. CI false-failures for new snapshot tests (snapshot-tests.yml + post-snapshot-comment.mjs) The 'Fail if comparison found differences' step fired on 'missing baseline' failures (expected for new tests in a PR) as well as actual pixel-diff failures. Fix: - post-snapshot-comment.mjs outputs has_changes=true/false to GITHUB_OUTPUT (true only when changed.length > 0, i.e. real diffs) - 'Fail if' step now checks steps.post-comment.outputs.has_changes == 'true' instead of compare.outcome == 'failure', so PRs that only add new snapshot tests pass CI cleanly. Co-authored-by: openhands <openhands@all-hands.dev> * fix: reject invalid host URLs in backend form; use http for local addresses Two related fixes to backend host validation / normalisation: 1. isValidHostUrl() — reject invalid host strings canSubmit previously only checked host.trim().length > 0, so garbage like 'not://:::a valid url!!!' passed through and enabled the Save button. isValidHostUrl() adds two checks before the URL constructor: (a) the trimmed value must be non-empty, (b) it must contain no whitespace. This catches the test-case input whose spaces are the tell-tale sign of a malformed value. 2. normalizeHost() — http:// for local addresses Bare hostnames (no explicit scheme) were unconditionally prepended with https://, but local servers almost never have TLS certificates. The new isLocalAddress() helper detects localhost, 127.x, RFC-1918 private ranges (10.x, 192.168.x, 172.16-31.x), .local / mDNS names, and single-label hostnames — all get http:// instead of https://. Hostnames with dots that are not in those ranges (e.g. app.all-hands.dev) still default to https://. Explicit http:// or https:// prefixes are always preserved as-is. Test update: the 'backend-add-invalid-url-accepted' snapshot is renamed to 'backend-add-invalid-url-disabled' and the assertion flips from not.toBeDisabled() → toBeDisabled(), reflecting the new behaviour. Co-authored-by: openhands <openhands@all-hands.dev> * feat: inline error feedback on Name and Host fields in BackendForm Three parts: 1. SettingsInput gains error / showRequiredTag / onBlur props - error?: string — red border on the input plus a small red alert paragraph below it (role=alert, data-testid=${testId}-error, linked via aria-describedby). - showRequiredTag?: boolean — renders a red * after the label to signal that the field is mandatory, consistent with OptionalTag. - onBlur?: () => void — forwarded directly to the <input>. - aria-invalid is set automatically when error is truthy. 2. BackendForm wires touched state → errors → inputs - nameTouched / hostTouched (both false on open, set on blur) - nameError: 'Name is required' when touched + empty - hostError: 'Host is required' when touched + blank/whitespace; 'Enter a valid URL (e.g. http://localhost:8080)' when touched + non-empty but fails isValidHostUrl() - Both name and host SettingsInputs get showRequiredTag, the computed error, and onBlur={() => setXTouched(true)}. Errors are intentionally suppressed until blur so the form does not scold the user before they have had a chance to type anything. 3. Three snapshot tests call .blur() after .fill() to reveal errors - backend-add-name-only-disabled: focus+blur empty host → 'Host is required' appears below the Host field. - backend-add-whitespace-host-disabled: blur after fill(' ') → same 'Host is required' (whitespace counts as empty). - backend-add-invalid-url-disabled: blur after invalid URL fill → 'Enter a valid URL...' appears below the Host field. The backend-add-blank-disabled snapshot is unchanged (neither field touched, no errors yet — correct for the fresh-open state). New i18n keys: BACKEND$NAME_REQUIRED, BACKEND$HOST_REQUIRED, BACKEND$HOST_INVALID (English only; other locales fall back to en). Co-authored-by: openhands <openhands@all-hands.dev> * fix: prettier formatting on nameError / hostError ternaries Co-authored-by: openhands <openhands@all-hands.dev> * fix: disable OAuth Login button until name and host are both valid Previously the 'Login with OpenHands' button was enabled as soon as a non-empty host was typed, even when the Name field was still blank. This let users go through the full OAuth device-flow only to find they still couldn't save because the name was missing. Gate isDisabled on !name.trim() || !isValidHostUrl(host) so the button stays disabled until the form is actually ready to save (modulo the API key that OAuth itself will provide). Update Flow 3 snapshot test to fill the name before asserting the button becomes enabled, and update the test description accordingly. Co-authored-by: openhands <openhands@all-hands.dev> * chore: address PR review feedback (#450) IPv6 parsing fixes (normalizeHost / isLocalAddress): - normalizeHost: handle bracket notation [::1]:8080 (extract ::1), bare IPv6 addresses with multiple colons (use whole string as hostname), and regular host:port as before — prevents split(':')[0] from grabbing only the first segment of a multi-colon IPv6 address - isLocalAddress: strip brackets before comparison; add :: (any-addr), ::ffff:127.x.x.x (IPv4-mapped loopback), fe80::/10 (link-local), fc00::/7 (unique local); tighten single-label check to exclude addresses that contain colons (bare IPv6 non-local addresses) Mark fields touched on submit attempt: - handleSubmit sets nameTouched + hostTouched when !canSubmit so inline errors appear for keyboard users who press Enter on an incomplete form Snapshot workflow comparison-crash detection: - Pass COMPARE_OUTCOME=${{ steps.compare.outcome }} to post-comment - post-snapshot-comment.mjs reads COMPARE_OUTCOME and prepends a '[!WARNING]' block when the comparison step itself crashed (timeout/OOM) so the comment accurately reflects the run state instead of silently showing an incomplete/empty diff table Remove unnecessary serial mode from backends-extended snapshot suite: - Each test calls setupPage() with fresh state on its own Playwright page; no shared mutable state exists between tests, so serial is unnecessary and slows the suite Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This commit is contained in:
Rohit Malhotra
committed by
GitHub
GitHub
co-authored by
openhands
github-actions[bot]
openhands
github-actions[bot]
parent
b2b71855c6
commit
14d2e9454b
@@ -14,7 +14,10 @@ on:
|
||||
default: false
|
||||
|
||||
concurrency:
|
||||
group: snapshot-tests-${{ github.workflow }}-${{ github.ref }}
|
||||
# One run at a time per PR — a new commit cancels the in-flight run.
|
||||
# Falls back to github.ref for main pushes and manual dispatch so those
|
||||
# don't interfere with each other or with PR runs.
|
||||
group: snapshot-tests-${{ github.event.pull_request.number || github.ref }}
|
||||
cancel-in-progress: true
|
||||
|
||||
permissions:
|
||||
@@ -134,6 +137,10 @@ jobs:
|
||||
echo "Found snapshot-baselines artifact from run $RUN_ID"
|
||||
fi
|
||||
|
||||
- name: Clear snapshot directory before downloading baselines
|
||||
if: github.event_name == 'pull_request'
|
||||
run: rm -rf tests/e2e/__snapshots__/
|
||||
|
||||
- name: Download main-branch baselines
|
||||
if: github.event_name == 'pull_request' && steps.find-run.outputs.has_baselines == 'true'
|
||||
uses: actions/download-artifact@v4
|
||||
@@ -176,10 +183,13 @@ jobs:
|
||||
|
||||
- name: Generate current PR snapshots
|
||||
if: github.event_name == 'pull_request'
|
||||
id: generate
|
||||
run: npm run test:e2e:snapshots:update
|
||||
continue-on-error: true
|
||||
|
||||
- name: Post snapshot report to PR
|
||||
if: github.event_name == 'pull_request'
|
||||
if: always() && github.event_name == 'pull_request'
|
||||
id: post-comment
|
||||
env:
|
||||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
@@ -189,6 +199,8 @@ jobs:
|
||||
MAIN_BASELINES_DIR: /tmp/main-baselines
|
||||
COMPARISON_RESULTS_DIR: /tmp/comparison-results
|
||||
SNAPSHOTS_APPROVED: ${{ contains(github.event.pull_request.labels.*.name, 'update-snapshots') }}
|
||||
GENERATE_OUTCOME: ${{ steps.generate.outcome }}
|
||||
COMPARE_OUTCOME: ${{ steps.compare.outcome }}
|
||||
run: node tests/e2e/snapshots/scripts/post-snapshot-comment.mjs
|
||||
|
||||
- name: Upload test results artifact
|
||||
@@ -202,11 +214,22 @@ jobs:
|
||||
retention-days: 7
|
||||
|
||||
- name: Fail if snapshot comparison found differences
|
||||
# Only fail on real pixel-diff failures (changed snapshots), not on
|
||||
# missing-baseline failures which are expected for new tests added in
|
||||
# this PR. `has_changes` is written by post-snapshot-comment.mjs to
|
||||
# $GITHUB_OUTPUT; it is false when all failures are "new" snapshots.
|
||||
if: >-
|
||||
github.event_name == 'pull_request' &&
|
||||
steps.compare.outcome == 'failure' &&
|
||||
steps.find-run.outputs.has_baselines == 'true' &&
|
||||
steps.post-comment.outputs.has_changes == 'true' &&
|
||||
!contains(github.event.pull_request.labels.*.name, 'update-snapshots')
|
||||
run: |
|
||||
echo "::error::Snapshot differences detected. Add the 'update-snapshots' label to acknowledge intentional changes."
|
||||
exit 1
|
||||
|
||||
- name: Fail if snapshot generation had test crashes
|
||||
if: >-
|
||||
github.event_name == 'pull_request' &&
|
||||
steps.generate.outcome == 'failure'
|
||||
run: |
|
||||
echo "::error::One or more snapshot tests crashed during generation. See the PR comment and CI logs for details."
|
||||
exit 1
|
||||
|
||||
Reference in New Issue
Block a user