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*,/); + }); +});