Skip to content

test: harden false-positive e2e assertions (#1229) #1303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggfevans merged 1 commit into from
Feb 20, 2026
Merged

test: harden false-positive e2e assertions (#1229) #1303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions e2e/keyboard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,14 @@ test.describe("Keyboard Shortcuts", () => {

test("Ctrl+S triggers save", async ({ page }) => {
// Set up download listener
const downloadPromise = page
.waitForEvent("download", { timeout: 5000 })
.catch(() => null);
const downloadPromise = page.waitForEvent("download", { timeout: 5000 });

// Press Ctrl+S
await page.keyboard.press("Control+s");

// Should trigger download
const download = await downloadPromise;
if (download) {
expect(download.suggestedFilename()).toContain(".Rackula.zip");
}
expect(download.suggestedFilename()).toMatch(/\.zip$/);
});

test("Escape closes dialogs", async ({ page }) => {
Expand All @@ -107,19 +103,27 @@ test.describe("Keyboard Shortcuts", () => {
});

test("Arrow keys move device in rack", async ({ page }) => {
// Add a device to the rack
await dragDeviceToRack(page);
// Add a device lower in the rack so ArrowUp can move it.
await dragDeviceToRack(page, { yOffsetPercent: 70 });

// Wait for device
await expect(page.locator(".rack-device").first()).toBeVisible();
const device = page.locator(".rack-front .rack-device").first();
await expect(device).toBeVisible();

// Select the device (first one in dual-view)
await page.locator(".rack-device").first().click();
await device.click();
const beforePosition = await device.boundingBox();
expect(beforePosition).not.toBeNull();
if (!beforePosition) {
throw new Error("Could not determine device position before ArrowUp");
}

// Press Arrow Up
await page.keyboard.press("ArrowUp");

// Note: This test verifies the key is handled, actual movement depends on implementation
await expect(page.locator(".rack-device").first()).toBeVisible();
await expect
.poll(async () => {
const afterPosition = await device.boundingBox();
return afterPosition?.y ?? beforePosition.y;
})
.not.toBe(beforePosition.y);
});
});
74 changes: 53 additions & 21 deletions e2e/rack-context-menu-focus.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ async function getPanzoomTransform(page: Page) {
});
}

async function setPanzoomTransform(page: Page, transform: string) {
await page.evaluate((nextTransform) => {
const panzoomContainer = document.querySelector(".panzoom-container");
if (panzoomContainer) {
(panzoomContainer as HTMLElement).style.transform = nextTransform;
}
}, transform);
}
Comment on lines +20 to +27
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find panzoom in package.json and check for any panzoom imports
rg "panzoom" package.json package-lock.json 2>/dev/null | head -20

Repository: RackulaLives/Rackula

Length of output: 290

🏁 Script executed:

#!/bin/bash
# Locate the test file and examine setPanzoomTransform and getPanzoomTransform
fd "rack-context-menu-focus.spec.ts" --type f

Repository: RackulaLives/Rackula

Length of output: 99

🏁 Script executed:

#!/bin/bash
# Search for panzoom usage across the codebase
rg -n "panzoom" src/ e2e/ --type ts --type tsx -A 2 | head -40

Repository: RackulaLives/Rackula

Length of output: 91

🏁 Script executed:

#!/bin/bash
# Read the test file to understand the structure and helpers
cat -n e2e/rack-context-menu-focus.spec.ts | head -150

Repository: RackulaLives/Rackula

Length of output: 5310

🏁 Script executed:

#!/bin/bash
# Search for getPanzoomTransform definition
rg -n "getPanzoomTransform|setPanzoomTransform" e2e/ --type ts -A 5

Repository: RackulaLives/Rackula

Length of output: 6337

🏁 Script executed:

#!/bin/bash
# Search for panzoom initialization and focus implementation
rg -n "import.*panzoom|from.*panzoom|new.*Panzoom|panzoom\(" src/ --type ts -A 3 | head -60

Repository: RackulaLives/Rackula

Length of output: 3767

🏁 Script executed:

#!/bin/bash
# Look for focusRack or focus-related functions
rg -n "focusRack|focus.*function|const.*focus" src/ --type ts -A 5 | head -80

