test: stabilize cloud template filtering e2e #12999
+20
−2
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,24 @@ export class TemplateHelper { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| async mockIndex(): Promise<void> { | ||||||||||||||||
| const customTemplatesHandler = async (route: Route) => { | ||||||||||||||||
| const customTemplates: Record<string, string[]> = {} | ||||||||||||||||
| await route.fulfill({ | ||||||||||||||||
| status: 200, | ||||||||||||||||
| body: JSON.stringify(customTemplates), | ||||||||||||||||
|
Comment on lines
+73
to
+76
Contributor
There was a problem hiding this comment. The typed local
Suggested change
Collaborator
Author
There was a problem hiding this comment. Agreed, this is a valid nit. The inline @coderabbitai please create an issue for this and assign it to me.
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo "Comfy-Org/ComfyUI_frontend" \
--title "Simplify TemplateHelper.mockIndex(): inline '{}' instead of JSON.stringify(customTemplates)" \
--assignee "jaeone94" \
--body "## Summary
In \`browser_tests/fixtures/helpers/TemplateHelper.ts\`, the \`mockIndex()\` method constructs an unnecessary intermediate variable before serializing it:
\`\`\`ts
const customTemplates: Record<string, string[]> = {}
await route.fulfill({
status: 200,
body: JSON.stringify(customTemplates),
...
})
\`\`\`
Since the response is always a fixed empty object, the typed local variable and \`JSON.stringify\` call are indirection that can be replaced with an inlined string literal:
\`\`\`ts
await route.fulfill({
status: 200,
body: '{}',
...
})
\`\`\`
## Context
This was flagged as a nit during review of PR `#12999` (comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12999#discussion_r3443589801). It was deferred from that PR to avoid a non-functional review round on an already-approved stabilization PR.
Requested by `@jaeone94`."Length of output: 1202
🐇 ( 🧠 Learnings used |
||||||||||||||||
| headers: { | ||||||||||||||||
| 'Content-Type': 'application/json', | ||||||||||||||||
| 'Cache-Control': 'no-store' | ||||||||||||||||
| } | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| const customTemplatesPattern = '**/api/workflow_templates' | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Convention nit (non-blocking): sibling fixtures anchor with regex (
Collaborator
Author
There was a problem hiding this comment. Agreed. The current glob is safe for this endpoint, but the mixed route pattern convention is a valid cleanup concern. Since that is more of a fixture consistency/refactor pass than part of the E2E stabilization itself, I think it is better handled in a follow-up PR. @coderabbitai please create an issue for this and assign it to me.
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo "Comfy-Org/ComfyUI_frontend" \
--assignee "jaeone94" \
--title "Refactor: Standardize Playwright route pattern convention in E2E fixture helpers" \
--body "## Summary
Fixture helpers under \`browser_tests/fixtures/helpers/\` currently use a mixed convention for Playwright route patterns:
- Some fixtures (e.g. the assets and prompt handlers) anchor routes with regex patterns such as \`/\/api\/assets(?:\?.*)?$/\` and \`/\/api\/prompt$/\`.
- The \`TemplateHelper.mockIndex()\` handler added in `#12999` uses a glob pattern (\`**/api/workflow_templates\`) consistent with the \`README\`-documented \`**/api/object_info\` style.
Both approaches work correctly, but the inconsistency makes the fixture layer harder to maintain and reason about. A follow-up refactor pass should align all route patterns to a single convention (either regex or glob) across the fixture helpers.
## References
- PR: `#12999`
- Comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12999#discussion_r3443589817
- Requested by: `@jaeone94`
"Length of output: 1191
(ᵔᴥᵔ)~🐇 🧠 Learnings used
Contributor
There was a problem hiding this comment. Also, if this is a constant, could you pull it out to module scope and name it something like
Collaborator
Author
There was a problem hiding this comment. Agreed. Pulling the workflow templates route pattern to module scope would be a valid cleanup, and it likely makes the most sense together with normalizing the other TemplateHelper route patterns rather than changing only this new one. I kept that refactor out of this stabilization PR for scope control. @coderabbitai please create an issue for this and assign it to me.
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo "Comfy-Org/ComfyUI_frontend" \
--title "Refactor: Extract TemplateHelper route patterns to named module-scope constants" \
--assignee "jaeone94" \
--body "## Summary
The route pattern strings inside \`TemplateHelper\` in \`browser_tests/fixtures/helpers/TemplateHelper.ts\` are currently defined as inline \`const\` variables inside method bodies. These should be lifted to module-scope named constants (e.g., \`ROUTE_PATTERN_WORKFLOW_TEMPLATES\`, \`ROUTE_PATTERN_TEMPLATE_INDEX\`, \`ROUTE_PATTERN_TEMPLATE_THUMBNAILS\`) to:
- Improve readability and discoverability
- Enable reuse across tests without duplication
- Make intent explicit through descriptive names
## Scope
Normalize **all three** existing route patterns in \`TemplateHelper\` at the same time:
- \`'**/api/workflow_templates'\`
- \`'**/templates/index.json'\`
- \`'**/templates/**.webp'\`
## Background
Identified during review of PR `#12999` (comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12999#discussion_r3443639562). The refactor was intentionally kept out of that stabilization PR for scope control.
Requested by: `@jaeone94`"Length of output: 1283
It covers normalizing all three existing route patterns in 🧠 Learnings used |
||||||||||||||||
| this.routeHandlers.push({ | ||||||||||||||||
| pattern: customTemplatesPattern, | ||||||||||||||||
| handler: customTemplatesHandler | ||||||||||||||||
| }) | ||||||||||||||||
| await this.page.route(customTemplatesPattern, customTemplatesHandler) | ||||||||||||||||
|
Comment on lines
+84
to
+88
Contributor
There was a problem hiding this comment. All three handlers ( private async registerRoute(
pattern: string,
handler: (route: Route) => Promise<void>
): Promise<void> {
this.routeHandlers.push({ pattern, handler })
await this.page.route(pattern, handler)
}Then each call site collapses to
Collaborator
Author
There was a problem hiding this comment. Agreed, this helper extraction would make the route registration safer and clearer. I did not want to broaden this PR from a targeted E2E stabilization into a TemplateHelper refactor, especially since the immediate goal was to stop cloud-only runtime requests from leaking into exact-count assertions. This is a good follow-up cleanup. @coderabbitai please create an issue for this and assign it to me.
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo Comfy-Org/ComfyUI_frontend \
--title "Refactor: Extract registerRoute private helper in TemplateHelper" \
--assignee jaeone94 \
--body "## Summary
In \`browser_tests/fixtures/helpers/TemplateHelper.ts\`, the pattern of pushing to \`routeHandlers\` and calling \`page.route()\` is repeated across all three route registrations (\`customTemplates\`, \`index\`, \`thumbnails\`). Extracting a small private helper would DRY this up and prevent the easy mistake of routing without registering for cleanup.
## Proposed change
\`\`\`ts
private async registerRoute(
pattern: string,
handler: (route: Route) => Promise<void>
): Promise<void> {
this.routeHandlers.push({ pattern, handler })
await this.page.route(pattern, handler)
}
\`\`\`
Each call site (\`mockIndex\`, \`mockThumbnails\`) then collapses to:
\`\`\`ts
await this.registerRoute(pattern, handler)
\`\`\`
## Context
Identified during review of PR `#12999` (comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12999#discussion_r3443589821).
The current PR was intentionally scoped to E2E test stabilization; this cleanup is deferred as a follow-up.
## Related
- PR: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12999
- File: \`browser_tests/fixtures/helpers/TemplateHelper.ts\`
"Length of output: 1429
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🧠 Learnings used |
||||||||||||||||
|
|
||||||||||||||||
| const indexHandler = async (route: Route) => { | ||||||||||||||||
| const payload = this.index ?? mockTemplateIndex(this.templates) | ||||||||||||||||
| await route.fulfill({ | ||||||||||||||||
|
|
||||||||||||||||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Route interception is correct and necessary here — confirmed it matches
<api_base>/api/workflow_templatesand that the empty{}keeps the asserted template counts deterministic.Nit (optional): this custom-templates mock lives inside
mockIndex(), which otherwise only concerns the core index. It works because every test callsmockIndex(), but a dedicatedmockCustomTemplates()invoked frommock()would read more truthfully.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is a valid readability cleanup. I kept the custom templates mock colocated with the existing template index mocking in this PR because the change was intentionally scoped to stabilizing the cloud template count E2E, and I wanted to avoid widening it into fixture refactoring while we continue the missing model detection work before .46 becomes stable.
This would be better handled as follow-up cleanup rather than mixed into this stabilization PR.
@coderabbitai please create an issue for this and assign it to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 1310
@jaeone94, the follow-up issue has been created and assigned to you:#13016— Refactor TemplateHelper: extractmockCustomTemplates()frommockIndex(). It includes the full context from this discussion so nothing gets lost.🧠 Learnings used