From c2f41e123be6a0632ad16497d5d875d8aa7280f6 Mon Sep 17 00:00:00 2001 From: ch <1938389+netravnen@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:21:16 +0200 Subject: [PATCH] test(build): make the fake fetch method-aware and header-readable The node:test harness could not express the cases needed to test lib/admincom-common.js's retry/backoff wrappers, which is why that code shipped with a defect nothing caught. Two limits caused it. The fake response exposed only headers.forEach(), while fetchWithRetry reads response.headers.get('retry-after') -- so any 503 fixture threw TypeError before reaching the logic under test. And the fake fetch was `async (url) =>`, dropping the init argument entirely, so a test could not observe the request method or count requests. Since a retried request and a single one return the same value, request count was the only observable that distinguishes them, and it was unavailable. This closes both so the following commit can test the retry policy, and pins the harness's own capabilities: a gap in the harness is invisible in a green run, and produces confident, empty coverage. Changes: - Extracted makeFakeResponse(status, body, headers) with a real case-insensitive headers.get()/has()/forEach(), correct ok derivation, and json()/text(). - makeFakeFetch now takes (url, init) and appends { url, method, body, headers } to a per-load call log, returned from loadScript as `fetchCalls`. - fetchMap entries may now be a response descriptor ({ __response: true, status, body, headers }) to model failures and header-bearing responses; a bare JSON body still means 200, unchanged. - Added tests/browser-shim.test.js asserting these capabilities directly. Security: - N/A -- test harness only, never inlined into a generated .user.js. Testing: - node --test from user.js/: 480 tests, 479 pass, 1 skipped (live tests are opt-in), up from 476/475. - All 476 pre-existing tests pass unchanged, confirming the bare-body fetchMap form and the 404-on-unmapped-URL behavior are preserved. Backwards Compatibility: - Purely additive for existing callers: the descriptor form is opt-in via an explicit __response marker, and loadScript's return value gains a property rather than changing one. Assisted-by: Claude:claude-opus-5 --- user.js/tests/browser-shim.test.js | 84 +++++++++++++++++++++++++++ user.js/tests/helpers/browser-shim.js | 81 ++++++++++++++++++++------ 2 files changed, 148 insertions(+), 17 deletions(-) create mode 100644 user.js/tests/browser-shim.test.js diff --git a/user.js/tests/browser-shim.test.js b/user.js/tests/browser-shim.test.js new file mode 100644 index 0000000..80d5c35 --- /dev/null +++ b/user.js/tests/browser-shim.test.js @@ -0,0 +1,84 @@ +'use strict'; + +// The test harness itself, pinned. +// +// This file exists because a gap in the harness -- not in the code under test +// -- is what let a critical bug ship. lib/admincom-common.js's fetchWithRetry +// reads response.headers.get('retry-after'), and the fake response used to +// expose only forEach(), so any 503 fixture threw TypeError before reaching +// the retry logic. The retry path was not merely untested; it was inexpressible. +// A harness whose limits are invisible produces confident, empty coverage, so +// the capabilities other tests depend on are asserted here directly. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { loadScript, makeFakeResponse } = require('./helpers/browser-shim'); + +const SCRIPT_PATH = path.join(__dirname, '..', 'peeringdb-deskpro-tools.user.js'); + +test('fake response headers support get(), not just forEach()', () => { + const res = makeFakeResponse(503, {}, { 'Retry-After': '3' }); + + assert.equal(res.status, 503); + assert.equal(res.ok, false); + assert.equal(res.headers.get('Retry-After'), '3'); + assert.equal(res.headers.get('retry-after'), '3', 'header lookup must be case-insensitive'); + assert.equal(res.headers.get('absent'), null, 'a missing header reads as null, like the real API'); + assert.equal(res.headers.has('retry-after'), true); +}); + +test('a 2xx status is ok, a 4xx/5xx is not', () => { + assert.equal(makeFakeResponse(200, {}).ok, true); + assert.equal(makeFakeResponse(204, {}).ok, true); + assert.equal(makeFakeResponse(404, {}).ok, false); + assert.equal(makeFakeResponse(503, {}).ok, false); +}); + +test('fetchMap accepts a response descriptor alongside a bare body', async () => { + const url = 'https://www.peeringdb.com/api/net/1'; + const { window } = loadScript(SCRIPT_PATH, { + hooksKey: '__pdbDpTestHooks__', + pathname: '/app/ticket', + fetchMap: { + [url]: { __response: true, status: 503, body: { detail: 'busy' }, headers: { 'Retry-After': '1' } }, + 'https://www.peeringdb.com/api/net/2': { data: [{ id: 2 }] }, + }, + }); + + const failing = await window.fetch(url); + assert.equal(failing.status, 503); + assert.equal(failing.headers.get('retry-after'), '1'); + assert.deepEqual(await failing.json(), { detail: 'busy' }); + + // The bare-body form must keep working unchanged -- every pre-existing test + // depends on it. + const ok = await window.fetch('https://www.peeringdb.com/api/net/2'); + assert.equal(ok.status, 200); + assert.equal(ok.ok, true); + assert.deepEqual(await ok.json(), { data: [{ id: 2 }] }); + + // An unmapped URL still 404s rather than throwing. + const missing = await window.fetch('https://www.peeringdb.com/api/net/999'); + assert.equal(missing.status, 404); +}); + +test('every request is recorded with its method, so retries are observable', async () => { + const url = 'https://www.peeringdb.com/api/netixlan/7'; + const { window, fetchCalls } = loadScript(SCRIPT_PATH, { + hooksKey: '__pdbDpTestHooks__', + pathname: '/app/ticket', + fetchMap: { [url]: { data: [] } }, + }); + + assert.equal(fetchCalls.length, 0, 'no requests before anything runs'); + + await window.fetch(url); + await window.fetch(url, { method: 'delete' }); + await window.fetch(url, { method: 'PUT', body: '{"speed":1000}' }); + + assert.equal(fetchCalls.length, 3); + assert.deepEqual(fetchCalls.map((c) => c.method), ['GET', 'DELETE', 'PUT'], 'method is captured and upper-cased'); + assert.equal(fetchCalls[0].url, url); + assert.equal(fetchCalls[2].body, '{"speed":1000}'); +}); diff --git a/user.js/tests/helpers/browser-shim.js b/user.js/tests/helpers/browser-shim.js index c601032..0e63c89 100644 --- a/user.js/tests/helpers/browser-shim.js +++ b/user.js/tests/helpers/browser-shim.js @@ -165,6 +165,11 @@ function loadScript(scriptPath, opts) { localStorage = makeFakeStorage(), } = opts; + // Every fake-fetch request lands here in order. Returned from loadScript so a + // test can assert request count and method -- the only observable difference + // between one request and a retried one. + const fetchCalls = []; + const source = fs.readFileSync(scriptPath, 'utf-8'); const fakeDocument = { @@ -207,7 +212,7 @@ function loadScript(scriptPath, opts) { URL, URLSearchParams, AbortController, - fetch: makeFakeFetch(fetchMap), + fetch: makeFakeFetch(fetchMap, fetchCalls), MutationObserver: class { observe() {} disconnect() {} @@ -231,7 +236,30 @@ function loadScript(scriptPath, opts) { throw new Error(`${path.basename(scriptPath)} did not expose window.${hooksKey} -- was window.__PDB_TEST__ wired up?`); } - return { window: sandbox, document: fakeDocument, hooks }; + return { window: sandbox, document: fakeDocument, hooks, fetchCalls }; +} + +/** + * Builds a fake response object with headers real enough for production code + * to read. The previous version exposed only forEach(), so any code calling + * response.headers.get() -- which lib/admincom-common.js's fetchWithRetry does + * for Retry-After -- threw TypeError before its logic could be reached. That + * is why the retry/backoff path had no coverage: not oversight, but a harness + * that could not express the case. + */ +function makeFakeResponse(status, body, headers = {}) { + const lowered = new Map(Object.entries(headers).map(([k, v]) => [k.toLowerCase(), String(v)])); + return { + ok: status >= 200 && status < 300, + status, + headers: { + get: (name) => (lowered.has(String(name).toLowerCase()) ? lowered.get(String(name).toLowerCase()) : null), + has: (name) => lowered.has(String(name).toLowerCase()), + forEach: (cb) => lowered.forEach((v, k) => cb(v, k)), + }, + json: async () => body, + text: async () => JSON.stringify(body), + }; } /** @@ -240,24 +268,43 @@ function loadScript(scriptPath, opts) { * Purpose: Let API-calling functions run for real (no manual mocking of the * function itself) while keeping tests fully offline/deterministic -- no * accidental live traffic, no network flakiness. + * + * A fetchMap value is normally the JSON body to return with a 200. To model a + * failure or a header-bearing response, use the descriptor form instead: + * + * { __response: true, status: 503, body: {}, headers: { 'Retry-After': '1' } } + * + * Every call is appended to `calls` as { url, method, body, headers }, so a + * test can assert *how many* requests a function issued and with which method + * -- the only way to observe retry behavior, since a retried request is + * indistinguishable from a single one by its return value alone. + * + * @param {object} fetchMap - URL -> JSON body, or URL -> response descriptor. + * @param {object[]} calls - Array the fake appends each request to, in order. */ -function makeFakeFetch(fetchMap) { - return async (url) => { +function makeFakeFetch(fetchMap, calls) { + return async (url, init = {}) => { const key = String(url); + calls.push({ + url: key, + method: String(init.method || 'GET').toUpperCase(), + body: init.body, + headers: init.headers, + }); + if (!Object.prototype.hasOwnProperty.call(fetchMap, key)) { - return { - ok: false, - status: 404, - headers: { forEach() {} }, - json: async () => ({}), - }; + return makeFakeResponse(404, {}); + } + + const entry = fetchMap[key]; + if (entry && typeof entry === 'object' && entry.__response === true) { + return makeFakeResponse( + typeof entry.status === 'number' ? entry.status : 200, + 'body' in entry ? entry.body : {}, + entry.headers || {}, + ); } - return { - ok: true, - status: 200, - headers: { forEach() {} }, - json: async () => fetchMap[key], - }; + return makeFakeResponse(200, entry); }; } @@ -282,4 +329,4 @@ function makeFakeStorage() { }; } -module.exports = { loadScript, el, FakeElement, FakeTextNode, FakeDocumentFragment, makeFakeStorage }; +module.exports = { loadScript, el, FakeElement, FakeTextNode, FakeDocumentFragment, makeFakeStorage, makeFakeResponse };