test(e2e): align test default menu to Top; make legacy specs explicit #5746
Conversation
dosubot
bot
added
the
size:L
🎭 Playwright Test Results⏰ Completed at: 09/25/2025, 01:10:56 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
the only failing test is just flaky 
|
benceruleanlu
added
the
New Browser Test Expectations
| try { | ||
| await comfyPage.setupSettings({ | ||
| 'Comfy.UseNewMenu': 'Disabled', | ||
| 'Comfy.UseNewMenu': 'Top', |
There was a problem hiding this comment.
question (non-blocking): would it work to just remove this line altogether since this is the default value?
There was a problem hiding this comment.
yeah I think so, good point, will make this change after merge since we have the setting to dismiss approvals after commits
| **Most tests require the new menu system** - Add to your test: | ||
|
|
||
| ```typescript | ||
| test.beforeEach(async ({ comfyPage }) => { | ||
| await comfyPage.setSetting('Comfy.UseNewMenu', 'Top') | ||
| }) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
question (non-blocking): should we just change this to say like "Some tests require the old menu system"? Or maybe not because that should never be necessary anymore?
There was a problem hiding this comment.
I think it would be clear enough, since it would say that explicitly by setting it to Disabled, so I don't think this would be needed
Summary
UseNewMenu has been defaulted to Top in the app for over a year; Playwright’s test default lagged behind. This PR aligns the test default with reality and keeps legacy specs stable.
Changes
Review Focus
Screenshots (if applicable)
N/A
┆Issue is synchronized with this Notion page by Unito