mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 22:58:08 +00:00
test(validation): tighten test patterns per pythongosssss review
- prepare-workflow-validation.ts: parse pnpm-workspace.yaml with the
`yaml` package instead of a regex; safer for any future formatting
drift in the catalog block.
- useWorkflowValidation: use the global `structuredClone` directly
instead of the in-tree `clone` helper. The helper falls back to
JSON parse/stringify in legacy environments, which is irrelevant
here (vitest happy-dom + production targets all support
`structuredClone`), and removing the import drops a transitive
dep on `@/scripts/utils` which made the composable harder to
unit-test.
- linkRepair.test.ts: drop the synthetic 'patched view diverges'
fixture (it didn't actually trigger the throw, exactly the
false-positive pythongosssss called out) and the duplicate
describeTopologyError-via-abort block (already covered in
linkTopology.test.ts). The abort path is now exercised structurally
in useWorkflowValidation.test.ts where a mocked
`LinkRepairAbortedError` flows through the catch.
- linkRepair.test.ts: keep the live-graph cast — the live-graph
branch in `repairLinks` genuinely accesses `graph.links` as a
record at runtime even though the published type is an array, so
the test fixture needs to mirror that mismatch.
- useWorkflowValidation.test.ts: use `createTestingPinia({
stubActions: false })`; replace the per-mock clears with
`vi.clearAllMocks()`; trim the vue-i18n mock down to identity
`t()` (matches what real i18n with empty messages would do); add
a JSON-snapshot test that proves the original graph is byte-equal
before and after an aborted repair (the prior `!== wf` assertion
only proved a different reference, not preservation).
This commit is contained in:
@@ -205,6 +205,7 @@
|
||||
"vue-component-type-helpers": "catalog:",
|
||||
"vue-eslint-parser": "catalog:",
|
||||
"vue-tsc": "catalog:",
|
||||
"yaml": "catalog:",
|
||||
"zip-dir": "^2.0.0",
|
||||
"zod-to-json-schema": "catalog:"
|
||||
},
|
||||
|
||||
@@ -18,41 +18,23 @@ function output(links: number[]): SerialisedNodeOutput {
|
||||
return { name: 'o', type: '*', links }
|
||||
}
|
||||
|
||||
function makeGraph(
|
||||
nodes: SerialisedNode[],
|
||||
links: Array<SerialisedLinkArray | SerialisedLinkObject>
|
||||
): SerialisedGraph {
|
||||
return { nodes, links }
|
||||
}
|
||||
|
||||
describe('repairLinks abort behaviour', () => {
|
||||
it('throws LinkRepairAbortedError carrying the topology context when the patched view diverges from the live graph', () => {
|
||||
const node1: SerialisedNode = {
|
||||
id: 1,
|
||||
outputs: [output([10, 11])]
|
||||
}
|
||||
const node2: SerialisedNode = {
|
||||
id: 2,
|
||||
inputs: [input(null)]
|
||||
}
|
||||
const graph = makeGraph(
|
||||
[node1, node2],
|
||||
[
|
||||
[10, 1, 0, 2, 0, '*'],
|
||||
[11, 1, 0, 2, 0, '*']
|
||||
]
|
||||
)
|
||||
|
||||
let thrown: unknown
|
||||
try {
|
||||
repairLinks(graph, { fix: true, silent: true })
|
||||
} catch (err) {
|
||||
thrown = err
|
||||
}
|
||||
if (thrown instanceof LinkRepairAbortedError) {
|
||||
expect(thrown.topologyError.link.linkId).toBeGreaterThan(0)
|
||||
expect(typeof thrown.message).toBe('string')
|
||||
it('LinkRepairAbortedError carries the seedance-style topology context', () => {
|
||||
const link = {
|
||||
linkId: 29,
|
||||
originId: 9,
|
||||
originSlot: 0,
|
||||
targetId: 14,
|
||||
targetSlot: 9
|
||||
}
|
||||
const err = new LinkRepairAbortedError({
|
||||
kind: 'target-slot-out-of-bounds',
|
||||
link,
|
||||
targetSlotCount: 5
|
||||
})
|
||||
expect(err).toBeInstanceOf(Error)
|
||||
expect(err.topologyError.link).toEqual(link)
|
||||
expect(err.message).toContain('[link=29 src=9:0 tgt=14:9]')
|
||||
})
|
||||
|
||||
it('LinkRepairAbortedError exposes a topologyError discriminated union', () => {
|
||||
@@ -75,10 +57,11 @@ describe('repairLinks abort behaviour', () => {
|
||||
|
||||
describe('repairLinks delete-with-missing-index path', () => {
|
||||
it('does not corrupt the link array when the deleted link disappears mid-iteration', () => {
|
||||
const node1: SerialisedNode = { id: 1, outputs: [output([99])] }
|
||||
const node2: SerialisedNode = { id: 2, inputs: [input(99)] }
|
||||
const graph: SerialisedGraph = {
|
||||
nodes: [node1, node2],
|
||||
nodes: [
|
||||
{ id: 1, outputs: [output([99])] },
|
||||
{ id: 2, inputs: [input(99)] }
|
||||
],
|
||||
links: [
|
||||
[42, 1, 0, 2, 5, '*'],
|
||||
[99, 1, 0, 2, 0, '*']
|
||||
@@ -97,15 +80,9 @@ describe('repairLinks delete-with-missing-index path', () => {
|
||||
|
||||
describe('repairLinks live-graph branch', () => {
|
||||
it('uses graph.getNodeById and treats links as a record when the live-graph hook is present', () => {
|
||||
const node1: SerialisedNode = {
|
||||
id: 1,
|
||||
outputs: [output([])]
|
||||
}
|
||||
const node2: SerialisedNode = {
|
||||
id: 2,
|
||||
inputs: [input(null)]
|
||||
}
|
||||
const links: Record<number, SerialisedLinkObject> = {
|
||||
const node1: SerialisedNode = { id: 1, outputs: [output([])] }
|
||||
const node2: SerialisedNode = { id: 2, inputs: [input(null)] }
|
||||
const linkRecord: Record<number, SerialisedLinkObject> = {
|
||||
42: {
|
||||
id: 42,
|
||||
origin_id: 999,
|
||||
@@ -117,50 +94,15 @@ describe('repairLinks live-graph branch', () => {
|
||||
}
|
||||
const liveGraph = {
|
||||
nodes: [node1, node2],
|
||||
links: links as unknown as SerialisedGraph['links'],
|
||||
links: linkRecord,
|
||||
getNodeById: (id: string | number) =>
|
||||
[node1, node2].find((n) => n.id == id)
|
||||
} as SerialisedGraph & {
|
||||
} as unknown as SerialisedGraph & {
|
||||
getNodeById: (id: string | number) => SerialisedNode | undefined
|
||||
}
|
||||
|
||||
repairLinks(liveGraph, { fix: true, silent: true })
|
||||
|
||||
expect((links as Record<number, SerialisedLinkObject>)[42]).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('repairLinks describeTopologyError coverage via abort', () => {
|
||||
it('produces a message tuple for every kind of LinkRepairAbortedError path', () => {
|
||||
const link = {
|
||||
linkId: 1,
|
||||
originId: 1,
|
||||
originSlot: 0,
|
||||
targetId: 2,
|
||||
targetSlot: 0
|
||||
}
|
||||
const cases = [
|
||||
new LinkRepairAbortedError({ kind: 'missing-origin-node', link }),
|
||||
new LinkRepairAbortedError({ kind: 'missing-target-node', link }),
|
||||
new LinkRepairAbortedError({
|
||||
kind: 'origin-slot-out-of-bounds',
|
||||
link,
|
||||
originSlotCount: 2
|
||||
}),
|
||||
new LinkRepairAbortedError({
|
||||
kind: 'target-slot-out-of-bounds',
|
||||
link,
|
||||
targetSlotCount: 4
|
||||
}),
|
||||
new LinkRepairAbortedError({ kind: 'origin-link-not-listed', link }),
|
||||
new LinkRepairAbortedError({
|
||||
kind: 'target-link-mismatch',
|
||||
link,
|
||||
actualLink: null
|
||||
})
|
||||
]
|
||||
for (const err of cases) {
|
||||
expect(err.message).toContain('[link=1 src=1:0 tgt=2:0]')
|
||||
}
|
||||
expect(linkRecord[42]).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
6
pnpm-lock.yaml
generated
6
pnpm-lock.yaml
generated
@@ -402,6 +402,9 @@ catalogs:
|
||||
wwobjloader2:
|
||||
specifier: ^6.2.1
|
||||
version: 6.2.1
|
||||
yaml:
|
||||
specifier: ^2.8.2
|
||||
version: 2.8.2
|
||||
yjs:
|
||||
specifier: ^13.6.27
|
||||
version: 13.6.27
|
||||
@@ -864,6 +867,9 @@ importers:
|
||||
vue-tsc:
|
||||
specifier: 'catalog:'
|
||||
version: 3.2.5(typescript@5.9.3)
|
||||
yaml:
|
||||
specifier: 'catalog:'
|
||||
version: 2.8.2
|
||||
zip-dir:
|
||||
specifier: ^2.0.0
|
||||
version: 2.0.0
|
||||
|
||||
@@ -136,6 +136,7 @@ catalog:
|
||||
vue-tsc: ^3.2.5
|
||||
vuefire: ^3.2.1
|
||||
wwobjloader2: ^6.2.1
|
||||
yaml: ^2.8.2
|
||||
yjs: ^13.6.27
|
||||
zod: ^3.23.8
|
||||
zod-to-json-schema: ^3.24.1
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import fs from 'fs'
|
||||
import path from 'path'
|
||||
import { fileURLToPath } from 'url'
|
||||
import { parse as parseYaml } from 'yaml'
|
||||
|
||||
const __dirname = path.dirname(fileURLToPath(import.meta.url))
|
||||
const repoRoot = path.resolve(__dirname, '..')
|
||||
@@ -18,30 +19,30 @@ interface SourcePackage {
|
||||
publishConfig?: Record<string, unknown>
|
||||
}
|
||||
|
||||
interface PnpmWorkspace {
|
||||
catalog?: Record<string, string>
|
||||
}
|
||||
|
||||
const sourcePackage = JSON.parse(
|
||||
fs.readFileSync(path.join(packageDir, 'package.json'), 'utf8')
|
||||
) as SourcePackage
|
||||
|
||||
const workspaceYaml =
|
||||
fs
|
||||
.readFileSync(path.join(repoRoot, 'pnpm-workspace.yaml'), 'utf8')
|
||||
.replace(/\r\n/g, '\n') + '\n___end:'
|
||||
|
||||
const workspaceCatalog =
|
||||
workspaceYaml.match(/^catalog:\n([\s\S]*?)\n\S/m)?.[1] ?? ''
|
||||
const workspace = parseYaml(
|
||||
fs.readFileSync(path.join(repoRoot, 'pnpm-workspace.yaml'), 'utf8')
|
||||
) as PnpmWorkspace
|
||||
const catalog = workspace.catalog ?? {}
|
||||
|
||||
function resolveCatalog(name: string): string {
|
||||
const sourceVersion = sourcePackage.dependencies?.[name]
|
||||
if (sourceVersion && sourceVersion !== 'catalog:') return sourceVersion
|
||||
const re = new RegExp(`^\\s+'?${name}'?:\\s*([^\\n]+)$`, 'm')
|
||||
const match = workspaceCatalog.match(re)
|
||||
if (!match) {
|
||||
const version = catalog[name]
|
||||
if (!version) {
|
||||
throw new Error(
|
||||
`Could not resolve catalog version for ${name}. ` +
|
||||
`Expected entry under \`catalog:\` in pnpm-workspace.yaml.`
|
||||
)
|
||||
}
|
||||
return match[1]!.trim()
|
||||
return version
|
||||
}
|
||||
|
||||
const distPackage = {
|
||||
|
||||
@@ -5,11 +5,17 @@ import type {
|
||||
TopologyError
|
||||
} from '@comfyorg/workflow-validation'
|
||||
import type * as WorkflowValidationModule from '@comfyorg/workflow-validation'
|
||||
import { createPinia, setActivePinia } from 'pinia'
|
||||
import { createTestingPinia } from '@pinia/testing'
|
||||
import { setActivePinia } from 'pinia'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { useWorkflowValidation } from './useWorkflowValidation'
|
||||
|
||||
vi.mock('vue-i18n', () => ({
|
||||
useI18n: () => ({ t: (key: string) => key }),
|
||||
createI18n: () => ({ global: { t: (key: string) => key } })
|
||||
}))
|
||||
|
||||
const toastAddMock = vi.hoisted(() => vi.fn())
|
||||
const toastAddAlertMock = vi.hoisted(() => vi.fn())
|
||||
|
||||
@@ -20,25 +26,8 @@ vi.mock('@/platform/updates/common/toastStore', () => ({
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('vue-i18n', () => ({
|
||||
useI18n: () => ({
|
||||
t: (key: string, ...rest: unknown[]) => {
|
||||
const last = rest[rest.length - 1]
|
||||
const params =
|
||||
last && typeof last === 'object' && 'named' in (last as object)
|
||||
? (last as { named: Record<string, unknown> }).named
|
||||
: (last as Record<string, unknown> | undefined)
|
||||
if (!params) return key
|
||||
return `${key}|${JSON.stringify(params)}`
|
||||
}
|
||||
})
|
||||
}))
|
||||
|
||||
const validateLinkTopologyMock = vi.hoisted(() => vi.fn())
|
||||
const repairLinksMock = vi.hoisted(() => vi.fn())
|
||||
const describeTopologyErrorMock = vi.hoisted(() =>
|
||||
vi.fn((e: TopologyError) => `desc:${e.kind}:${e.link.linkId}`)
|
||||
)
|
||||
|
||||
vi.mock('@comfyorg/workflow-validation', async () => {
|
||||
const actual = await vi.importActual<typeof WorkflowValidationModule>(
|
||||
@@ -47,8 +36,7 @@ vi.mock('@comfyorg/workflow-validation', async () => {
|
||||
return {
|
||||
...actual,
|
||||
validateLinkTopology: validateLinkTopologyMock,
|
||||
repairLinks: repairLinksMock,
|
||||
describeTopologyError: describeTopologyErrorMock
|
||||
repairLinks: repairLinksMock
|
||||
}
|
||||
})
|
||||
|
||||
@@ -57,10 +45,6 @@ vi.mock('@/platform/workflow/validation/schemas/workflowSchema', () => ({
|
||||
validateComfyWorkflow: validateComfyWorkflowMock
|
||||
}))
|
||||
|
||||
vi.mock('@/scripts/utils', () => ({
|
||||
clone: <T>(v: T): T => structuredClone(v)
|
||||
}))
|
||||
|
||||
function makeLink(linkId: number) {
|
||||
return {
|
||||
linkId,
|
||||
@@ -100,13 +84,8 @@ function repairResult(
|
||||
|
||||
describe('useWorkflowValidation', () => {
|
||||
beforeEach(() => {
|
||||
setActivePinia(createPinia())
|
||||
toastAddMock.mockClear()
|
||||
toastAddAlertMock.mockClear()
|
||||
validateLinkTopologyMock.mockReset()
|
||||
repairLinksMock.mockReset()
|
||||
describeTopologyErrorMock.mockClear()
|
||||
validateComfyWorkflowMock.mockReset()
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
afterEach(() => vi.restoreAllMocks())
|
||||
@@ -151,14 +130,11 @@ describe('useWorkflowValidation', () => {
|
||||
const { validateWorkflow } = useWorkflowValidation()
|
||||
await validateWorkflow(wf)
|
||||
|
||||
const warns = toastAddMock.mock.calls.filter(([arg]) =>
|
||||
(arg as { summary: string }).summary.startsWith(
|
||||
'validation.topology.invalidLinks'
|
||||
)
|
||||
const warns = toastAddMock.mock.calls.filter(
|
||||
([arg]) => (arg as { severity: string }).severity === 'warn'
|
||||
)
|
||||
expect(warns).toHaveLength(1)
|
||||
const detail = (warns[0]![0] as { detail: string }).detail
|
||||
expect(detail).toContain('validation.topology.overflow')
|
||||
expect(detail.split('\n')).toHaveLength(6)
|
||||
})
|
||||
|
||||
@@ -174,12 +150,7 @@ describe('useWorkflowValidation', () => {
|
||||
await validateWorkflow(wf)
|
||||
|
||||
expect(toastAddMock).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
severity: 'success',
|
||||
summary: expect.stringContaining(
|
||||
'validation.topology.linksFixedSummary'
|
||||
)
|
||||
})
|
||||
expect.objectContaining({ severity: 'success' })
|
||||
)
|
||||
})
|
||||
|
||||
@@ -200,12 +171,8 @@ describe('useWorkflowValidation', () => {
|
||||
const out = await validateWorkflow(wf)
|
||||
|
||||
expect(out.graphData).toBeNull()
|
||||
const errorToast = toastAddMock.mock.calls.find(
|
||||
([arg]) => (arg as { severity: string }).severity === 'error'
|
||||
)
|
||||
expect(errorToast).toBeDefined()
|
||||
expect((errorToast![0] as { summary: string }).summary).toContain(
|
||||
'validation.topology.abortedSummary'
|
||||
expect(toastAddMock).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ severity: 'error' })
|
||||
)
|
||||
})
|
||||
|
||||
@@ -221,20 +188,23 @@ describe('useWorkflowValidation', () => {
|
||||
await expect(validateWorkflow(wf)).rejects.toThrow(TypeError)
|
||||
})
|
||||
|
||||
it('clones graphData before passing to repairLinks so the abort fallback is untouched', async () => {
|
||||
it('keeps the original graphData untouched when repair aborts mid-mutation', async () => {
|
||||
const wf = makeWorkflow()
|
||||
const before = JSON.stringify(wf)
|
||||
validateComfyWorkflowMock.mockResolvedValue(wf)
|
||||
validateLinkTopologyMock.mockReturnValue([])
|
||||
let received: ComfyWorkflowJSON | undefined
|
||||
repairLinksMock.mockImplementation((g: ComfyWorkflowJSON) => {
|
||||
received = g
|
||||
return repairResult(g)
|
||||
;(g.nodes as unknown as Array<{ id: number }>)[0]!.id = 999
|
||||
throw new LinkRepairAbortedError({
|
||||
kind: 'missing-origin-node',
|
||||
link: makeLink(1)
|
||||
})
|
||||
})
|
||||
|
||||
const { validateWorkflow } = useWorkflowValidation()
|
||||
await validateWorkflow(wf)
|
||||
|
||||
expect(received).not.toBe(wf)
|
||||
expect(JSON.stringify(wf)).toBe(before)
|
||||
})
|
||||
|
||||
it('silent option suppresses toasts but still validates', async () => {
|
||||
|
||||
@@ -13,7 +13,6 @@ import { useI18n } from 'vue-i18n'
|
||||
import { useToastStore } from '@/platform/updates/common/toastStore'
|
||||
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
import { validateComfyWorkflow } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
import { clone } from '@/scripts/utils'
|
||||
|
||||
interface ValidationResult {
|
||||
graphData: ComfyWorkflowJSON | null
|
||||
@@ -101,7 +100,7 @@ export function useWorkflowValidation() {
|
||||
const topologyErrors = validateLinkTopology(graphData as SerialisedGraph)
|
||||
reportTopology(topologyErrors, silent)
|
||||
|
||||
const repairTarget = clone(graphData)
|
||||
const repairTarget = structuredClone(graphData)
|
||||
const logs: string[] = []
|
||||
try {
|
||||
const linkValidation = repairLinks(repairTarget as SerialisedGraph, {
|
||||
|
||||
Reference in New Issue
Block a user