From 3387b5bb6b0edd4741fec0040ce55a85537a7322 Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Wed, 6 May 2026 09:43:28 +0300 Subject: [PATCH] chore(tests): guard more test lookups - Remove non-null assertions from context menu, panels, perf, properties, MCP, SVG, undo, and instance override tests - Add explicit store, renderer, node, and bounding-box guards - Validate affected engine and E2E tests --- tests/e2e/context-menu/basic.spec.ts | 29 +++++++++---- tests/e2e/panels/basic.spec.ts | 23 +++++++--- tests/e2e/perf/basic.spec.ts | 24 +++++++---- tests/e2e/properties/panel.spec.ts | 20 ++++----- tests/engine/editor/undo/create-shape.test.ts | 22 +++++----- tests/engine/mcp.test.ts | 29 +++++++------ .../scene-graph/instance-overrides.test.ts | 43 +++++++++++++++---- tests/engine/svg/import.test.ts | 31 +++++++------ 8 files changed, 142 insertions(+), 79 deletions(-) diff --git a/tests/e2e/context-menu/basic.spec.ts b/tests/e2e/context-menu/basic.spec.ts index c83a3b93b..30dc1af27 100644 --- a/tests/e2e/context-menu/basic.spec.ts +++ b/tests/e2e/context-menu/basic.spec.ts @@ -1,5 +1,6 @@ import { expect, test, type Page } from '@playwright/test' +import { expectDefined } from '#tests/helpers/assert' import { CanvasHelper } from '#tests/helpers/canvas' let page: Page @@ -33,12 +34,16 @@ function getPageChildren() { } function getSelectedCount() { - return page.evaluate(() => window.__OPEN_PENCIL_STORE__!.state.selectedIds.size) + return page.evaluate(() => { + const store = window.__OPEN_PENCIL_STORE__ + if (!store) throw new Error('OpenPencil store not initialized') + return store.state.selectedIds.size + }) } async function rightClickShape(x: number, y: number) { - const box = await canvas.canvas.boundingBox() - await page.mouse.click(box!.x + x, box!.y + y, { button: 'right' }) + const box = expectDefined(await canvas.canvas.boundingBox(), 'canvas bounds') + await page.mouse.click(box.x + x, box.y + y, { button: 'right' }) } function contextMenu() { @@ -104,17 +109,23 @@ test('toggle visibility via context menu', async () => { await canvas.click(250, 230) await canvas.waitForRender() - const nodeId = await page.evaluate(() => [...window.__OPEN_PENCIL_STORE__!.state.selectedIds][0]) + const nodeId = await page.evaluate(() => { + const store = window.__OPEN_PENCIL_STORE__ + if (!store) throw new Error('OpenPencil store not initialized') + return [...store.state.selectedIds][0] + }) await rightClickShape(250, 230) await contextItem('context-toggle-visibility').click() await canvas.waitForRender() const hidden = await page.evaluate((id) => { - const n = window.__OPEN_PENCIL_STORE__!.graph.getNode(id) + const store = window.__OPEN_PENCIL_STORE__ + if (!store) throw new Error('OpenPencil store not initialized') + const n = store.graph.getNode(id) return n ? { visible: n.visible } : null }, nodeId) - expect(hidden!.visible).toBe(false) + expect(hidden?.visible).toBe(false) // Toggle back: select via store since invisible nodes can't be hit-tested await page.evaluate(() => { @@ -125,10 +136,12 @@ test('toggle visibility via context menu', async () => { await canvas.waitForRender() const restored = await page.evaluate((id) => { - const n = window.__OPEN_PENCIL_STORE__!.graph.getNode(id) + const store = window.__OPEN_PENCIL_STORE__ + if (!store) throw new Error('OpenPencil store not initialized') + const n = store.graph.getNode(id) return n ? { visible: n.visible } : null }, nodeId) - expect(restored!.visible).toBe(true) + expect(restored?.visible).toBe(true) }) test('toggle lock via context menu', async () => { diff --git a/tests/e2e/panels/basic.spec.ts b/tests/e2e/panels/basic.spec.ts index aa01b36a3..f6deb7443 100644 --- a/tests/e2e/panels/basic.spec.ts +++ b/tests/e2e/panels/basic.spec.ts @@ -1,5 +1,6 @@ import { test, expect, type Page } from '@playwright/test' +import { expectDefined } from '#tests/helpers/assert' import { CanvasHelper } from '#tests/helpers/canvas' let page: Page @@ -27,8 +28,10 @@ test('layers panel resize increases width', async () => { const handleBox = await handle.boundingBox() expect(handleBox).not.toBeNull() - const cx = handleBox!.x + handleBox!.width / 2 - const cy = handleBox!.y + handleBox!.height / 2 + const handleBounds = expectDefined(handleBox, 'splitter handle bounds') + const beforeBounds = expectDefined(before, 'layers panel bounds') + const cx = handleBounds.x + handleBounds.width / 2 + const cy = handleBounds.y + handleBounds.height / 2 await page.mouse.move(cx, cy) await page.mouse.down() @@ -36,22 +39,28 @@ test('layers panel resize increases width', async () => { await page.mouse.up() await canvas.waitForRender() - const after = await panel.boundingBox() - expect(after!.width).toBeGreaterThan(before!.width + 40) + const after = expectDefined(await panel.boundingBox(), 'resized layers panel bounds') + expect(after.width).toBeGreaterThan(beforeBounds.width + 40) canvas.assertNoErrors() }) test('panel width persists after page reload', async () => { // Allow Reka's auto-save debounce to flush before recording the width await page.waitForTimeout(300) - const recordedWidth = (await page.locator('[data-test-id="layers-panel"]').boundingBox())!.width + const recordedWidth = expectDefined( + await page.locator('[data-test-id="layers-panel"]').boundingBox(), + 'persisted layers panel bounds' + ).width await page.reload() canvas = new CanvasHelper(page) await canvas.waitForInit() - const after = await page.locator('[data-test-id="layers-panel"]').boundingBox() - expect(Math.abs(after!.width - recordedWidth)).toBeLessThanOrEqual(2) + const after = expectDefined( + await page.locator('[data-test-id="layers-panel"]').boundingBox(), + 'reloaded layers panel bounds' + ) + expect(Math.abs(after.width - recordedWidth)).toBeLessThanOrEqual(2) canvas.assertNoErrors() }) diff --git a/tests/e2e/perf/basic.spec.ts b/tests/e2e/perf/basic.spec.ts index c13f0a866..a5b901423 100644 --- a/tests/e2e/perf/basic.spec.ts +++ b/tests/e2e/perf/basic.spec.ts @@ -37,9 +37,9 @@ test.describe('Render performance', () => { { type: 'SOLID', color: { - r: arr[i * 3]! / 255, - g: arr[i * 3 + 1]! / 255, - b: arr[i * 3 + 2]! / 255, + r: (arr[i * 3] ?? 0) / 255, + g: (arr[i * 3 + 1] ?? 0) / 255, + b: (arr[i * 3 + 2] ?? 0) / 255, a: 1 }, visible: true, @@ -89,7 +89,8 @@ test.describe('Render performance', () => { const results = await helper.page.evaluate((iterations: number) => { const store = window.__OPEN_PENCIL_STORE__ if (!store) throw new Error('OpenPencil store not initialized') - const renderer = store.renderer! + const renderer = store.renderer + if (!renderer) throw new Error('OpenPencil renderer not initialized') function setupRenderer() { renderer.dpr = window.devicePixelRatio || 1 @@ -119,11 +120,15 @@ test.describe('Render performance', () => { const panMs = performance.now() - panStart // Benchmark 2: Scene change (cache miss every frame) - const nodes = [...store.graph.getNode(store.state.currentPageId)!.childIds] + const pageNode = store.graph.getNode(store.state.currentPageId) + if (!pageNode) throw new Error(`Page ${store.state.currentPageId} not found`) + const nodes = [...pageNode.childIds] setupRenderer() const sceneStart = performance.now() for (let i = 0; i < iterations; i++) { - const node = store.graph.getNode(nodes[i % nodes.length]!) + const nodeId = nodes[i % nodes.length] + if (!nodeId) throw new Error('Benchmark node id not found') + const node = store.graph.getNode(nodeId) if (node) store.graph.updateNode(node.id, { x: node.x + 0.1 }) store.state.sceneVersion++ renderer.render(store.graph, store.state.selectedIds, {}, store.state.sceneVersion) @@ -177,7 +182,8 @@ test.describe('Render performance', () => { ({ count, iterations }) => { const store = window.__OPEN_PENCIL_STORE__ if (!store) throw new Error('OpenPencil store not initialized') - const renderer = store.renderer! + const renderer = store.renderer + if (!renderer) throw new Error('OpenPencil renderer not initialized') const graph = store.graph const pageId = store.state.currentPageId @@ -225,7 +231,9 @@ test.describe('Render performance', () => { setupRenderer() const sceneStart = performance.now() for (let i = 0; i < iterations; i++) { - const node = graph.getNode(shadowIds[i % shadowIds.length]!) + const nodeId = shadowIds[i % shadowIds.length] + if (!nodeId) throw new Error('Shadow benchmark node id not found') + const node = graph.getNode(nodeId) if (node) graph.updateNode(node.id, { x: node.x + 0.1 }) store.state.sceneVersion++ renderer.render(graph, store.state.selectedIds, {}, store.state.sceneVersion) diff --git a/tests/e2e/properties/panel.spec.ts b/tests/e2e/properties/panel.spec.ts index 154bb64b7..daa43773b 100644 --- a/tests/e2e/properties/panel.spec.ts +++ b/tests/e2e/properties/panel.spec.ts @@ -1,5 +1,6 @@ import { expect, test, type Page } from '@playwright/test' +import { expectDefined } from '#tests/helpers/assert' import { CanvasHelper } from '#tests/helpers/canvas' import { getPageChildren, getSelectedNode } from '#tests/helpers/store' @@ -23,8 +24,7 @@ test('ScrubInput drag changes X position', async () => { await canvas.clearCanvas() await canvas.drawRect(100, 100, 80, 80) const before = await getSelectedNode(page) - expect(before).not.toBeNull() - const initialX = before!.x + const initialX = expectDefined(before, 'selected rectangle before drag').x const xScrub = page .locator('[data-test-id="position-section"] [data-test-id="scrub-input"]') @@ -32,7 +32,7 @@ test('ScrubInput drag changes X position', async () => { await canvas.dragScrubInput(xScrub, 50) const after = await getSelectedNode(page) - expect(after!.x).not.toBe(initialX) + expect(after?.x).not.toBe(initialX) canvas.assertNoErrors() }) @@ -51,7 +51,7 @@ test('corner radius uniform sets cornerRadius', async () => { await canvas.waitForRender() const node = await getSelectedNode(page) - expect(node!.cornerRadius).toBe(12) + expect(node?.cornerRadius).toBe(12) canvas.assertNoErrors() }) @@ -86,8 +86,8 @@ test('fill gradient switch changes fill type', async () => { await page.locator('[data-test-id="fill-picker-tab-gradient"]').click() await canvas.waitForRender() - const node = await getSelectedNode(page) - expect(node!.fills[0].type).toBe('GRADIENT_LINEAR') + const node = expectDefined(await getSelectedNode(page), 'gradient-filled node') + expect(node.fills[0]?.type).toBe('GRADIENT_LINEAR') canvas.assertNoErrors() }) @@ -246,7 +246,7 @@ test('flip horizontal sets flipX', async () => { await canvas.waitForRender() const node = await getSelectedNode(page) - expect(node!.flipX).toBe(true) + expect(node?.flipX).toBe(true) canvas.assertNoErrors() }) @@ -260,13 +260,13 @@ test('clip content checkbox toggles clipsContent', async () => { await canvas.pressKey('Shift+a') await canvas.waitForRender() - const before = await getSelectedNode(page) - const initialValue = before!.clipsContent + const before = expectDefined(await getSelectedNode(page), 'selected frame before clipping') + const initialValue = before.clipsContent await page.locator('[data-test-id="clip-content-checkbox"]').click() await canvas.waitForRender() const after = await getSelectedNode(page) - expect(after!.clipsContent).toBe(!initialValue) + expect(after?.clipsContent).toBe(!initialValue) canvas.assertNoErrors() }) diff --git a/tests/engine/editor/undo/create-shape.test.ts b/tests/engine/editor/undo/create-shape.test.ts index 481b708ca..9d3e266d7 100644 --- a/tests/engine/editor/undo/create-shape.test.ts +++ b/tests/engine/editor/undo/create-shape.test.ts @@ -2,6 +2,8 @@ import { describe, test, expect } from 'bun:test' import { createEditor } from '@open-pencil/core/editor' +import { getNodeOrThrow } from '#tests/helpers/assert' + describe('create shape undo/redo', () => { test('batched create+resize undoes in one step', () => { const editor = createEditor() @@ -13,8 +15,8 @@ describe('create shape undo/redo', () => { editor.commitResize(id, { x: 100, y: 100, width: 0, height: 0 }) editor.undo.commitBatch() - expect(editor.graph.getNode(id)!.width).toBe(200) - expect(editor.graph.getNode(id)!.height).toBe(150) + expect(getNodeOrThrow(editor.graph, id).width).toBe(200) + expect(getNodeOrThrow(editor.graph, id).height).toBe(150) // Single undo removes the shape entirely editor.undo.undo() @@ -22,11 +24,11 @@ describe('create shape undo/redo', () => { // Single redo restores with full dimensions editor.undo.redo() - expect(editor.graph.getNode(id)).not.toBeUndefined() - expect(editor.graph.getNode(id)!.width).toBe(200) - expect(editor.graph.getNode(id)!.height).toBe(150) - expect(editor.graph.getNode(id)!.x).toBe(100) - expect(editor.graph.getNode(id)!.y).toBe(100) + const restored = getNodeOrThrow(editor.graph, id) + expect(restored.width).toBe(200) + expect(restored.height).toBe(150) + expect(restored.x).toBe(100) + expect(restored.y).toBe(100) }) test('redo after create+move+duplicate restores correct state', () => { @@ -46,7 +48,7 @@ describe('create shape undo/redo', () => { // Undo move editor.undo.undo() - expect(editor.graph.getNode(id)!.x).toBe(50) + expect(getNodeOrThrow(editor.graph, id).x).toBe(50) // Undo create (single step) editor.undo.undo() @@ -54,10 +56,10 @@ describe('create shape undo/redo', () => { // Redo create (single step, full dimensions) editor.undo.redo() - expect(editor.graph.getNode(id)!.width).toBe(120) + expect(getNodeOrThrow(editor.graph, id).width).toBe(120) // Redo move editor.undo.redo() - expect(editor.graph.getNode(id)!.x).toBe(200) + expect(getNodeOrThrow(editor.graph, id).x).toBe(200) }) }) diff --git a/tests/engine/mcp.test.ts b/tests/engine/mcp.test.ts index e5b847270..9cb5b3adb 100644 --- a/tests/engine/mcp.test.ts +++ b/tests/engine/mcp.test.ts @@ -2,6 +2,8 @@ import { describe, expect, test } from 'bun:test' import { ALL_TOOLS, FigmaAPI, SceneGraph, computeAllLayouts, parseFigFile } from '@open-pencil/core' +import { expectDefined } from '#tests/helpers/assert' + describe('MCP tool execution', () => { function setup() { const graph = new SceneGraph() @@ -59,12 +61,11 @@ describe('MCP tool execution', () => { expect(result.type).toBe('FRAME') expect(result.name).toBe('TestFrame') - const node = api.getNodeById(result.id) - expect(node).not.toBeNull() - expect(node!.x).toBe(10) - expect(node!.y).toBe(20) - expect(node!.width).toBe(300) - expect(node!.height).toBe(200) + const node = expectDefined(api.getNodeById(result.id), 'created frame') + expect(node.x).toBe(10) + expect(node.y).toBe(20) + expect(node.width).toBe(300) + expect(node.height).toBe(200) }) test('set_fill applies color', () => { @@ -78,7 +79,7 @@ describe('MCP tool execution', () => { }) as { id: string } findTool('set_fill').execute(api, { id: frame.id, color: '#ff0000' }) - const node = api.getNodeById(frame.id)! + const node = expectDefined(api.getNodeById(frame.id), 'filled rectangle') expect(node.fills).toHaveLength(1) expect(node.fills[0].color.r).toBeCloseTo(1) expect(node.fills[0].color.g).toBeCloseTo(0) @@ -102,7 +103,7 @@ describe('MCP tool execution', () => { padding: 24 }) - const node = api.getNodeById(frame.id)! + const node = expectDefined(api.getNodeById(frame.id), 'layout frame') expect(node.layoutMode).toBe('VERTICAL') expect(node.itemSpacing).toBe(16) expect(node.paddingTop).toBe(24) @@ -133,9 +134,11 @@ describe('MCP tool execution', () => { const tree = findTool('get_page_tree').execute(api, {}) as { children: { id: string; children?: unknown[] }[] } - const parentNode = tree.children.find((c: { id: string }) => c.id === parent.id) - expect(parentNode).toBeDefined() - expect(parentNode!.children).toBeArray() + const parentNode = expectDefined( + tree.children.find((c: { id: string }) => c.id === parent.id), + 'parent tree node' + ) + expect(parentNode.children).toBeArray() }) test('delete_node removes node', () => { @@ -195,7 +198,7 @@ describe('MCP tool execution', () => { const clone = findTool('clone_node').execute(api, { id: frame.id }) as { id: string } expect(clone.id).not.toBe(frame.id) - const clonedNode = api.getNodeById(clone.id)! + const clonedNode = expectDefined(api.getNodeById(clone.id), 'cloned node') expect(clonedNode.width).toBe(100) expect(clonedNode.height).toBe(50) }) @@ -224,7 +227,7 @@ describe('MCP tool execution', () => { expect(result.updated).toContain('cornerRadius') expect(result.updated).toContain('name') - const node = api.getNodeById(frame.id)! + const node = expectDefined(api.getNodeById(frame.id), 'updated node') expect(node.x).toBe(50) expect(node.opacity).toBe(0.5) expect(node.cornerRadius).toBe(8) diff --git a/tests/engine/scene-graph/instance-overrides.test.ts b/tests/engine/scene-graph/instance-overrides.test.ts index 0ded9bff0..b2f9dd899 100644 --- a/tests/engine/scene-graph/instance-overrides.test.ts +++ b/tests/engine/scene-graph/instance-overrides.test.ts @@ -2,6 +2,8 @@ import { describe, test, expect } from 'bun:test' import { importNodeChanges, type NodeChange } from '@open-pencil/core' +import { expectDefined } from '#tests/helpers/assert' + const ID = { m00: 1, m01: 0, m02: 0, m10: 0, m11: 1, m12: 0 } const SIZE = { x: 100, y: 100 } @@ -85,7 +87,10 @@ function swapOverrideFixture(opts?: { customName?: string }): NodeChange[] { describe('instance swap overrides', () => { test('swap override renames icon and reclones children', () => { const graph = importNodeChanges(swapOverrideFixture()) - const page = graph.getPages().find((p) => p.name === 'Page1')! + const page = expectDefined( + graph.getPages().find((p) => p.name === 'Page1'), + 'Page1' + ) const button = graph.getChildren(page.id)[0] expect(button.name).toBe('ButtonInstance') @@ -100,7 +105,10 @@ describe('instance swap overrides', () => { test('swap override preserves user-given name', () => { const graph = importNodeChanges(swapOverrideFixture({ customName: 'MyCustomIcon' })) - const page = graph.getPages().find((p) => p.name === 'Page1')! + const page = expectDefined( + graph.getPages().find((p) => p.name === 'Page1'), + 'Page1' + ) const button = graph.getChildren(page.id)[0] const icon = graph.getChildren(button.id)[0] @@ -138,11 +146,16 @@ describe('instance swap overrides', () => { ] const graph = importNodeChanges(nodes) - const page = graph.getPages().find((p) => p.name === 'Page1')! + const page = expectDefined( + graph.getPages().find((p) => p.name === 'Page1'), + 'Page1' + ) const children = graph.getChildren(page.id) - const wrapper = children.find((c) => c.name === 'WrapperInstance')! - expect(wrapper).toBeDefined() + const wrapper = expectDefined( + children.find((c) => c.name === 'WrapperInstance'), + 'wrapper instance' + ) // WrapperInstance > ButtonInstance > icon const wrapperButton = graph.getChildren(wrapper.id)[0] @@ -168,14 +181,23 @@ describe('instance swap overrides', () => { ] const graph = importNodeChanges(nodes) - const page = graph.getPages().find((p) => p.name === 'Page1')! + const page = expectDefined( + graph.getPages().find((p) => p.name === 'Page1'), + 'Page1' + ) const children = graph.getChildren(page.id) - const swapped = children.find((c) => c.name === 'ButtonInstance')! + const swapped = expectDefined( + children.find((c) => c.name === 'ButtonInstance'), + 'swapped button instance' + ) const swappedIcon = graph.getChildren(swapped.id)[0] expect(swappedIcon.name).toBe('IconB') - const defaultBtn = children.find((c) => c.name === 'ButtonDefault')! + const defaultBtn = expectDefined( + children.find((c) => c.name === 'ButtonDefault'), + 'default button instance' + ) const defaultIcon = graph.getChildren(defaultBtn.id)[0] expect(defaultIcon.name).toBe('IconA') }) @@ -226,7 +248,10 @@ describe('instance swap overrides', () => { ] const graph = importNodeChanges(nodes) - const page = graph.getPages().find((p) => p.name === 'Page1')! + const page = expectDefined( + graph.getPages().find((p) => p.name === 'Page1'), + 'Page1' + ) const container = graph.getChildren(page.id)[0] // ContainerInstance > ButtonSwapped > icon diff --git a/tests/engine/svg/import.test.ts b/tests/engine/svg/import.test.ts index 69496eafc..434348378 100644 --- a/tests/engine/svg/import.test.ts +++ b/tests/engine/svg/import.test.ts @@ -3,6 +3,8 @@ import { describe, test, expect, beforeEach } from 'bun:test' import { FigmaAPI, SceneGraph } from '@open-pencil/core' import { importSvg } from '@open-pencil/core/tools' +import { expectDefined, getNodeOrThrow } from '#tests/helpers/assert' + let graph: SceneGraph let figma: FigmaAPI @@ -20,16 +22,17 @@ describe('import_svg', () => { expect(result.id).toBeDefined() expect(result.type).toBe('FRAME') - const frame = graph.getNode(result.id) - expect(frame).not.toBeUndefined() - expect(frame!.width).toBe(24) - expect(frame!.height).toBe(24) + const frame = getNodeOrThrow(graph, result.id) + expect(frame.width).toBe(24) + expect(frame.height).toBe(24) const children = graph.getChildren(result.id) expect(children.length).toBe(1) expect(children[0].type).toBe('VECTOR') expect(children[0].vectorNetwork).toBeDefined() - expect(children[0].vectorNetwork!.vertices.length).toBeGreaterThan(0) + expect( + expectDefined(children[0].vectorNetwork, 'imported vector network').vertices.length + ).toBeGreaterThan(0) }) test('imports multiple shapes', async () => { @@ -51,9 +54,9 @@ describe('import_svg', () => { svg: '' })) as { id: string } - const frame = graph.getNode(result.id) - expect(frame!.width).toBe(200) - expect(frame!.height).toBe(100) + const frame = getNodeOrThrow(graph, result.id) + expect(frame.width).toBe(200) + expect(frame.height).toBe(100) }) test('uses width/height attributes when no viewBox', async () => { @@ -61,9 +64,9 @@ describe('import_svg', () => { svg: '' })) as { id: string } - const frame = graph.getNode(result.id) - expect(frame!.width).toBe(48) - expect(frame!.height).toBe(48) + const frame = getNodeOrThrow(graph, result.id) + expect(frame.width).toBe(48) + expect(frame.height).toBe(48) }) test('sets custom name', async () => { @@ -116,9 +119,9 @@ describe('import_svg', () => { y: 200 })) as { id: string } - const frame = graph.getNode(result.id) - expect(frame!.x).toBe(100) - expect(frame!.y).toBe(200) + const frame = getNodeOrThrow(graph, result.id) + expect(frame.x).toBe(100) + expect(frame.y).toBe(200) }) test('returns error for empty SVG', async () => {