Repository: RackulaLives/Rackula

Length of output: 4256

🏁 Script executed:

#!/bin/bash
# Read the focusRack function implementation
sed -n '289,350p' src/lib/stores/canvas.svelte.ts

Repository: RackulaLives/Rackula

Length of output: 2554

🏁 Script executed:

#!/antml:bash
# Also check how panzoom is initialized and if RAF is used
rg -n "new Panzoom|panzoom\(" src/lib --type ts -B 2 -A 5

Repository: RackulaLives/Rackula

Length of output: 46

🏁 Script executed:

#!/bin/bash
# Look at panzoom mock to understand expected API
cat -n src/tests/mocks/panzoom.ts | head -70

Repository: RackulaLives/Rackula

Length of output: 2161

🏁 Script executed:

#!/bin/bash
# Continue reading focusRack to see panzoom API calls
sed -n '289,400p' src/lib/stores/canvas.svelte.ts

Repository: RackulaLives/Rackula

Length of output: 4070

🏁 Script executed:

#!/bin/bash
# Search for smoothZoomAbs or moveTo calls in focusRack
rg -n "smoothZoomAbs|moveTo|zoomAbs" src/lib/stores/canvas.svelte.ts -A 2 -B 2

Repository: RackulaLives/Rackula

Length of output: 1851

🏁 Script executed:

#!/bin/bash
# Verify that panzoom internal state is separate from CSS
# Check if there's any initialization code that might set different defaults
rg -n "new Panzoom|panzoom(" src/lib --type ts -B 3 -A 10 | head -60

Repository: RackulaLives/Rackula

Length of output: 154

Move transformBefore capture immediately before the Focus click to avoid false positives from panzoom's RAF loop.

setPanzoomTransform writes only to the CSS style.transform; it does not update the panzoom library's internal state. The multiple async operations between capturing transformBefore (line 42) and clicking Focus (line 64)—dispatching the contextmenu event, waiting for .context-menu-content, finding and asserting focusItem—create await points where panzoom's RAF loop continuously synchronizes the CSS to panzoom's actual internal state. This can overwrite the manually set CSS before Focus fires.

If Focus is broken and doesn't update panzoom's internal state, the test will still pass because transformBefore (captured earlier) now holds a stale value that differs from the current CSS, making the poll succeed despite the failure.

Fix: Move `transformBefore` capture to immediately before the click action

In Test 1 (lines 40–64), move the capture block:

   // Force a non-focused transform so Focus must change it.
   await setPanzoomTransform(page, "matrix(0.4, 0, 0, 0.4, 0, 0)");
-  const transformBefore = await getPanzoomTransform(page);
-  expect(transformBefore).toBeTruthy();
-  expect(transformBefore?.scale).toBe(0.4);
   
   // Open the canvas context menu directly on the rack element.
   await page.evaluate(() => {
     const rack = document.querySelector(".rack-svg");
     if (!rack) throw new Error("Could not find rack svg");
     rack.dispatchEvent(new MouseEvent("contextmenu", { bubbles: true, cancelable: true }));
   });
   
   await expect(page.locator(".context-menu-content")).toBeVisible();
   
   const focusItem = page.locator('.context-menu-item:has-text("Focus")');
   await expect(focusItem).toBeVisible();
+  
+  // Capture the actual current transform immediately before clicking,
+  // ensuring it reflects the true CSS state at the moment Focus fires.
+  const transformBefore = await getPanzoomTransform(page);
+  expect(transformBefore).toBeTruthy();
+  expect(transformBefore?.scale).toBe(0.4);
   await focusItem.click();

