From 903e68e07367575c8a2120b018932c974446687c Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Tue, 18 Aug 2026 10:41:28 +0100 Subject: [PATCH] Make the e2e assertions capable of failing Four scenarios could not fail, which is worse than not having them: they report green regardless, so the suite's coverage looks larger than it is. Counts that matched zero: discover /\d+ hosts?/ matched "0 hosts" sweep /\d+ hosts? .* \d+ with open ports?/ interfaces /\d+ interfaces?/ All three now require [1-9]\d*. Each targets something guaranteed present: discover and sweep run against 127.0.0.0/30, which contains the loopback address the probe server listens on, and every machine has at least a loopback interface. A total backend regression in any of them used to go green. ARP deliberately still tolerates zero, and now says why: an empty neighbour cache is legitimate on a freshly booted runner, so requiring >= 1 would be flaky rather than strict. A hard failure is still caught, because the status bar would show an error rather than a count. The pcap scenario returned false and passed when the Packet Capture menu item was missing, so the whole capture surface disappearing was indistinguishable from a healthy skip. Now that builds without the feature keep the tool visible and show setup guidance (#193), an absent entry means something broke -- so it is a hard assertion. The unavailable branch also asserts the guidance names the feature and says how to get a capture-capable build, rather than only checking the element exists. Two silent skips: - `closeSettingsDialog` returned quietly when `.settings-close` was absent, so a dialog that could no longer be dismissed left the suite green and every later step ran against a still-open modal. Now an assertion. - `assertInterfaceReadinessReflectsSelection` early-returns on a host with no down interface. That is a real skip, but it now logs SKIP with the reason instead of reading like a pass. Verified by `node --check` and by reading. NOT executed: the render harness cannot pass on this host (WebView2 ignores the remote-debugging port on an elevated session, wry#1782), and gui-render.yml is schedule-only for the same reason. These changes are therefore unexercised until that is resolved -- which is worth stating plainly, since the whole point of the change is not trusting assertions that were never really running. --- .../tauri-render/scenarios/helpers/menu.mjs | 24 +++++++-- .../e2e/tauri-render/scenarios/tools.mjs | 50 +++++++++++++++---- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/apps/netscli-gui/e2e/tauri-render/scenarios/helpers/menu.mjs b/apps/netscli-gui/e2e/tauri-render/scenarios/helpers/menu.mjs index 3f11fe49..f9a0bb30 100644 --- a/apps/netscli-gui/e2e/tauri-render/scenarios/helpers/menu.mjs +++ b/apps/netscli-gui/e2e/tauri-render/scenarios/helpers/menu.mjs @@ -202,7 +202,16 @@ async function assertInterfaceReadinessReflectsSelection(driver) { return { found: true, originalName, originalUp, downName }; `); await closeSettingsDialog(driver); - if (!selection.found) return; + if (!selection.found) { + // A genuine skip, but say so. On a runner with a single healthy NIC + // there is no down interface to select, so this assertion has nothing to + // exercise -- it used to return silently, which reads in the log exactly + // like a pass. + console.log( + 'SKIP assertInterfaceReadinessReflectsSelection: no down interface on this host to select', + ); + return; + } await waitForText(driver, '[data-testid="statusbar"]', /Interface down/i); if (selection.originalName && selection.originalName !== selection.downName) { @@ -232,11 +241,16 @@ async function openSettingsDialog(driver) { } async function closeSettingsDialog(driver) { + // A missing close button is a failure, not a no-op. + // + // This used to return silently when `.settings-close` was absent, so a + // dialog that could no longer be dismissed left the suite green -- and + // every later step ran against a modal that was still open, producing + // confusing downstream failures instead of naming the real one. const closeButtons = await driver.findElements(By.css('.settings-close')); - if (closeButtons.length > 0) { - await closeButtons[0].click(); - await waitForNoElement(driver, '[data-testid="settings-dialog"]'); - } + assert.ok(closeButtons.length > 0, 'settings dialog has no .settings-close button to dismiss it'); + await closeButtons[0].click(); + await waitForNoElement(driver, '[data-testid="settings-dialog"]'); } async function openMenu(driver, menuLabel) { diff --git a/apps/netscli-gui/e2e/tauri-render/scenarios/tools.mjs b/apps/netscli-gui/e2e/tauri-render/scenarios/tools.mjs index 4bf8ec5e..26b54a2b 100644 --- a/apps/netscli-gui/e2e/tauri-render/scenarios/tools.mjs +++ b/apps/netscli-gui/e2e/tauri-render/scenarios/tools.mjs @@ -70,7 +70,11 @@ export async function exerciseDiscover(driver) { await assertCommand(driver, /netscli discover 127\.0\.0\.0\/30 --resolve --json/); await runActiveTool(driver); await assertOperationToastReturnsToTab(driver, /Discover/i); - await waitForText(driver, '[data-testid="statusbar"]', /\d+ hosts?/i, 25_000); + // At least one, not "0 hosts". 127.0.0.0/30 contains the loopback address + // the probe server runs on, so a working backend always finds it -- and + // /\d+ hosts?/ matched "0 hosts", meaning a total Discover regression went + // green. + await waitForText(driver, '[data-testid="statusbar"]', /[1-9]\d* hosts?/i, 25_000); await assertNoErrorStrip(driver); } @@ -81,7 +85,14 @@ export async function exerciseSweep(driver, port) { await replaceInput(driver, '[data-testid="sweep-ports-input"]', String(port)); await assertCommand(driver, new RegExp(`netscli sweep 127\\.0\\.0\\.0\\/30 -p ${port} --resolve --json`)); await runActiveTool(driver); - await waitForText(driver, '[data-testid="statusbar"]', /\d+ hosts? .* \d+ with open ports?/i, 30_000); + // Both counts must be non-zero: the sweep targets the probe server's own + // port on loopback, so a working backend finds a host and an open port. + await waitForText( + driver, + '[data-testid="statusbar"]', + /[1-9]\d* hosts? .* [1-9]\d* with open ports?/i, + 30_000, + ); await waitForText(driver, '[data-testid="result-table"]', /MAC/i); await waitForText(driver, '[data-testid="result-table"]', /RTT/i); await assertNoErrorStrip(driver); @@ -91,7 +102,9 @@ export async function exerciseInterfaces(driver) { await addToolTab(driver, 'Interfaces'); await withElement(driver, '[data-testid="run-active-tab"]'); await assertCommand(driver, /netscli interfaces --json/); - await waitForText(driver, '[data-testid="statusbar"]', /\d+ interfaces?/i, 20_000); + // Every machine has at least a loopback interface, so zero here is a + // backend failure rather than a legitimate empty result. + await waitForText(driver, '[data-testid="statusbar"]', /[1-9]\d* interfaces?/i, 20_000); await waitForRow(driver, '[data-testid^="result-row-iface-"]', 20_000); await waitForText(driver, '[data-testid="result-table"]', /App/i); await assertKeyboardSelection(driver); @@ -102,25 +115,44 @@ export async function exerciseArp(driver) { await addToolTab(driver, 'ARP Table'); await withElement(driver, '[data-testid="run-active-tab"]'); await assertCommand(driver, /netscli arp --json/); + // Deliberately still tolerates zero, unlike the assertions above. An empty + // neighbour cache is a legitimate state on a freshly booted CI runner, so + // requiring >= 1 here would be flaky rather than strict. A hard backend + // failure is still caught: the status bar would show an error instead of a + // count, and assertNoErrorStrip below would fire. await waitForText(driver, '[data-testid="statusbar"]', /\d+ ARP entries?/i, 20_000); await assertNoErrorStrip(driver); } export async function exercisePcapValidation(driver) { await clickButtonText(driver, '.menu-button', 'Tools'); + // Packet Capture must always be listed, on every build. + // + // This used to `return false` and pass when the menu item was missing, so + // the entire capture surface disappearing was indistinguishable from a + // healthy skip. It is now a hard failure: builds without the feature keep + // the tool visible and show setup guidance instead of hiding it, so an + // absent entry means something broke. const pcapVisible = await driver.executeScript( "return Array.from(document.querySelectorAll('.menu-popover-item')).some((button) => button.textContent.trim() === 'Packet Capture');", ); - if (!pcapVisible) { - await driver.findElement(By.css('.workspace')).click(); - await waitForNoElement(driver, '.menu-popover'); - return false; - } + assert.ok( + pcapVisible, + 'Packet Capture must appear in the Tools menu on every build, with setup guidance when unsupported', + ); await clickButtonText(driver, '.menu-popover-item', 'Packet Capture'); await withElement(driver, '[data-testid="pcap-interface-input"]'); const unavailable = await driver.findElements(By.css('[data-testid="pcap-unavailable-state"]')); if (unavailable.length > 0) { - await waitForText(driver, '[data-testid="pcap-unavailable-state"]', /Packet Capture/i); + // The expected state on a published build. Assert the guidance actually + // tells the user what to do, rather than just noting the element exists. + const guidance = await unavailable[0].getText(); + assert.match(guidance, /packet capture/i, 'unavailable state should name the feature'); + assert.match( + guidance, + /install|build|support|npcap|libpcap/i, + 'unavailable state should say how to get a capture-capable build, not just that it is missing', + ); await assertNoErrorStrip(driver); return false; }