rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

test: act on review -- the sprite gap, and the branch no e2e can reach

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-08-02 13:42:42 -06:00 • committed by lrnselfreliance
parent 7bf8102e81
commit d375cb4b84
2 changed files with 82 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

28
app/cypress/e2e/map-themes.cy.js
View file

@ -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));

57
app/src/themes/map-flavor.test.js
View file

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