Apply the same pattern to Test 2 (lines 84–110): move the capture block to immediately before line 110's focusItem.click().

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@e2e/rack-context-menu-focus.spec.ts` around lines 20 - 27, The test captures
transformBefore too early (it reads the CSS transform set by setPanzoomTransform
but panzoom's RAF loop may overwrite it before the Focus click), so move the
transformBefore capture to immediately before the focus click to ensure you're
comparing the current CSS at the moment of the click: in the spec's two tests
that use setPanzoomTransform, remove the earlier transformBefore reads and
instead call page.evaluate(() =>
document.querySelector(".panzoom-container")?.getAttribute("style") or read
.style.transform) right before invoking focusItem.click() (and similarly before
the second test's focusItem.click()), keeping the rest of the assertions intact.


test.describe("Rack Context Menu Focus", () => {
test.beforeEach(async ({ page }) => {
await gotoWithRack(page);
Expand All @@ -28,12 +37,23 @@ test.describe("Rack Context Menu Focus", () => {
// Rack should be visible
await expect(page.locator(".rack-container").first()).toBeVisible();

// Get the initial transform
// Force a non-focused transform so Focus must change it.
await setPanzoomTransform(page, "matrix(0.4, 0, 0, 0.4, 0, 0)");
const transformBefore = await getPanzoomTransform(page);
expect(transformBefore).toBeTruthy();
expect(transformBefore?.scale).toBe(0.4);

// Right-click on the rack-svg (inside the dual view)
await page.locator(".rack-svg").first().click({ button: "right" });
// Open the canvas context menu directly on the rack element.
await page.evaluate(() => {
const rack = document.querySelector(".rack-svg");
if (!rack) throw new Error("Could not find rack svg");
rack.dispatchEvent(
new MouseEvent("contextmenu", {
bubbles: true,
cancelable: true,
}),
);
Comment on lines +50 to +55
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Programmatic contextmenu event is missing mouse coordinates.

The event is dispatched without clientX, clientY, screenX, or screenY (all default to 0). If the context menu handler uses event coordinates for positioning or hit-testing (e.g., "was the right-click inside the rack?"), the menu may appear at the top-left corner or the handler may reject the event. Using Playwright's native click({ button: 'right' }) on .rack-svg would be more faithful and avoid this fragility.

🛠️ Proposed fix — use Playwright's built-in right-click 
-   // Open the canvas context menu directly on the rack element.
-   await page.evaluate(() => {
-     const rack = document.querySelector(".rack-svg");
-     if (!rack) throw new Error("Could not find rack svg");
-     rack.dispatchEvent(
-       new MouseEvent("contextmenu", {
-         bubbles: true,
-         cancelable: true,
-       }),
-     );
-   });
+   // Open the canvas context menu via a native right-click on the rack.
+   await page.locator(".rack-svg").first().click({ button: "right" });
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@e2e/rack-context-menu-focus.spec.ts` around lines 50 - 55, The test
dispatches a programmatic "contextmenu" MouseEvent without clientX/clientY which
can default to 0 and break handlers; replace the manual dispatch (the
rack.dispatchEvent(new MouseEvent("contextmenu", ...)) call) with Playwright's
native right-click on the element (e.g., await page.locator('.rack-svg').click({
button: 'right' }) or await page.click('.rack-svg', { button: 'right' })) so the
browser will generate proper coordinates and a faithful contextmenu event.

});

