mirror of
https://github.com/kevinveenbirkenbach/computer-playbook.git
synced 2026-11-10 12:47:04 +00:00
fix(playwright): stop target-role lists from silently reading as empty
Five specs parsed their target list as
const raw = process.env.X_TARGET_ROLES_JSON || "[]";
try { const parsed = JSON.parse(raw); return Array.isArray(parsed) ? parsed : []; }
catch { return []; }
`docker --env-file` preserves the quotes that `dotenv_quote` emits, so the
value arrives as a JSON string containing JSON. `JSON.parse` then SUCCEEDS and
returns a string, `Array.isArray` is false, and the quiet `: []` branch hands
back an empty list. The `catch` never runs, because nothing throws. Every
assertion that iterates such a list then runs zero times and passes.
The cost is measurable. Main's own Nextcloud#8 (job 109321355456) tested
`web-app-peertube` and `web-app-prometheus` together and passed its metricz
test in 20 ms - the per-role loop does a 30 s warm GET and a 60 s poll, so it
cannot have run once - while `test.skip` reported "no prometheus consumer
roles in inventory" with twelve of them in the list. peertube's missing metric
label sat behind that for as long as the assertion existed.
`decodeDotenvJsonList` takes the value through the decoder this module already
owns, so the quoted form is unwrapped rather than rejected, and throws when
what remains is not an array. An empty list is now a real answer only when the
variable is absent or empty. Four call sites move over: web-app-prometheus,
web-svc-css, web-app-dashboard's tile spec and web-app-matomo's shared module.
The fifth, web-svc-logout, carries an unrelated IdP change in the same file and
lands with that one.
The ESLint rule reports the shape rather than this instance of it:
`no-restricted-syntax` on a `CatchClause` whose block returns without throwing,
opted out of per site with the built-in `eslint-disable` comment. It is "warn",
not "error", because 26 sites predate it and each needs its own verdict - some
drop a malformed entry deliberately, some hide a config error. That is the
staging `prefer-web-first-assertions` already uses in the same file.
Consequence worth stating: in compose these four lists were empty and are now
filled, so dashboard tiles, matomo tracking, CSS injection and prometheus
labels are asserted there for the first time. Failures this surfaces are
pre-existing, not introduced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
14e55b8aee
commit
ce1a19d3c2
6 files changed
+67
-42
No files matched your search
@@ -109,6 +109,20 @@ module.exports = [
|
||||
// a hand-migration pass; promote to "error" as those are reviewed.
|
||||
"playwright/prefer-web-first-assertions": "off",
|
||||
"playwright/no-wait-for-navigation": "error",
|
||||
// A catch returning a fallback reads downstream as a legitimate value:
|
||||
// `JSON.parse` on a dotenv-quoted list yields a string, the fallback
|
||||
// made it `[]`, and every assertion over it passed vacuously. "warn"
|
||||
// while the 26 pre-existing sites get their individual verdict, then
|
||||
// "error" - the staging this file already uses above.
|
||||
"no-restricted-syntax": [
|
||||
"warn",
|
||||
{
|
||||
selector:
|
||||
"CatchClause > BlockStatement:has(ReturnStatement):not(:has(ThrowStatement))",
|
||||
message:
|
||||
"This catch swallows the error and returns a fallback, which reads downstream as a legitimate value. Rethrow, assert, or add `// eslint-disable-next-line no-restricted-syntax` naming why swallowing is correct here.",
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
];
|
||||
@@ -33,4 +33,36 @@ function normalizeBaseUrl(value) {
|
||||
return decodeDotenvQuotedValue(value || "").replace(/\/$/, "");
|
||||
}
|
||||
|
||||
module.exports = { decodeDotenvQuotedValue, normalizeBaseUrl };
|
||||
/**
|
||||
* Decode a dotenv value carrying a JSON array of target roles.
|
||||
*
|
||||
* `JSON.parse` on the quoted form succeeds and yields a STRING, which
|
||||
* `Array.isArray` then reads as "no targets" - every assertion over the list
|
||||
* passes vacuously. Only an absent or empty variable yields an empty list.
|
||||
*/
|
||||
function decodeDotenvJsonList(raw, name) {
|
||||
const decoded = decodeDotenvQuotedValue(raw || "");
|
||||
if (decoded === "") {
|
||||
return [];
|
||||
}
|
||||
let parsed;
|
||||
try {
|
||||
parsed = JSON.parse(decoded);
|
||||
} catch (error) {
|
||||
throw new Error(`${name} is not valid JSON`, { cause: error });
|
||||
}
|
||||
if (!Array.isArray(parsed)) {
|
||||
throw new Error(
|
||||
`${name} decoded to ${typeof parsed}, not an array — the value reached ` +
|
||||
"the container still dotenv-quoted, and reading it as an empty list " +
|
||||
"would pass every assertion that iterates it."
|
||||
);
|
||||
}
|
||||
return parsed;
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
decodeDotenvQuotedValue,
|
||||
decodeDotenvJsonList,
|
||||
normalizeBaseUrl,
|
||||
};
|
||||
@@ -8,18 +8,12 @@
|
||||
|
||||
const { test, expect } = require("./fixtures/onion-test");
|
||||
const { resolveTimeout } = require("./timeouts");
|
||||
const { gotoOnion } = require("./personas");
|
||||
|
||||
const dashboardTargetRoles = (() => {
|
||||
const raw = process.env.DASHBOARD_TARGET_ROLES_JSON || "[]";
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
return Array.isArray(parsed) ? parsed : [];
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
})();
|
||||
const { decodeDotenvJsonList, gotoOnion } = require("./personas");
|
||||
|
||||
const dashboardTargetRoles = decodeDotenvJsonList(
|
||||
process.env.DASHBOARD_TARGET_ROLES_JSON,
|
||||
"DASHBOARD_TARGET_ROLES_JSON"
|
||||
);
|
||||
async function findVisibleTile(page, canonicalDomain) {
|
||||
const tile = page.locator(`a[href*="${canonicalDomain}"]:visible`).first();
|
||||
const visible = await tile.isVisible().catch(() => false);
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
|
||||
const { expect } = require("@playwright/test");
|
||||
const { resolveTimeout } = require("./timeouts");
|
||||
const { decodeDotenvQuotedValue, gotoOnion, installCspViolationObserver, normalizeBaseUrl } = require("./personas");
|
||||
const { decodeDotenvJsonList, decodeDotenvQuotedValue, gotoOnion, installCspViolationObserver, normalizeBaseUrl } = require("./personas");
|
||||
|
||||
const appBaseUrl = normalizeBaseUrl(process.env.APP_BASE_URL || "");
|
||||
const oidcIssuerUrl = normalizeBaseUrl(process.env.OIDC_ISSUER_URL || "");
|
||||
@@ -28,15 +28,10 @@ const matomoCanonicalDomain = (() => {
|
||||
// Emitted at deploy time by templates/playwright.env.j2 via the
|
||||
// roles_with_service('matomo') Ansible filter: one entry per role declared as a
|
||||
// matomo consumer in its meta/services.yml.
|
||||
const matomoTargetRoles = (() => {
|
||||
const raw = process.env.MATOMO_TARGET_ROLES_JSON || "[]";
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
return Array.isArray(parsed) ? parsed : [];
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
})();
|
||||
const matomoTargetRoles = decodeDotenvJsonList(
|
||||
process.env.MATOMO_TARGET_ROLES_JSON,
|
||||
"MATOMO_TARGET_ROLES_JSON"
|
||||
);
|
||||
|
||||
function attachDiagnostics(page) {
|
||||
const consoleErrors = [];
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
const { test, expect } = require("@playwright/test");
|
||||
const { resolveTimeout } = require("./timeouts");
|
||||
|
||||
const { apiGetOnion, decodeDotenvQuotedValue, gotoOnion, inAppLogout, performKeycloakLoginForm, runAdminFlow, runBiberFlow, runGuestFlow, safeSkipUnlessEnabled } = require("./personas");
|
||||
const { apiGetOnion, decodeDotenvJsonList, decodeDotenvQuotedValue, gotoOnion, inAppLogout, performKeycloakLoginForm, runAdminFlow, runBiberFlow, runGuestFlow, safeSkipUnlessEnabled } = require("./personas");
|
||||
test.use({
|
||||
ignoreHTTPSErrors: true
|
||||
});
|
||||
@@ -210,15 +210,10 @@ test("prometheus: biber is denied access after sso login", async ({ browser }) =
|
||||
// roles' personas no longer drive the prometheus surface.
|
||||
// -----------------------------------------------------------------------------
|
||||
|
||||
const prometheusTargetRoles = (() => {
|
||||
const raw = process.env.PROMETHEUS_TARGET_ROLES_JSON || "[]";
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
return Array.isArray(parsed) ? parsed : [];
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
})();
|
||||
const prometheusTargetRoles = decodeDotenvJsonList(
|
||||
process.env.PROMETHEUS_TARGET_ROLES_JSON,
|
||||
"PROMETHEUS_TARGET_ROLES_JSON"
|
||||
);
|
||||
|
||||
test("prometheus scrape: every consumer role reports up=1", async ({ page }) => {
|
||||
test.skip(prometheusTargetRoles.length === 0, "no prometheus consumer roles in inventory");
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
const { test, expect } = require("@playwright/test");
|
||||
|
||||
const { assertInjectedAssetLoadsWithoutCspBlock, decodeDotenvQuotedValue } = require("./personas");
|
||||
const { assertInjectedAssetLoadsWithoutCspBlock, decodeDotenvJsonList, decodeDotenvQuotedValue } = require("./personas");
|
||||
const { resolveTimeout } = require("./timeouts");
|
||||
|
||||
test.use({ ignoreHTTPSErrors: true });
|
||||
@@ -19,15 +19,10 @@ const cssAssetHosts = [cdnBaseUrl, cssBaseUrl]
|
||||
})
|
||||
.filter(Boolean);
|
||||
|
||||
const cssTargetRoles = (() => {
|
||||
const raw = process.env.CSS_TARGET_ROLES_JSON || "[]";
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
return Array.isArray(parsed) ? parsed : [];
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
})();
|
||||
const cssTargetRoles = decodeDotenvJsonList(
|
||||
process.env.CSS_TARGET_ROLES_JSON,
|
||||
"CSS_TARGET_ROLES_JSON"
|
||||
);
|
||||
|
||||
test.beforeEach(() => {
|
||||
expect(
|
||||
|
||||
Reference in new issue
Block a user