From 15071b58bcb71a946b68c70ebd9524d967066775 Mon Sep 17 00:00:00 2001 From: Keiichiro Ui Date: Sun, 12 Jul 2026 17:12:15 +0900 Subject: [PATCH] Audit e2e/ and playwright.config.ts comments (#225 Phase 2) - e2e/index.spec.ts: delete self-evident comments, convert rationale to WHY: - e2e/prefabs.spec.ts: delete self-evident comments, convert rationale to WHY: - playwright.config.ts: move workers rationale to JSDoc - Remove empty EXCLUDED_PATHS and isExcludedFile from lint plugin --- e2e/index.spec.ts | 40 +++++-------------- e2e/prefabs.spec.ts | 16 +++----- playwright.config.ts | 6 +-- .../lint-plugins/require-comment-rationale.ts | 20 +--------- tools/test/require-comment-rationale.test.ts | 16 +------- 5 files changed, 20 insertions(+), 78 deletions(-) diff --git a/e2e/index.spec.ts b/e2e/index.spec.ts index c5d68fc5d..5819a4f47 100644 --- a/e2e/index.spec.ts +++ b/e2e/index.spec.ts @@ -16,8 +16,7 @@ test.describe("index.html", () => { test("language select change is persisted in localStorage", async ({ page }) => { await page.goto("/index.html"); - // The label-lang select is populated asynchronously by LabelHandler; wait - // for at least one extra option to appear beyond the default. + // WHY: LabelHandler populates the select asynchronously, so wait for options before interacting. await expect .poll(async () => await page.locator("#label-lang option").count()) .toBeGreaterThan(1); @@ -32,8 +31,6 @@ test.describe("index.html", () => { test("min-tier cannot exceed max-tier — they correct each other", async ({ page }) => { await page.goto("/index.html"); - // Set min above current max; min-max-inputs.ts should drag max up so - // max >= min. await page.locator("#max-tier").evaluate((el: HTMLInputElement) => { el.value = "2"; el.dispatchEvent(new Event("input", { bubbles: true })); @@ -52,18 +49,15 @@ test.describe("index.html", () => { test.setTimeout(60_000); await page.goto("/index.html"); - // Wait for the bundled-map select to be populated. + // WHY: The bundled-map select is populated asynchronously. await expect .poll(async () => await page.locator("#bundled-map-select option").count() ) .toBeGreaterThan(1); - // Install an observer that latches when the load/processing dialog opens - // and when it subsequently closes. The dialog is opened via showModal() - // and closed once processing finishes; using the close transition as the - // "processing done" signal avoids polling canvas dimensions before the - // worker has resized the canvas. + /* WHY: Latch dialog open/close via MutationObserver so we can detect + when processing finishes without polling canvas dimensions prematurely. */ await page.evaluate(() => { const w = globalThis as unknown as { __dialogWasOpen?: boolean; @@ -80,7 +74,6 @@ test.describe("index.html", () => { await page.selectOption("#bundled-map-select", "Navezgane"); - // Loading dialog should have been open at some point during processing. await expect .poll(async () => await page.evaluate(() => @@ -90,9 +83,7 @@ test.describe("index.html", () => { ) .toBe(true); - // Wait for the dialog to close, signalling processing finished. Cold - // workers under parallel test load can take tens of seconds, so allow a - // generous window before checking canvas dimensions. + // WHY: Cold workers under parallel test load can take tens of seconds, so allow a generous timeout before asserting canvas dimensions. await expect .poll( async () => @@ -104,8 +95,7 @@ test.describe("index.html", () => { ) .toBe(true); - // Canvas dimensions: Navezgane HeightMapSize is 6144x6144 and the default - // render scale is 0.12 -> 6144 * 0.12 = 737.28 -> 737. + // WHY: Navezgane HeightMapSize is 6144x6144 and the default render scale is 0.12, so the expected canvas size is 6144 * 0.12 = 737.28 -> 737. const slow = { timeout: 20_000 }; await expect.poll( async () => @@ -118,19 +108,16 @@ test.describe("index.html", () => { slow, ).toBe(737); - // Prefab list is populated. await expect.poll( async () => await page.locator("#prefabs-list li").count(), slow, ).toBeGreaterThan(0); - // Map name input reflects the selection. await expect.poll( async () => await page.locator("#map-name").inputValue(), slow, ).toBe("Navezgane"); - // Apply the "trader" prefab-filter preset. await page.click( 'button[data-input-prefab-filter="trader"]:text-is("All Traders")', ); @@ -141,7 +128,6 @@ test.describe("index.html", () => { await page.locator("#prefabs-list li").count() ).toBeGreaterThan(0); - // Clear the prefab filter via its X button (data-input-prefab-filter=""). await page.click( 'button[data-input-prefab-filter=""]', ); @@ -149,7 +135,6 @@ test.describe("index.html", () => { await page.locator("#prefab-filter").inputValue() ).toBe(""); - // Apply the "Super Corn" block-filter preset. await page.click( 'button[data-input-block-filter="(Grace|Super)Corn"]:text-is("Super Corn")', ); @@ -160,8 +145,6 @@ test.describe("index.html", () => { await page.locator("#prefabs-list li").count() ).toBeGreaterThan(0); - // Narrow the tier range to [0, 0]; with Super Corn still in the block - // filter, no prefab should match. await page.locator("#max-tier").evaluate((el: HTMLInputElement) => { el.value = "0"; el.dispatchEvent(new Event("input", { bubbles: true })); @@ -180,8 +163,7 @@ test.describe("index.html", () => { test("block filter input stays responsive (freeze regression)", async ({ page }) => { test.setTimeout(60_000); - // Collect long tasks with their start times so we can look only at the - // window around typing, ignoring long tasks from the initial map load. + // WHY: Collect long-task timings so we can isolate typing-window latency from initial load overhead. await page.addInitScript(() => { const w = globalThis as unknown as { __longtasks?: { start: number; duration: number }[]; @@ -248,8 +230,7 @@ test.describe("index.html", () => { }, typingStart); const maxSingle = durations.length ? Math.max(...durations) : 0; const total = durations.reduce((a, b) => a + b, 0); - // Thresholds are loose vs the pre-fix baseline (single 1,864ms / total - // 5,100ms) to tolerate CI speed variance; prefer retries over loosening. + // WHY: Thresholds are loose versus the pre-fix baseline (single 1,864ms / total 5,100ms) so CI speed variance does not cause flakes. expect(maxSingle).toBeLessThan(500); expect(total).toBeLessThan(1500); @@ -272,10 +253,8 @@ test.describe("index.html", () => { "terrain-viewer-title", ); - // Show button is disabled until a DTM is loaded. await expect(page.locator("#terrain-viewer-show")).toBeDisabled(); - // Close button carries an accessible name for screen readers. await expect(page.locator("#terrain-viewer-close")) .toHaveAttribute("aria-label", "Close terrain viewer"); }); @@ -300,8 +279,7 @@ test.describe("index.html", () => { ); await expect(button).toBeVisible(); - // Replace clipboard.writeText with a spy stored on globalThis so the test - // can assert it was invoked by a keyboard activation. + // WHY: Replace clipboard.writeText with a spy on globalThis so the test can assert keyboard activation invoked it. await page.evaluate(() => { const w = globalThis as unknown as { __copyCalls: string[]; diff --git a/e2e/prefabs.spec.ts b/e2e/prefabs.spec.ts index 99509aa5d..6147aeb5e 100644 --- a/e2e/prefabs.spec.ts +++ b/e2e/prefabs.spec.ts @@ -6,7 +6,7 @@ test.describe("prefabs.html", () => { await page.goto("/prefabs.html"); await expect(page.locator("#prefab-filter")).toBeVisible(); await expect(page.locator("#prefabs-list")).toBeAttached(); - // The status updates when the worker finishes the first filter pass. + // WHY: Poll until the worker finishes the first filter pass so the status text is non-empty. await expect.poll(async () => (await page.locator("#prefabs-status").textContent())?.trim().length ?? 0 ).toBeGreaterThan(0); @@ -15,7 +15,7 @@ test.describe("prefabs.html", () => { test("filter input updates the URL querystring", async ({ page }) => { await page.goto("/prefabs.html"); await page.fill("#prefab-filter", "trader"); - // URL update is debounced behind UrlState; poll until it shows up. + // WHY: UrlState debounces updates, so poll until the querystring reflects the input. await expect.poll(() => new URL(page.url()).searchParams.get("prefab-filter") ) @@ -36,13 +36,10 @@ test.describe("prefabs.html", () => { }); test("highlighted prefab names do not execute injected scripts (XSS regression)", async ({ page }) => { - // The XSS surface was the highlighted name being injected via innerHTML. - // We can't easily plant a malicious-named prefab without controlling the - // backing JSON, so we verify (a) the page does not surface any - // accidental dialog (alert/confirm/prompt) within a short window, and - // (b) elements are produced when filtering — which proves the - // highlighter path runs and uses the escape-then-mark pipeline added in - // commit 92abcb8c. + /* WHY: We can't easily plant a malicious-named prefab without controlling + the backing JSON. We therefore verify (a) no accidental dialog fires + and (b) elements are produced, which proves the escape-then-mark + pipeline added in commit 92abcb8c is active. */ let dialogTriggered = false; page.on("dialog", async (d) => { dialogTriggered = true; @@ -50,7 +47,6 @@ test.describe("prefabs.html", () => { }); await page.goto("/prefabs.html"); await page.fill("#prefab-filter", "house"); - // Wait for at least one filtered result. await expect .poll(async () => await page.locator("#prefabs-list li").count()) .toBeGreaterThan(0); diff --git a/playwright.config.ts b/playwright.config.ts index afcbc22cb..458d57acf 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -10,9 +10,9 @@ export default defineConfig({ fullyParallel: true, forbidOnly: !!CI, retries: CI ? 2 : 0, - // The bundled-map load test fetches ~5MB of assets through the dev server. - // Cap workers to keep server contention bounded even though serve:static is - // sturdier than esbuild's serve under load. + /** The bundled-map load test fetches ~5MB of assets through the dev server. + * Cap workers to keep server contention bounded even though serve:static is + * sturdier than esbuild's serve under load. */ workers: CI ? 1 : 2, reporter: CI ? [["html", { open: "never" }], ["list"]] : "list", timeout: 30_000, diff --git a/tools/lint-plugins/require-comment-rationale.ts b/tools/lint-plugins/require-comment-rationale.ts index 083ec3ba8..81ac971f2 100644 --- a/tools/lint-plugins/require-comment-rationale.ts +++ b/tools/lint-plugins/require-comment-rationale.ts @@ -17,18 +17,9 @@ const PRESERVE_PATTERNS: readonly string[] = [ "FIXME", ]; -/* WHY: gradual rollout ratchet for #225. Entries skip the rule so existing code - is not audited in one big sweep; shrink to empty, then delete this mechanism. - Match is POSIX-style substring on the absolute filename. */ -const EXCLUDED_PATHS: readonly string[] = [ - "/e2e/index.spec.ts", - "/e2e/prefabs.spec.ts", - "/playwright.config.ts", -]; - const TRIPLE_SLASH_RE = /^\/\s*<(reference|amd-)/; -export function shouldPreserve( +function shouldPreserve( comment: { type: string; value: string }, ): boolean { const raw = comment.value; @@ -39,20 +30,11 @@ export function shouldPreserve( return PRESERVE_PATTERNS.some((p) => raw.includes(p)); } -export function isExcludedFile( - filename: string, - excludedPaths: readonly string[] = EXCLUDED_PATHS, -): boolean { - const normalized = filename.replaceAll("\\", "/"); - return excludedPaths.some((p) => normalized.includes(p)); -} - const plugin: Deno.lint.Plugin = { name: "local", rules: { "require-comment-rationale": { create(context) { - if (isExcludedFile(context.filename)) return {}; return { Program() { for (const comment of context.sourceCode.getAllComments()) { diff --git a/tools/test/require-comment-rationale.test.ts b/tools/test/require-comment-rationale.test.ts index 825db09a4..10f2d8528 100644 --- a/tools/test/require-comment-rationale.test.ts +++ b/tools/test/require-comment-rationale.test.ts @@ -1,9 +1,7 @@ /// import { expect } from "@std/expect"; import { describe, it } from "@std/testing/bdd"; -import plugin, { - isExcludedFile, -} from "../lint-plugins/require-comment-rationale.ts"; +import plugin from "../lint-plugins/require-comment-rationale.ts"; function run(source: string, filename = "main.ts") { return Deno.lint.runPlugin(plugin, filename, source); @@ -87,18 +85,6 @@ describe("require-comment-rationale", () => { expect(d.length).toBe(1); }); - it("skips files matching excluded path substrings", () => { - expect(isExcludedFile("/repo/src/index/foo.ts", ["/src/index/"])).toBe( - true, - ); - expect(isExcludedFile("/repo/src/lib/bar.ts", ["/src/index/"])).toBe(false); - expect(isExcludedFile("C:\\repo\\src\\index\\foo.ts", ["/src/index/"])) - .toBe(true); - expect(isExcludedFile("/repo/src/lib/foo.ts", ["/src/lib/foo.ts"])).toBe( - true, - ); - }); - it("flags multiple bare comments in one file", () => { const d = run( "// first\n// second\n// WHY: keep me\n// fourth\nconst x = 1;\n",