From 25e7a96fd58f0a0e9caad18c00131ec16958f3b8 Mon Sep 17 00:00:00 2001 From: amazon7737 Date: Fri, 29 May 2026 14:30:14 +0900 Subject: [PATCH] Fix QA reliability issues from code review Verified against installed chrome-devtools-mcp@1.1.0. Two earlier review claims ("list_* defaults to a 20-item page" and "README scripts are missing") did not hold for the current code and are intentionally not changed; paginate() returns all items when neither pageSize nor pageIdx is passed. - chatbot: question scenario waits only on submitted text when answerDoneText is unset, instead of hanging until timeout (countTextOccurrences('') is always 0) - chatbot: empty-input node-delta tolerance is configurable via spec.maxNodeDelta (default 2); error now reports actual vs allowed - quality: fix misleading "error/warning" label to "error" - artifacts: waitForSnapshot uses an in-memory pollSnapshot() so polling no longer writes files or advances the artifact counter - utils: parseJsonOutput throws on unparseable CLI output instead of returning {raw}, which previously surfaced as "element not found" - add quality/parseJsonOutput regression tests (15 tests pass) --- docs/profile-schema.md | 14 +++++++----- src/core/artifacts.mjs | 10 ++++++++- src/core/quality.mjs | 2 +- src/core/utils.mjs | 4 +++- src/scenarios/chatbot.mjs | 18 ++++++++++++--- test/quality.test.mjs | 47 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 11 deletions(-) create mode 100644 test/quality.test.mjs diff --git a/docs/profile-schema.md b/docs/profile-schema.md index 788bf5f..47551ff 100644 --- a/docs/profile-schema.md +++ b/docs/profile-schema.md @@ -36,11 +36,12 @@ Required selectors for chatbot-style profiles: ```json { - "chatInput": { "role": "textbox", "nameIncludes": "..." }, - "answerDoneText": "..." + "chatInput": { "role": "textbox", "nameIncludes": "..." } } ``` +`answerDoneText` is optional. When set, `question` scenarios wait for an extra occurrence of this text to confirm the answer finished; when omitted, they wait only for the submitted text to appear. + Optional consent selectors: ```json @@ -66,7 +67,7 @@ Finds the consent button, optionally fills a label input, clicks agree, then wai ### `question` -Fills `selectors.chatInput`, presses Enter, waits for submitted text and an additional `selectors.answerDoneText` occurrence. +Fills `selectors.chatInput`, presses Enter, then waits for the submitted text to appear. If `selectors.answerDoneText` is set, it additionally waits for one more occurrence of that marker (used to detect that the answer finished streaming). When `answerDoneText` is omitted, the scenario waits only for the submitted text — it does not hang. ```json { @@ -79,14 +80,17 @@ Fills `selectors.chatInput`, presses Enter, waits for submitted text and an addi ### `empty-input` -Attempts to submit empty/blank input and asserts the accessibility tree does not materially change. +Attempts to submit empty/blank input and asserts the accessibility tree does not materially change (i.e. no new answer was produced). + +`maxNodeDelta` (default `2`) is the number of additional accessibility nodes tolerated after submitting blank input — raise it if the UI legitimately shows an aria-live validation hint on empty submit. ```json { "type": "empty-input", "name": "empty-input-guard", "text": " ", - "waitMs": 500 + "waitMs": 500, + "maxNodeDelta": 2 } ``` diff --git a/src/core/artifacts.mjs b/src/core/artifacts.mjs index 65b0a03..4d3b3d3 100644 --- a/src/core/artifacts.mjs +++ b/src/core/artifacts.mjs @@ -31,11 +31,19 @@ export class ArtifactStore { if (item) item.screenshots.push(rel); } + // In-memory snapshot for polling. Unlike snapshot(), it does not write a file + // or advance the artifact counter, so poll iterations don't flood the output + // directory or scramble the numbering of persisted artifacts. + async pollSnapshot() { + const result = await this.client.run('snapshot-poll', ['take_snapshot']); + return result.snapshot || result; + } + async waitForSnapshot(predicate, timeout) { const deadline = Date.now() + timeout; let last; while (Date.now() < deadline) { - last = await this.snapshot(`poll-${Date.now()}`); + last = await this.pollSnapshot(); if (predicate(last)) return last; await sleep(1000); } diff --git a/src/core/quality.mjs b/src/core/quality.mjs index fe799f9..26e6dc0 100644 --- a/src/core/quality.mjs +++ b/src/core/quality.mjs @@ -3,7 +3,7 @@ export function analyzeQuality(r, quality = {}) { const warnings = []; for (const msg of r.consoleMessages?.consoleMessages || []) { - if (msg.type === 'error') warnings.push(`Console error/warning: ${msg.text}`); + if (msg.type === 'error') warnings.push(`Console error: ${msg.text}`); } for (const req of r.networkRequests?.networkRequests || []) { diff --git a/src/core/utils.mjs b/src/core/utils.mjs index c21cee2..db8e317 100644 --- a/src/core/utils.mjs +++ b/src/core/utils.mjs @@ -27,5 +27,7 @@ export function parseJsonOutput(stdout) { // CLI startup notices can precede JSON; keep scanning. } } - return { raw: stdout }; + // Surface the real CLI failure instead of letting an unparseable response flow + // downstream as missing `.snapshot`, which masquerades as "element not found". + throw new Error(`chrome-devtools did not return parseable JSON output:\n${stdout.slice(0, 500)}`); } diff --git a/src/scenarios/chatbot.mjs b/src/scenarios/chatbot.mjs index b50241c..7451326 100644 --- a/src/scenarios/chatbot.mjs +++ b/src/scenarios/chatbot.mjs @@ -25,10 +25,16 @@ async function runConsentScenario({ spec, item, profile, client, artifacts }) { async function runQuestionScenario({ spec, item, profile, client, artifacts, timeoutMs }) { const before = await artifacts.snapshot(`${item.name}-before`, item); - const beforeCount = countTextOccurrences(before, profile.selectors?.answerDoneText || ''); + const answerDoneText = profile.selectors?.answerDoneText || ''; + const beforeCount = countTextOccurrences(before, answerDoneText); await askFromSnapshot({ snap: before, text: spec.text || '', profile, client }); const done = await artifacts.waitForSnapshot((snap) => { - return hasText(snap, spec.text || '') && countTextOccurrences(snap, profile.selectors?.answerDoneText || '') >= beforeCount + 1; + if (!hasText(snap, spec.text || '')) return false; + // When no answerDoneText marker is configured, degrade to waiting only for + // the submitted text. countTextOccurrences('') is always 0, so requiring the + // marker here would otherwise guarantee a timeout. + if (!answerDoneText) return true; + return countTextOccurrences(snap, answerDoneText) >= beforeCount + 1; }, spec.timeoutMs || timeoutMs); assert(hasText(done, spec.text || ''), 'submitted text should appear in snapshot'); await artifacts.snapshot(`${item.name}-after`, item); @@ -41,7 +47,13 @@ async function runEmptyInputScenario({ spec, item, profile, client, artifacts }) await askFromSnapshot({ snap: before, text: spec.text || ' ', profile, client }); await sleep(spec.waitMs || 500); const after = await artifacts.snapshot(`${item.name}-after`, item); - assert(flatten(after).length <= beforeTextCount + 2, 'empty input should not materially change the accessibility tree'); + // Empty input should not produce a new answer. A few extra nodes (e.g. an + // aria-live validation hint) are tolerated; tune via spec.maxNodeDelta. + const maxNodeDelta = Number.isInteger(spec.maxNodeDelta) ? spec.maxNodeDelta : 2; + assert( + flatten(after).length <= beforeTextCount + maxNodeDelta, + `empty input should not materially change the accessibility tree (added ${flatten(after).length - beforeTextCount} nodes, allowed ${maxNodeDelta})`, + ); await artifacts.screenshot(`${item.name}-after`, item); } diff --git a/test/quality.test.mjs b/test/quality.test.mjs new file mode 100644 index 0000000..0c7136e --- /dev/null +++ b/test/quality.test.mjs @@ -0,0 +1,47 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { analyzeQuality } from '../src/core/quality.mjs'; +import { parseJsonOutput } from '../src/core/utils.mjs'; + +test('analyzeQuality flags 5xx as failure and 4xx as warning', () => { + const result = analyzeQuality({ + networkRequests: { + networkRequests: [ + { method: 'GET', url: 'https://x/api', status: '502' }, + { method: 'GET', url: 'https://x/missing', status: '404' }, + { method: 'GET', url: 'https://x/ok', status: '200' }, + ], + }, + }); + assert.equal(result.status, 'fail'); + assert.equal(result.failures.length, 1); + assert.match(result.failures[0], /502/); + assert.equal(result.warnings.length, 1); + assert.match(result.warnings[0], /404/); +}); + +test('analyzeQuality reports console errors as warnings', () => { + const result = analyzeQuality({ + consoleMessages: { consoleMessages: [{ type: 'error', text: 'boom' }] }, + }); + assert.equal(result.status, 'warning'); + assert.equal(result.warnings[0], 'Console error: boom'); +}); + +test('analyzeQuality honors ignoreFavicon404', () => { + const result = analyzeQuality( + { networkRequests: { networkRequests: [{ method: 'GET', url: 'https://x/favicon.ico', status: '404' }] } }, + { ignoreFavicon404: true }, + ); + assert.equal(result.status, 'pass'); + assert.equal(result.warnings.length, 0); +}); + +test('parseJsonOutput returns the last JSON object after notices', () => { + const stdout = 'startup notice\nupdate available\n{"snapshot":{"role":"RootWebArea"}}'; + assert.deepEqual(parseJsonOutput(stdout), { snapshot: { role: 'RootWebArea' } }); +}); + +test('parseJsonOutput throws on unparseable output instead of masking it', () => { + assert.throws(() => parseJsonOutput('Error: daemon failed to start'), /did not return parseable JSON/); +});