// Wait for context menu to appear
await expect(page.locator(".context-menu-content")).toBeVisible();
Expand All @@ -43,22 +63,30 @@ test.describe("Rack Context Menu Focus", () => {
await expect(focusItem).toBeVisible();
await focusItem.click();

// Focus recalculates and applies optimal zoom/pan for the rack.
// Even from the initial position, Focus will center the rack.
// Wait for transform to potentially change (animation may take time)
await expect(async () => {
const transformAfter = await getPanzoomTransform(page);
expect(transformAfter).toBeTruthy();
expect(transformAfter?.scale).toBeDefined();
expect(transformAfter?.x).toBeDefined();
expect(transformAfter?.y).toBeDefined();
}).toPass({ timeout: 1000 });
await expect
.poll(async () => {
const transformAfter = await getPanzoomTransform(page);
if (!transformBefore || !transformAfter) return false;

return (
transformAfter.scale !== transformBefore.scale ||
transformAfter.x !== transformBefore.x ||
transformAfter.y !== transformBefore.y
);
})
.toBe(true);
Comment on lines +66 to +77
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Duplicate polling logic — extract into a shared helper.

The expect.poll(...) block at lines 66-77 is copy-pasted verbatim at lines 112-123. Extract it into a named helper at the top of the file for DRY and maintainability.

♻️ Proposed refactor 
+async function assertTransformChanged(
+  page: Page,
+  transformBefore: { scale: number; x: number; y: number },
+) {
+  await expect
+    .poll(
+      async () => {
+        const transformAfter = await getPanzoomTransform(page);
+        if (!transformAfter) return false;
+        return (
+          transformAfter.scale !== transformBefore.scale ||
+          transformAfter.x !== transformBefore.x ||
+          transformAfter.y !== transformBefore.y
+        );
+      },
+      { timeout: 5000 },
+    )
+    .toBe(true);
+}

Then replace both polling blocks with:

-   await expect
-     .poll(async () => {
-       const transformAfter = await getPanzoomTransform(page);
-       if (!transformBefore || !transformAfter) return false;
-       return (
-         transformAfter.scale !== transformBefore.scale ||
-         transformAfter.x !== transformBefore.x ||
-         transformAfter.y !== transformBefore.y
-       );
-     })
-     .toBe(true);
+   await assertTransformChanged(page, transformBefore!);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await expect
.poll(async () => {
const transformAfter = await getPanzoomTransform(page);
if (!transformBefore || !transformAfter) return false;
return (
transformAfter.scale !== transformBefore.scale ||
transformAfter.x !== transformBefore.x ||
transformAfter.y !== transformBefore.y
);
})
.toBe(true);
async function assertTransformChanged(
page: Page,
transformBefore: { scale: number; x: number; y: number },
) {
await expect
.poll(
async () => {
const transformAfter = await getPanzoomTransform(page);
if (!transformAfter) return false;
return (
transformAfter.scale !== transformBefore.scale ||
transformAfter.x !== transformBefore.x ||
transformAfter.y !== transformBefore.y
);
},
{ timeout: 5000 },
)
.toBe(true);
}
// Then at lines 66-77:
await assertTransformChanged(page, transformBefore!);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@e2e/rack-context-menu-focus.spec.ts` around lines 66 - 77, Extract the
duplicated expect.poll logic around getPanzoomTransform into a reusable async
helper (e.g., waitForPanzoomTransformChange) declared at the top of the spec
file; the helper should accept the Playwright Page and the prior transform (or
compute transformBefore inside) and return a boolean or await until
transform.scale/x/y changes by using the same polling logic currently
duplicated, then replace both duplicated expect.poll(...) usages with calls to
waitForPanzoomTransformChange(page, transformBefore) (or
waitForPanzoomTransformChange(page) as implemented) to keep behavior identical
while removing duplication.

});

test("Focus option in Racks panel context menu works", async ({ page }) => {
// Rack should be visible
await expect(page.locator(".rack-container").first()).toBeVisible();

// Force a non-focused transform so Focus must change it.
await setPanzoomTransform(page, "matrix(0.4, 0, 0, 0.4, 0, 0)");
const transformBefore = await getPanzoomTransform(page);
expect(transformBefore).toBeTruthy();
expect(transformBefore?.scale).toBe(0.4);

// Switch to the Racks tab in the sidebar
// This test requires the sidebar to be visible (desktop viewport)
const racksTab = page.locator('button[role="tab"]:has-text("Racks")');
Expand All @@ -81,13 +109,17 @@ test.describe("Rack Context Menu Focus", () => {
// Click Focus - this triggers the focusRack function via callback chain
await focusItem.click();

// Verify the transform exists (Focus was applied)
await expect(async () => {
const transformAfter = await getPanzoomTransform(page);
expect(transformAfter).toBeTruthy();
expect(transformAfter?.scale).toBeDefined();
expect(transformAfter?.x).toBeDefined();
expect(transformAfter?.y).toBeDefined();
}).toPass({ timeout: 1000 });
await expect
.poll(async () => {
const transformAfter = await getPanzoomTransform(page);
if (!transformBefore || !transformAfter) return false;

return (
transformAfter.scale !== transformBefore.scale ||
transformAfter.x !== transformBefore.x ||
transformAfter.y !== transformBefore.y
);
})
.toBe(true);
});
});