Define e2e/lifecycle test taxonomy, inventory specs, and run lifecycle tests nightly#1247
Open
Kosinkadink wants to merge 15 commits into
Open
Define e2e/lifecycle test taxonomy, inventory specs, and run lifecycle tests nightly#1247Kosinkadink wants to merge 15 commits into
Kosinkadink wants to merge 15 commits into
Conversation
…le nightly The lifecycle Playwright project was never wired into CI, so its tests went stale (#1242). This defines the test categories and revives the suite as an automated run: - e2e/README.md: category definitions (unit / integration / e2e / lifecycle), the zero-mock policy for lifecycle tests, and a per-spec inventory noting which @lifecycle specs inject synthetic state and are retag candidates - test:e2e:lifecycle script (playwright test --project=lifecycle) - .github/workflows/lifecycle.yml: nightly + manual lifecycle run on windows-latest, non-PR-blocking, failure artifacts uploaded - README: document the new script
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesLifecycle testing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
- Carve out the harness's global dialog.showErrorBox no-op and native OS file-dialog path stubs as allowed scaffolding - Correct SeedOptions timing: installations/snapshots are written after launch, not before - Reclassify dismiss-error, progress-reboot, deep-links, and nav-matrix-cloud as retag candidates based on actual runtime injection evidence - Recast first-use-migrate and first-use-skip as strengthen-or-retag
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/lifecycle.yml:
- Around line 38-39: Update the lifecycle workflow’s “Build app” step to fetch
or create the required bootstrap-python directory before running pnpm run build,
following the repository’s README bootstrap instructions. Ensure the bootstrap
setup completes first so lifecycle tests and the build use the bundled Git
backend.
- Around line 41-42: Update the “Run lifecycle tests” workflow step to install
the required Playwright browser binaries before executing test:e2e:lifecycle,
using the repository’s existing package manager and Playwright setup
conventions.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 78b33e11-868b-47f0-b7b7-08ebc673f468
📒 Files selected for processing (4)
.github/workflows/lifecycle.ymlREADME.mde2e/README.mdpackage.json
…ence - Allow public window.api triggers (runAction, probeInstallation) as scaffolding; forbid synthesized internal IPC and renderer-store writes - Correct lifecycle-deep-links evidence: it replays panel-trigger-overlay directly, never exercising real comfy:// handling - Drop the zero-mock label from the root README script row - Drop the PT translation from the nightly cron comment
Lifecycle tests exist to replace manual click-through testing, so a broken button must fail them. Direct window.api triggers (runAction, probe/track) and synthesized internal IPC are now forbidden; read-only queries remain fine for assertions. Reclassifies 8 more specs as strengthen-or-retag: add-existing, copy, copy-update-fail, migrate, snapshot, snapshot-restore, update-check, and lifecycle.test.ts itself.
The Update-tab watcher fired check-update on every picker open, even when the selected channel card was checked seconds ago. The per-instance dedupe set only stops repeats within one component instance, and the picker remounts on every reopen — so fresh caches were re-fetched anyway, and the lifecycle dedupe test caught it. Gate the watcher on data.lastCheckedAt being missing or older than STALE_CHANNEL_CARD_MS (15 min), matching the original behavior from b13636c which is not an ancestor of this branch. Also fix lifecycle-update-check.test.ts to read lastCheckedAt (the actual channel-card field) instead of checkedAt, and add unit coverage for the staleness gate.
…ndow.api Rewrites lifecycle-update-check, lifecycle-snapshot, lifecycle-copy, lifecycle-snapshot-restore, and lifecycle-copy-update-fail to click the actual settings tabs, update buttons, snapshot CTA, and prompt inputs a user would use, per the no-direct-window.api policy. Adds the testids those flows need (settings tabs, compact check-update button, snapshot save CTA). copy-update-fail now seeds a source with ComfyUI/.git plus stale release metadata so the copy-update button is genuinely reachable through the UI, and asserts the failure via the durable app log since popup preloads don't expose comfy-output.
Continuing past the start screen into the Local branch pushed firstUseMode 'none' twice (FirstUseTakeover unmount + the overlay watcher) before the chain re-asserted 'post-consent' — and that re-assert waited on the install wizard's network-bound open(). For those seconds the waffle menu offered the full file menu instead of the single Skip Onboarding escape. Assert 'post-consent' up front in the chain handler and skip the 'none' pushes for chain handoffs: FirstUseTakeover tracks chain exits and the overlay watcher exempts the first-use -> takeover swap while the chain is in flight. The chain-migrate path re-asserts immediately after dismissTakeoverDirect instead of after the migration op.
Rewrites the rest of the lifecycle suite to fire flows from the controls a user clicks, per the zero-mock policy: - lifecycle.test.ts: restart via picker primary CTA, stop/relaunch via More menu + stopped-card Relaunch, copy via picker/dashboard-kebab prompts (BasePrompt ids, registry-record polling), untrack confirm in the panel's BaseAlert, RTD confirm helper covers panel and system-modal surfaces. - lifecycle-add-existing: waffle menu -> Add Existing Instance -> TrackModal -> Browse -> Track (only the native dir picker stubbed). - lifecycle-first-use-skip: real ToS -> Local -> Continue -> waffle Skip Onboarding. - lifecycle-first-use-migrate: adoption op really runs; the stub .venv raises the real venv-broken prompt, answered Cancel; failure asserted in the progress error surface. - lifecycle-migrate: chooser kebab -> Migrate -> rich adoption confirm in the picker popup -> Cancel; R2 CPU-variant contract guard kept. - lifecycle-delete-untrack, lifecycle-snapshot-roundtrip, nav-matrix-cloud: kebab/context-menu/picker window-options flows. - support: hasActiveOperation dev hook, WebContentsView-aware findWebContentsId, evalWithRetry hardening. - testIds: pickerPrimaryCta, pickerMoreTrigger, lifecycleRelaunch production hooks; drop unused pickerRowOpen/pickerRowManage.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@e2e/lifecycle-add-existing.test.ts`:
- Around line 101-130: Make the TrackModal assertions self-contained by ensuring
the test covering `#track-name` and the test covering the probed summary both
perform the title-bar “Add Existing Instance” navigation and Browse setup, or
merge them into one test. Do not rely on modal state left by a preceding test,
so focused execution and worker restarts can run each test independently.
In `@e2e/lifecycle-copy-update-fail.test.ts`:
- Around line 124-132: Update the confirmation handling around confirmSelector
so it waits for either the confirmation control or an observable indication that
the copy operation has started, rather than relying on the fixed 2-second
timeout. Click the confirmation when it appears, while allowing the progress
path to continue when no confirmation is required; preserve behavior across slow
Windows runs without introducing process-lifecycle races.
In `@e2e/lifecycle-first-use-skip.test.ts`:
- Around line 37-40: Update the comment in the test case to describe the merged
consent and cloud/local selection start screen without mentioning the
originating commit hash, leaving the hero and Continue CTA comments unchanged.
In `@e2e/lifecycle-update-check.test.ts`:
- Around line 139-181: Update the test around openPickerOnUpdateTab,
checkButton, and the latestVersion poll to distinguish the manual check from the
tab-open auto-refresh: after the button becomes enabled, reset the run-action
state, click the button, and explicitly wait for a new invocation plus operation
drain or button re-enablement before asserting the remote result and closing the
popup. Use the existing readiness signals/helpers rather than relying on
latestVersion data alone.
In `@e2e/lifecycle.test.ts`:
- Around line 706-719: Update the confirmation handling around panel and
system-modal clicks so clicked is set to true and the loop exits only when the
respective click call returns true. If either click returns false because the
surface disappeared, leave clicked false and continue polling through the
existing lifecycle readiness flow.
- Around line 245-252: Extract the repeated Electron/DOM probe used by both
waits into a shared helper near the lifecycle test utilities, using the existing
webContents lookup and .brand-primary.config-continue selector. Replace both
inline evalWithRetry callbacks with calls to that helper, preserving the current
behavior of returning false when panel.html is unavailable and true only when
the Continue button is enabled.
- Around line 1181-1212: Extract the duplicated prompt-input population and
copy-completion registry polling from the copy flow and the corresponding flow
near the referenced second occurrence into shared helpers in the test file.
Reuse those helpers in both flows, preserving the existing input/change event
behavior, completion timeout and polling intervals, and assignment of the copied
installation record and identifiers.
In `@src/renderer/src/views/FirstUseTakeover.vue`:
- Around line 275-278: Update resetContinue() to clear chainHandoff, and move
the exit-path assignment in emitCompleted() before the completedFired
early-return so every retry or alternate exit recomputes the current handoff
state. Apply the same ordering/reset behavior to the additional emitCompleted
call sites.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 020e4798-cf01-4c56-8505-af03538eb135
📒 Files selected for processing (26)
e2e/README.mde2e/lifecycle-add-existing.test.tse2e/lifecycle-copy-update-fail.test.tse2e/lifecycle-copy.test.tse2e/lifecycle-delete-untrack.test.tse2e/lifecycle-first-use-migrate.test.tse2e/lifecycle-first-use-skip.test.tse2e/lifecycle-migrate.test.tse2e/lifecycle-snapshot-restore.test.tse2e/lifecycle-snapshot-roundtrip.test.tse2e/lifecycle-snapshot.test.tse2e/lifecycle-update-check.test.tse2e/lifecycle.test.tse2e/nav-matrix-cloud.test.tse2e/support/cdpPages.tse2e/support/devHooks.tssrc/main/lib/e2eHooks.tssrc/renderer/src/components/settings/ComfyUISettingsContent.test.tssrc/renderer/src/components/settings/ComfyUISettingsContent.vuesrc/renderer/src/panel/ComfyLifecycleView.vuesrc/renderer/src/panel/useFirstUseChain.tssrc/renderer/src/panel/usePanelOverlays.tssrc/renderer/src/views/FirstUseTakeover.vuesrc/renderer/src/views/comfyUISettings/ChannelPicker.vuesrc/renderer/src/views/comfyUISettings/SnapshotsView.vuesrc/shared/testIds.ts
No renderer ever calls previewDesktopMigration: desktop adoption is an in-place reuse confirm (useAdoptAction) with no snapshot preview, and the standalone migrate path uses previewLocalMigration. Remove the dead main handler, both preload bridges, the titlepopup shim entry, the channel constant, and the now-orphaned stageDesktopSnapshot helper.
…jects These specs inject synthetic runtime state (seedRunningSession, store seeding, synthetic op errors) or assert IPC dispatch instead of real side effects, which makes them E2E tests under the taxonomy, not zero-mock lifecycle tests. Tag them @windows @MacOS @linux so they join PR-blocking CI, and drop the lifecycle- filename prefix where present. Stability check on Windows surfaced a real race in picker-stop-confirm: the run-action count poll could be satisfied by the picker's auto check-update before the self-stop wrapper dispatched update-comfyui, making the follow-up single read flake. Poll for the update-comfyui dispatch itself instead.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/desktopDetect.ts (1)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
captureDesktopSnapshotif nothing else calls it.
No in-repo references remain, so this looks like orphaned code—time to prune the snap before it becomes a fossil.🤖 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 `@src/main/lib/desktopDetect.ts` at line 153, Remove the exported captureDesktopSnapshot function from desktopDetect.ts because it has no remaining in-repository callers. Also remove any imports, types, or helper code used exclusively by this function, while preserving code still used elsewhere.
🤖 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 `@e2e/picker-stop-confirm.test.ts`:
- Around line 243-252: Fix the duplicated type assertion in the
getIpcInvocations call so the calls result is cast only once. Extract the
repeated run-action polling logic into a shared variable or helper, then reuse
it for both checks while preserving the existing timeout, intervals, and
actionId matching behavior.
- Around line 198-207: Remove the duplicated type assertion in the
getIpcInvocations call so the calls value has one valid array type annotation.
Extract the repeated run-action polling logic into a shared helper or variable,
parameterized by the required action identifier, and reuse it for both polling
checks to prevent divergence.
---
Outside diff comments:
In `@src/main/lib/desktopDetect.ts`:
- Line 153: Remove the exported captureDesktopSnapshot function from
desktopDetect.ts because it has no remaining in-repository callers. Also remove
any imports, types, or helper code used exclusively by this function, while
preserving code still used elsewhere.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e34c1a5b-8875-4ff9-b2b9-6a07af431168
📒 Files selected for processing (22)
e2e/README.mde2e/cancel-flow.test.tse2e/copy-update-destination.test.tse2e/deep-links.test.tse2e/dismiss-error.test.tse2e/lifecycle-first-use-migrate.test.tse2e/nav-matrix-dashboard.test.tse2e/nav-matrix-instance.test.tse2e/picker-cluster.test.tse2e/picker-settings-staleness.test.tse2e/picker-stop-confirm.test.tse2e/port-conflict.test.tse2e/progress-error-overflow.test.tse2e/progress-reboot.test.tssrc/main/lib/desktopDetect.tssrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/ipc/shared.tssrc/main/popups/pickerSettingsHandlers.tssrc/preload/api.tssrc/preload/comfyTitlePopupPreload.tssrc/renderer/src/comfyTitlePopup/pickerSettingsApiShim.tssrc/types/ipc.ts
💤 Files with no reviewable changes (6)
- src/preload/api.ts
- src/main/lib/ipc/registerSnapshotHandlers.ts
- src/main/popups/pickerSettingsHandlers.ts
- src/preload/comfyTitlePopupPreload.ts
- src/renderer/src/comfyTitlePopup/pickerSettingsApiShim.ts
- src/types/ipc.ts
Pull the overlay watcher's lockdown branching into a pure firstUseModeForOverlaySwap so the chain-handoff exemption is directly testable: the first-use -> takeover swap must not clobber the chain's post-consent assert, and a stale chain flag must never suppress the none push on ordinary overlay closes.
Member
Author
Code review (agent-assisted, full branch diff vs origin/main)Verdict: no blockers found. Reviewed with priority on the risk areas of this branch:
|
One practical entry point for the four test categories (unit / integration / E2E / lifecycle): what each may fake, where specs live, run commands, which category a new test belongs in, and the CI mapping. Deep policy and the per-spec inventory stay in e2e/README.md.
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 `@src/renderer/src/panel/usePanelOverlays.ts`:
- Around line 155-167: In firstUseModeForOverlaySwap, compute
isProgressTakeover(next) once in a shared local variable and reuse it for both
conditional checks, preserving the existing return behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 648d61b0-352b-4ee6-b0fb-0fb7dc806c87
📒 Files selected for processing (4)
README.mdTESTING.mdsrc/renderer/src/panel/usePanelOverlays.test.tssrc/renderer/src/panel/usePanelOverlays.ts
…etch - FirstUseTakeover: recompute chainHandoff before the completed-dedupe early return and clear it in resetContinue so a cancelled chain can't leave a stale handoff suppressing the unmount 'none' push - lifecycle.yml: fetch bootstrap Python before build so lifecycle CI exercises the bundled git backend instead of the runner's system git - e2e: merge add-existing into one self-contained test; replace the fixed 2s confirm guess in copy-update-fail with a confirm-or-started poll; assert the manual check-update dispatch (not auto-refresh data) in update-check; only mark the RTD confirm clicked when the click lands; extract shared helpers (config Continue probe, copy name prompt + registration poll, forwarded update dispatch poll) - usePanelOverlays: reuse the isProgressTakeover(next) result - drop a stale commit-hash reference from a test comment
… hook Seeded install records were written to installations.json after launch, so one-shot hydration missed them, no installations-changed broadcast fired, and the boot-time sweep could delete records whose dirs held only ignored entries. Seed via E2E_INSTALLATIONS_SEED before first load (mirroring E2E_SETTINGS_SEED), create .launcher markers so the sweep keeps seeded installs, harden popup helpers against destroyed webContents, and run the port-override launch test last since it leaves a launch op in flight.
On headless Linux CI, openPath spawned dbus-send (not unref'd) with an xdg-open fallback; the child kept the main process alive on quit, so picker-cluster's afterAll hit the 45s worker-teardown timeout. E2E asserts the run-action IPC fired, not the OS side effect, so gate the real open behind the existing E2E env check.
This was referenced Jul 15, 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.
Closes #1242.
What this PR does
Defines a test taxonomy (unit / integration / E2E / lifecycle), applies it to every Playwright spec, and rewrites the lifecycle suite so it actually replaces manual release click-through testing.
Taxonomy + zero-mock policy (
e2e/README.md).github/workflows/lifecycle.yml(new).window.apidirectly or synthesizing internal IPC. A broken button must fail these tests. Allowed scaffolding (fixture staging, stubbed native OS dialogs, read-only observation hooks) is spelled out in the README.Lifecycle suite rewritten to drive real UI
All lifecycle flows now go through real controls: picker primary CTA, More menu, stopped-card Relaunch, dashboard/chooser kebabs, waffle menu, TrackModal, prompt/confirm surfaces, snapshot rows. Remaining direct bridge calls are fixture setup/cleanup between tests, documented in the README. New test ids:
picker-primary-cta,picker-more-trigger,lifecycle-relaunch.Product bugs the rewrite caught
check-updateon every open because the dedupe set was per-component-instance and the picker remounts on reopen. Fixed by gating auto-refresh onlastCheckedAtolder than 15 min; unit coverage added.firstUseMode('none')before the chain re-assertedpost-consent, briefly exposing the full file menu instead of only Skip Onboarding. Fixed by tracking chain handoffs and re-assertingpost-consentat chain start. Caught by the rewrittenlifecycle-first-use-skip.test.ts- exactly the regression class the zero-mock policy exists for. The overlay watcher's branching is extracted into a purefirstUseModeForOverlaySwapwith table-test coverage, including the stale-chain-flag / ordinary-close case.Retag: 12 seeded-state specs move to platform projects
Specs that inject synthetic runtime state (
seedRunningSession, store seeding, synthetic op errors) or assert IPC dispatch are E2E by the taxonomy, not lifecycle. They are now tagged@windows @macos @linuxand join PR-blocking CI; thelifecycle-filename prefix was dropped where present (e.g.lifecycle-port-conflict.test.ts->port-conflict.test.ts).The Windows stability pass surfaced a real race in
picker-stop-confirm.test.ts: the run-action count poll could be satisfied by the picker's autocheck-updatebefore the self-stop wrapper dispatchedupdate-comfyui, flaking the follow-up read. The test now polls for theupdate-comfyuidispatch itself.Dead code removal
preview-desktop-migrationIPC had no renderer callers (desktop adoption is an in-place reuse confirm with no snapshot preview). Removed the main handler, both preload bridges, the titlepopup shim entry, the channel constant, and the orphanedstageDesktopSnapshothelper.Verification
pnpm run typecheck,typecheck:e2e,lint,build,test(188 files / 2797 tests) - all green.--project=lifecyclesuite green locally on Windows, including the ~500 MB real-install spec.--project=windows(27 tests), plus a repeat-each stability run of the fixedpicker-stop-confirm.test.ts.