From d375cb4b84b1c03b99aed78aa5dc4135574c832c Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 13:42:42 -0600 Subject: [PATCH] test: act on review -- the sprite gap, and the branch no e2e can reach Both comments were right, and the second named a real blind spot. The e2e iterated a hardcoded theme list while the unit suite iterated `themeNames`, so a fifth theme would have got mapping coverage for free and no cold start. It imports `themeNames` and `themeSessionKey` now. The sprite was invisible to it. A wrong sprite name 404s the icons and MapLibre carries on, so the canvas exists and the suite passes -- exactly the case the review described. The spec now intercepts `/map-assets/sprites/**` and asserts the requested set is `mapSprite()`. Planted it by collapsing the sprite back into the flavor: night and amber get PAST the canvas assertion and fail on the sprite, which is the point. The rebuild branch cannot be reached from a browser at all. MapViewer builds a style twice -- at init, and in an effect keyed on `[theme, mapReady, scaleUnit]` -- but the theme picker lives on Settings and in the gallery, never in the nav, so there is no way to change the theme with /map open; navigating to Settings unmounts the viewer. A Cypress case that switches themes would have nothing to click. So that branch is guarded in the source instead: both `buildStyle` calls must pass `mapFlavor(theme)` and `mapSprite(theme)`, and none may pass a bare `theme`. Crude, and the honest alternative to a browser test that cannot reach the branch it claims to cover. Planted by breaking ONLY the rebuild call: the source guards go red and the e2e stays green, which is the whole argument for keeping it. The call-site scan counts parens rather than matching a regex. The arguments are themselves calls, so anything non-greedy stops at `mapFlavor(theme)` and reports half an argument list as the whole of it -- which is how the first version of this test failed while the code was correct. 1098 jest / 61 suites, 4 e2e themes, clean build. Co-Authored-By: Claude Opus 5 (1M context) --- app/cypress/e2e/map-themes.cy.js | 28 +++++++++++++-- app/src/themes/map-flavor.test.js | 57 +++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/app/cypress/e2e/map-themes.cy.js b/app/cypress/e2e/map-themes.cy.js index ddccf46f..69780566 100644 --- a/app/cypress/e2e/map-themes.cy.js +++ b/app/cypress/e2e/map-themes.cy.js @@ -1,3 +1,5 @@ +import {mapSprite, themeNames, themeSessionKey} from '../../src/themes/names'; + /* * The map basemap has a "flavor" of its own, and MapViewer used to hand protomaps the WROLPi * theme name. That worked while the only themes were `light` and `dark`, and broke when @@ -9,18 +11,29 @@ * Driven through the real page in every theme, because the unit test can only check that the * mapping returns something valid -- it cannot see whether MapViewer USES it, which is the * half that was broken. + * + * `themeNames` rather than a list written out here, so a fifth theme is cold-started by this + * suite the day it is added rather than the day someone remembers to edit it. */ -const THEMES = ['light', 'dark', 'night', 'amber']; describe('the map viewer builds a basemap in every theme', () => { - THEMES.forEach((theme) => { + themeNames.forEach((theme) => { it(`initialises MapLibre in ${theme}`, () => { const errors = []; + /* + * The sprite is the half no canvas assertion can see: a wrong sprite name 404s + * the icons and MapLibre carries on, so the map still draws and this test would + * still pass. Watching the request is the only way to tell `sprites/dark` from + * `sprites/black` -- and `black` is exactly the name that would be requested if + * the flavor and the sprite were ever collapsed back into one variable. + */ + cy.intercept('GET', '**/map-assets/sprites/**').as('sprite'); + cy.visit('/map', { onBeforeLoad(win) { // Set the theme before the app boots, so the viewer builds its style once // with the theme under test rather than rebuilding after a switch. - win.localStorage.setItem('color-scheme', theme); + win.localStorage.setItem(themeSessionKey, theme); // MapViewer reports a failed init through console.error and then renders // nothing, so the console is where the defect is visible. const original = win.console.error; @@ -34,6 +47,15 @@ describe('the map viewer builds a basemap in every theme', () => { // The canvas only exists once MapLibre constructed a style successfully. cy.get('canvas.maplibregl-canvas', {timeout: 40000}).should('exist'); + cy.wait('@sprite', {timeout: 40000}).then(({request}) => { + const name = request.url + .split('/map-assets/sprites/')[1] + .replace(/[?#].*$/, '') + .replace(/\.(json|png)$/, '') + .replace('@2x', ''); + expect(name, `sprite set requested in ${theme}`).to.equal(mapSprite(theme)); + }); + cy.then(() => { const failures = errors.filter(message => /MapLibre failed to initialize|reading 'background'/.test(message)); diff --git a/app/src/themes/map-flavor.test.js b/app/src/themes/map-flavor.test.js index dc93484b..3de0ce36 100644 --- a/app/src/themes/map-flavor.test.js +++ b/app/src/themes/map-flavor.test.js @@ -101,3 +101,60 @@ describe('the monochrome themes get an achromatic basemap', () => { expect(hued.length).toBeGreaterThan(0); }); }); + +describe('every buildStyle call goes through the mapping', () => { + /* + * MapViewer builds a style in two places: once at init, and again in an effect keyed on + * `[theme, mapReady, scaleUnit]` that calls `map.setStyle` when any of them change. + * + * Only the first is reachable from an e2e. The theme picker lives on Settings and in the + * gallery, never in the nav, so there is no way to change the theme while /map is open -- + * navigating to Settings unmounts the viewer. The rebuild branch is real code that a + * future compact picker would light up, and a regression confined to it (capturing the + * flavor once at init, say) would pass every browser test in this repo. + * + * So it is checked in the source. Crude, and the honest alternative to a browser test + * that cannot reach the branch it claims to cover. + */ + const source = fs.readFileSync( + path.join(__dirname, '../components/MapViewer.js'), 'utf8'); + + /* + * Paren-counting rather than a regex: the arguments are themselves calls, so anything + * non-greedy stops at `mapFlavor(theme)` and reports half an argument list as the whole + * of it -- which is how the first version of this test passed while looking wrong. + */ + const buildStyleCalls = (text) => { + const calls = []; + const marker = 'buildStyle('; + for (let at = text.indexOf(marker); at !== -1; at = text.indexOf(marker, at + 1)) { + let depth = 0; + for (let i = at + marker.length - 1; i < text.length; i++) { + if (text[i] === '(') depth++; + else if (text[i] === ')' && --depth === 0) { + calls.push(text.slice(at, i + 1)); + break; + } + } + } + // The `function buildStyle(...)` declaration matches too; drop it. + return calls.filter(call => !text.includes(`function ${call}`)); + }; + + it('passes mapFlavor and mapSprite at both call sites', () => { + const calls = buildStyleCalls(source); + + // Two call sites plus the declaration; if this drops to one the test below is + // covering half of what it claims. + expect(calls.length).toBe(2); + calls.forEach((call) => { + expect(call).toContain('mapFlavor(theme)'); + expect(call).toContain('mapSprite(theme)'); + }); + }); + + it('never passes a bare theme as the flavor', () => { + // The original bug, written out: `buildStyle(sources, theme, ...)`. + expect(source).not.toMatch(/buildStyle\([^)]*,\s*theme\s*,/); + }); +});