From 6b3de7365503e16ff7c289cf3674e9243431aa1d Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Tue, 22 Jul 2025 11:53:50 -0700 Subject: [PATCH] [test] Clean up SubgraphMemory tests to remove invalid assertions (#1149) --- test/subgraph/SubgraphMemory.test.ts | 288 +-------------------------- 1 file changed, 3 insertions(+), 285 deletions(-) diff --git a/test/subgraph/SubgraphMemory.test.ts b/test/subgraph/SubgraphMemory.test.ts index 81d81bdbec..cebeb81675 100644 --- a/test/subgraph/SubgraphMemory.test.ts +++ b/test/subgraph/SubgraphMemory.test.ts @@ -3,7 +3,6 @@ import { describe, expect, it, vi } from "vitest" import { LGraph } from "@/litegraph" import { SubgraphNode } from "@/subgraph/SubgraphNode" -// Note: Avoiding createMemoryLeakTest as it relies on non-deterministic GC behavior import { subgraphTest } from "./fixtures/subgraphFixtures" import { createEventCapture, @@ -11,11 +10,6 @@ import { createTestSubgraphNode, } from "./fixtures/subgraphHelpers" -// Mock WeakRef to detect if objects can be garbage collected -declare global { - const gc: (() => void) | undefined -} - describe("SubgraphNode Memory Management", () => { describe("Event Listener Cleanup", () => { it("should register event listeners on construction", () => { @@ -40,7 +34,7 @@ describe("SubgraphNode Memory Management", () => { expect(eventTypes).toContain("renaming-output") }) - it("CRITICAL: should clean up input listeners on removal", () => { + it("should clean up input listeners on removal", () => { const subgraph = createTestSubgraph({ inputs: [{ name: "input1", type: "number" }], }) @@ -57,31 +51,6 @@ describe("SubgraphNode Memory Management", () => { expect(subgraphNode.inputs[0]._listenerController?.signal.aborted).toBe(true) }) - it("CRITICAL: main subgraph event listeners are NOT cleaned up (MEMORY LEAK)", () => { - const subgraph = createTestSubgraph() - - // Track listener registration - const removeEventSpy = vi.spyOn(subgraph.events, "removeEventListener") - const addEventSpy = vi.spyOn(subgraph.events, "addEventListener") - const initialAddCalls = addEventSpy.mock.calls.length - - const subgraphNode = createTestSubgraphNode(subgraph) - const addCallsAfterCreate = addEventSpy.mock.calls.length - - // Verify listeners were added - expect(addCallsAfterCreate).toBeGreaterThan(initialAddCalls) - - // Call onRemoved - subgraphNode.onRemoved() - - // CRITICAL BUG: Main subgraph event listeners are NOT removed - // onRemoved only cleans up input-specific listeners, not the main ones - expect(removeEventSpy).not.toHaveBeenCalled() - - // This proves the memory leak exists - console.warn("MEMORY LEAK CONFIRMED: SubgraphNode event listeners not cleaned up on removal") - }) - it("should not accumulate listeners during reconfiguration", () => { const subgraph = createTestSubgraph({ inputs: [{ name: "input1", type: "number" }], @@ -111,135 +80,6 @@ describe("SubgraphNode Memory Management", () => { const finalCalls = addEventSpy.mock.calls.length expect(finalCalls).toBe(initialCalls) // Main listeners not re-added }) - - it("should demonstrate memory leak with multiple instances (limited scope)", () => { - const subgraph = createTestSubgraph() - - // Track total listener count - const addEventSpy = vi.spyOn(subgraph.events, "addEventListener") - let totalListenersAdded = 0 - - // Create and "remove" multiple instances (limited to 3 for test performance) - const instances: SubgraphNode[] = [] - - for (let i = 0; i < 3; i++) { - const initialCalls = addEventSpy.mock.calls.length - const instance = createTestSubgraphNode(subgraph, { id: i }) - instances.push(instance) - - const callsAfterCreate = addEventSpy.mock.calls.length - totalListenersAdded += (callsAfterCreate - initialCalls) - - // Simulate removal (but listeners won't be cleaned up) - instance.onRemoved() - } - - // All listeners are still registered even though nodes are "removed" - expect(totalListenersAdded).toBeGreaterThan(0) - - // Document the leak without excessive accumulation - console.warn(`Memory leak demonstrated: ${totalListenersAdded} listeners accumulated from ${instances.length} instances`) - - // IMPORTANT: This test intentionally creates a small memory leak to demonstrate the bug. - // In production, this would accumulate over time and cause performance issues. - // The leak is limited to 3 instances to minimize test suite impact. - }) - }) - - describe("Memory Leak Prevention Tests", () => { - it("verifies proper cleanup workflow exists", () => { - // This test documents the expected cleanup workflow without relying on GC - const subgraph = createTestSubgraph() - const subgraphNode = createTestSubgraphNode(subgraph) - - // Track cleanup state - const cleanupState = { - inputListenersCleanedUp: false, - mainListenersCleanedUp: false, - widgetReferencesCleared: false, - } - - // Check if input listeners exist and are set up - const input = subgraphNode.inputs[0] - - // Call cleanup - subgraphNode.onRemoved() - - // Verify what gets cleaned up - if (input && "_listenerController" in input && (input as any)._listenerController) { - cleanupState.inputListenersCleanedUp = (input as any)._listenerController.signal.aborted === true - expect(cleanupState.inputListenersCleanedUp).toBe(true) // This works when input exists - } else { - // If no input or no listener controller, that's also valid - cleanupState.inputListenersCleanedUp = true - expect(cleanupState.inputListenersCleanedUp).toBe(true) - } - - // TODO: These should be true after proper implementation - // expect(cleanupState.mainListenersCleanedUp).toBe(true) - // expect(cleanupState.widgetReferencesCleared).toBe(true) - - // This test serves as documentation for what needs to be implemented - }) - - it("should clean up widget references properly", () => { - const subgraph = createTestSubgraph({ - inputs: [{ name: "input1", type: "number" }], - }) - const subgraphNode = createTestSubgraphNode(subgraph) - - // Simulate widget promotion - const input = subgraphNode.inputs[0] - input._widget = { - type: "number", - name: "test_widget", - value: 42, - draw: vi.fn(), - mouse: vi.fn(), - computeSize: vi.fn(), - createCopyForNode: vi.fn(), - } - - input.widget = { name: "test_widget" } - - expect(input._widget).toBeDefined() - expect(input.widget).toBeDefined() - - // Removal should clean up widget references - subgraphNode.onRemoved() - - // Input-specific listeners are cleaned up, but widget refs may remain - // This tests the current behavior - expect(input._listenerController?.signal.aborted).toBe(true) - }) - - it("should handle rapid creation/destruction cycles", () => { - const subgraph = createTestSubgraph() - - // Simulate rapid creation/destruction that might happen in UI - const instances: SubgraphNode[] = [] - - for (let cycle = 0; cycle < 10; cycle++) { - // Create instance - const instance = createTestSubgraphNode(subgraph, { id: cycle }) - instances.push(instance) - - // Add to graph - const parentGraph = new LGraph() - parentGraph.add(instance) - - // Remove from graph - parentGraph.remove(instance) - instance.onRemoved() - } - - // All instances have been "removed" but event listeners remain - // This demonstrates the accumulation problem - expect(instances).toHaveLength(10) - - // Each instance still holds references through event listeners - // In a real app, this would cause memory growth over time - }) }) describe("Widget Promotion Memory Management", () => { @@ -279,9 +119,6 @@ describe("SubgraphNode Memory Management", () => { // Widget should be removed from array expect(subgraphNode.widgets).not.toContain(mockWidget) - - // References should be cleaned up (testing current implementation) - // Note: The PR #1107 fix should clean these up }) it("should not leak widgets during reconfiguration", () => { @@ -312,115 +149,6 @@ describe("SubgraphNode Memory Management", () => { expect(subgraphNode.widgets.length).toBe(initialWidgetCount) }) }) - - describe("Memory Leak Documentation", () => { - it("documents the known memory leak pattern", () => { - // This test documents the exact memory leak pattern for future fixes - - const subgraph = createTestSubgraph() - const eventCapture = createEventCapture(subgraph.events, [ - "input-added", - "output-added", - "removing-input", - "removing-output", - "renaming-input", - "renaming-output", - ]) - - // Create SubgraphNode - this registers 6 event listeners - const subgraphNode = createTestSubgraphNode(subgraph) - - // The memory leak occurs here: - // 1. SubgraphNode constructor (lines 52-92) adds event listeners - // 2. These create strong references: subgraph.events -> SubgraphNode - // 3. onRemoved() method (lines 302-306) only cleans input listeners - // 4. Main subgraph event listeners are NEVER removed - - expect(subgraphNode).toBeDefined() - - // Cleanup test resources - eventCapture.cleanup() - - console.log("Memory leak pattern documented: SubgraphNode event listeners persist after removal") - }) - - it("provides solution blueprint for memory leak fix", () => { - // This test shows what the fix should look like - - const subgraph = createTestSubgraph() - - // SOLUTION: SubgraphNode should use AbortController pattern - // - // In constructor: - // this.eventAbortController = new AbortController() - // const signal = this.eventAbortController.signal - // subgraphEvents.addEventListener("input-added", handler, { signal }) - // - // In onRemoved(): - // this.eventAbortController.abort() // Removes ALL listeners - - const subgraphNode = createTestSubgraphNode(subgraph) - - // Current implementation only cleans input listeners - subgraphNode.onRemoved() - - // TODO: Implement AbortController pattern to fix memory leak - expect(true).toBe(true) // Placeholder - documents intended solution - }) - }) -}) - -describe("Performance Impact of Memory Leak", () => { - it("measures event handler overhead with multiple instances", () => { - const subgraph = createTestSubgraph() - - // Create multiple instances (reduced from 50 to 20 for test efficiency) - const instances: SubgraphNode[] = [] - const startTime = performance.now() - - for (let i = 0; i < 20; i++) { - instances.push(createTestSubgraphNode(subgraph, { id: i })) - } - - const creationTime = performance.now() - startTime - - // Trigger an event that will call ALL accumulated listeners - const eventStartTime = performance.now() - subgraph.addInput("performance_test", "number") - const eventTime = performance.now() - eventStartTime - - console.log(`Created ${instances.length} instances in ${creationTime.toFixed(2)}ms`) - console.log(`Event dispatch took ${eventTime.toFixed(2)}ms (${instances.length} listeners)`) - - // More instances = more event listeners = slower event handling - // This demonstrates the performance impact of the memory leak - expect(eventTime).toBeGreaterThan(0) - }) - - it("demonstrates listener accumulation impact", () => { - // Test with different numbers of instances (reduced scale for efficiency) - const testCases = [5, 10, 15] - - for (const instanceCount of testCases) { - // Clean test - create fresh subgraph - const testSubgraph = createTestSubgraph() - - // Create instances - for (let i = 0; i < instanceCount; i++) { - createTestSubgraphNode(testSubgraph, { id: i }) - } - - // Measure event dispatch time - const startTime = performance.now() - testSubgraph.addInput(`test_${instanceCount}`, "number") - const dispatchTime = performance.now() - startTime - - console.log(`${instanceCount} instances: ${dispatchTime.toFixed(3)}ms event dispatch`) - } - - // This test documents that more instances = more listeners = slower events - expect(true).toBe(true) - }) }) describe("SubgraphMemory - Event Listener Management", () => { @@ -522,9 +250,6 @@ describe("SubgraphMemory - Reference Management", () => { const subgraph = createTestSubgraph({ nodeCount: 2 }) const subgraphNode = createTestSubgraphNode(subgraph) - // Before adding to graph, node might already have a graph reference - // (This depends on how createTestSubgraphNode works) - // Add to graph rootGraph.add(subgraphNode) expect(subgraphNode.graph).toBe(rootGraph) @@ -533,9 +258,6 @@ describe("SubgraphMemory - Reference Management", () => { // Remove from graph rootGraph.remove(subgraphNode) expect(rootGraph.nodes).not.toContain(subgraphNode) - - // After removal, graph reference behavior may vary by implementation - // The important thing is that it's removed from the graph's nodes array }) it("prevents circular reference creation", () => { @@ -546,7 +268,6 @@ describe("SubgraphMemory - Reference Management", () => { expect(subgraph.nodes).not.toContain(subgraphNode) // If circular references were attempted, they should be detected - // (This documents the expected behavior - implementation may vary) expect(subgraphNode.subgraph).toBe(subgraph) expect(subgraph.nodes.includes(subgraphNode)).toBe(false) }) @@ -630,9 +351,6 @@ describe("SubgraphMemory - Widget Reference Management", () => { if (input && "_listenerController" in input) { expect((input as any)._listenerController?.signal.aborted).toBe(true) } - - // Note: Other references may still exist - this documents current behavior - // In a proper implementation, onRemoved should clean these up too }) }) @@ -667,7 +385,7 @@ describe("SubgraphMemory - Performance and Scale", () => { const rootGraph = new LGraph() const instances = [] - // Create instances (reduced from 50 to 25 for test efficiency) + // Create instances for (let i = 0; i < 25; i++) { const instance = createTestSubgraphNode(subgraph) rootGraph.add(instance) @@ -709,4 +427,4 @@ describe("SubgraphMemory - Performance and Scale", () => { expect(rootGraph.nodes.length).toBe(0) } }) -}) +}) \ No newline at end of file