[test] Clean up SubgraphMemory tests to remove invalid assertions

Removed tests that were not actually testing functionality:
- Placeholder tests with expect(true).toBe(true)
- Documentation-only tests trying to "prove" memory leaks without GC
- Tests with console.log/warn statements
- Tests that documented solutions rather than testing behavior

Kept valid tests that verify actual behavior:
- Event listener registration and cleanup
- Widget reference management
- Multiple instance handling
- Subgraph reference management

Fixes #1132

Author: christian-byrne

test/subgraph/SubgraphMemory.test.ts

@@ -3,19 +3,13 @@ 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,
   createTestSubgraph,
   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)
     }
   })
-})
+})