Stabilize install app integration test assertion#1459
Conversation
dosubot
bot
added
the
size:XS
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (9)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRemoved the test callback's window parameter, added a new public Changes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
benceruleanlu
added
the
New Browser Test Expectations
|
generating new test expectations, because one of them is an expected failure, actually on second thought I should also move that off of being a snapshot expectation |
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the install app integration test by replacing a brittle screenshot assertion with a more reliable visibility check. The screenshot comparison was prone to false failures due to minor visual variations, while the new approach verifies the essential behavior: that the templates grid is visible after installation.
- Removed screenshot assertion that was causing test flakiness
- Added visibility check for the templates grid with a 30-second timeout
- Maintained existing canvas readiness validation through the
waitUntilLoaded()helper
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/integration/install/installApp.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript as the primary language and maintain exceptionally high type standards - theanytype must not be used.unknowncan be used when the type is unknown.
Use JSDoc format to write documentation for methods, including common tags such as@paramand@return(do not use@returnforvoidreturn types), and use{@link }to reference symbols.
Follow existing TypeScript patterns in the codebase and maintain clean separation between main process, renderer, and preload scripts.
Follow Electron security best practices in TypeScript code.
Runyarn typescriptto perform TypeScript type checking after making changes.
Files:
tests/integration/install/installApp.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint and Prettier for code formatting and linting - run
yarn lintandyarn formatto ensure consistent code style.
Files:
tests/integration/install/installApp.spec.ts
tests/integration/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Test changes with E2E tests (Playwright) for UI changes.
Files:
tests/integration/install/installApp.spec.ts
tests/integration/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/integration-testing.mdc)
tests/integration/**/*.spec.ts: Use Playwright + TypeScript for Electron testing in integration tests
Import fixtures from testExtensions.ts, not raw Playwright
Use fixture classes (TestApp, TestInstallWizard, TestInstalledApp, TestServerStart, TestTroubleshooting, TestGraphCanvas) instead of raw locators
Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
Mark slow tests with test.slow() in integration tests
Add screenshots for visual regression testing in integration tests
Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Trust auto-cleanup via Symbol.asyncDispose rather than manual cleanup in test fixtures
Check CI behavior with process.env.CI in integration tests
tests/integration/**/*.spec.ts: Tests should be stored in subdirectories oftests/integration/
Prefer adding properties to test classes over using locator selectors directly in tests
Use convenience methods and properties defined in helper classes instead of direct locator selectors
Prefer imports fromtestExtensions.tsover Playwright defaults in spec files
Files:
tests/integration/install/installApp.spec.ts
tests/integration/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Use
path.join()andpath.septo ensure file path handling works consistently across Windows, macOS, and Linux platforms
Files:
tests/integration/install/installApp.spec.ts
**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,mts,cts}: Use features available in TypeScript 5.x
Use ESM only (type: module). Avoid CommonJS (require,module.exports)
Target ESNext runtime APIs and syntax. Prefer top-levelawaitonly when it improves clarity
Code must compile withstrictmode andnoImplicitAnyenabled, with zero implicitany
Useunknowninstead ofany. Narrow with type guards.anyis allowed only when interfacing with untyped code and must be localized and justified
Useinterfacefor public object shapes intended for extension/implementation
Usetypefor unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g.,kind: 'X' | 'Y'). Use exhaustiveswitchstatements
Preferas constobjects and string/number literal unions overenum
Preferreadonlyproperties andReadonlyArray<T>where appropriate. Do not mutate function parameters
PreferT[]overArray<T>for readability
PreferRecord<Key, T>for simple maps; useMap/Setwhen iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Usesatisfiesto enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Useimport type { X } from '…'for type-only imports. Keep value and type imports separated when helpful for clarity and bundling
Exported functions, classes, and public APIs must have explicit parameter and return types. Internal locals can rely on inference
Prefer arrow functions for local functions and callbacks. Use function declarations when hoisting orthisbinding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over rawthen/catchchains. Keep linear flow and usetry/catchfor failures
Eitherawaitpromises or deliberately mark them...
Files:
tests/integration/install/installApp.spec.ts
tests/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Files:
tests/integration/install/installApp.spec.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Consider running `yarn test:e2e` for UI changes before committing.
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Add screenshots for visual regression testing in integration tests
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Add screenshots for visual regression testing in integration tests
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Use fixture classes (TestApp, TestInstallWizard, TestInstalledApp, TestServerStart, TestTroubleshooting, TestGraphCanvas) instead of raw locators
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:28.076Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Applies to tests/integration/**/*.{ts,tsx} : Test changes with E2E tests (Playwright) for UI changes.
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/**/*.spec.ts : Use convenience methods and properties defined in helper classes instead of direct locator selectors
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Import fixtures from testExtensions.ts, not raw Playwright
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Mark slow tests with test.slow() in integration tests
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/**/*.spec.ts : Prefer imports from `testExtensions.ts` over Playwright defaults in spec files
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/**/*.spec.ts : Prefer adding properties to test classes over using locator selectors directly in tests
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Trust auto-cleanup via Symbol.asyncDispose rather than manual cleanup in test fixtures
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Applied to files:
tests/integration/install/installApp.spec.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Organize integration tests using Playwright test projects: install, post-install-setup, post-install, and post-install-teardown
Applied to files:
tests/integration/install/installApp.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: lint-and-format (macos-latest)
- GitHub Check: lint-and-format (windows-latest)
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
🔇 Additional comments (1)
tests/integration/install/installApp.spec.ts (1)
35-37: Verify if both visibility checks are necessary.Both
firstTimeTemplateWorkflowText(line 35) andtemplatesGrid(line 37) are checked for visibility with the same 30-second timeout. If these elements always appear together as part of the same first-run template dialog, one check might suffice.Please confirm whether both assertions serve distinct purposes (e.g., the grid could fail to populate even when the text is visible) or if one check is redundant.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
triple ai review |
Summary
Testing
┆Issue is synchronized with this Notion page by Unito