test(#1356): assert the rendered label, not where identifiers sit in map.js

[TeTeHacko]

Follow-up to the review on #1912, where this assertion cost a round trip. Independent of that PR — this branch is off current master and touches no code path it changes.

The problem

#1356 V3.e, V3.f and V3.g all describe what makeRepeaterLabelIcon produces, but all three assert it by grepping public/map.js. V3.e also bounds the distance between two identifiers:

assert(/MB_GLYPHS\[[^\]]+\][\s\S]{0,200}shortHash|shortHash[\s\S]{0,200}MB_GLYPHS\[/.test(mapSrc),
  'makeRepeaterLabelIcon prepends MB_GLYPHS glyph to the hash text');

Three separate failure modes, all observed:

1. It fails on edits that change nothing. #1912 inserts one variable declaration in that function; the markup is byte-identical and the build went red.

2. It cannot tell code from prose about code. My first attempt at fixing #1912 added a comment explaining the constraint — and the comment mentioned both identifiers, so it satisfied the grep by itself. With that comment present I moved the hash assignment away from the glyph, reintroducing the exact defect, and the test still reported green. A check that a comment can satisfy is worse than one that is merely brittle.

3. It does not assert the thing it is named after. On master the match is not the declaration order at all. It is MB_GLYPHS[...] reaching the later shortHash inside ariaStatus, 212 characters downstream. Whether the glyph is actually prepended to the hash is incidental to whether this passes.

That third point also corrects something I said on #1912, and it corrects it against myself: both the 312 you quoted and the 299 I "corrected" it to are the distance between the two declarations, which is not the distance the regex uses. Measuring the one it does use:

tree	MB_GLYPHS[ → next shortHash	assertion
master	212	pass
#1912 before the fix	277	fail
moving unknownWidth below the glyph	343	fail
moving shortHash below the glyph	54	pass

So moving unknownWidth down does not merely fall short — it makes the gap worse, because it lands between the glyph and ariaStatus. My earlier "233, still 33 over" was the wrong metric on the wrong pair. Apologies; the conclusion happened to hold but the reasoning did not.

What this does

Loads map.js in the same DOM-less vm sandbox test-map-clustering.js already uses, exposes makeRepeaterLabelIcon through the existing window.__meshcoreMapInternals hook, and asserts the emitted markup:

• glyph, U+2009 thin space, hash — in that order and adjacent;
• no glyph and no thin space when there is no multi-byte status;
• aria-label exactly multi-byte <status>, hash <ID>, and repeater hash <ID> without one;
• the visible span carries aria-hidden.

No browser, so it stays in the JS-unit-tests step rather than moving to Playwright.

Mutation-tested, not eyeballed

mutation	old V3.e/f/g	new
glyph moved after the hash	all silent	caught
plain space instead of U+2009	all silent	caught
span loses aria-hidden	V3.g caught	caught
aria-label loses its comma	all silent	caught
200 chars inserted between the identifiers (no behaviour change)	V3.e fails	passes

Full JS unit list from .github/workflows/deploy.yml: 65/65.

Notes for review

• V3.a–V3.d (MB_GLYPHS definitions, CSS variables, the border rule) are left as source/CSS greps. The glyph values are now covered implicitly by the rendered-output assertions, but converting the CSS ones needs a different approach and did not belong here.
• The sandbox loader has no try/catch that warns and continues. If map.js stops loading, the suite must fail rather than silently skip every assertion below it.
• If this lands, the ordering comment in fix(map): render an unobserved hash size as unknown, not as 1 byte #1912 becomes obsolete and I will drop it there. I deliberately did not touch it from this branch so the two do not conflict textually.

[efiten]

Reviewing this properly shortly; flagging the CI result first so you are not left guessing.

It failed on something unrelated to what this PR changes:

✗ setting persists across reload: multibyte toggle should restore checked=true from localStorage

That is a customizer persistence check, not one of the three a11y assertions you are replacing. Two things worth knowing before you go looking:

1. This repo has a documented flake family around focus and reload timing in headless Chromium (bug(slide-over): focus-restore to originating row is CI-flaky on X-click and resize-cleanup paths #1172, hard-fix: slide-over close path must not transition through focused-but-hidden state (supersedes #1172) #1616), and a localStorage-restore-after-reload assertion is the shape those take. feat(packets): add filter to All Observer dropdown #1884 failed on a swatch-focus test this morning and passed on the rerun with no change.
2. Master itself was red for part of this morning from an unrelated two-PR interaction (fix(#1784): ship pathTrust default 1, not 2 #1929 plus feat(#1784): gate ingestor neighbor-edge creation on the path-trust threshold (rebase of #1863) #1930, fixed in fix: unbreak master — decouple the pathTrust builder test from the default #1932), so runs started in that window tested a broken base. Yours started at 09:31, which is around when the fix landed, so it may or may not have picked it up.

Worth one rerun before treating it as real. If it fails the same way twice, it is worth a look and I will help chase it.

On the PR itself: replacing a source grep with a check on what the function renders is the right direction, and the mutation table showing what each version catches is exactly the evidence I would want. Full review to follow.

[efiten]

Recycling both of your PRs, and it is worth saying why, because it is a trap that has cost this queue a lot of time today.

Both of your runs are attempt=2, so they were re-run rather than recycled. A re-run does not help here. GitHub re-runs against the same commit the original run used, which for a pull_request event is the merge commit as it stood when that run was first created. It never sees a newer master.

That fully explains #1912. Its re-run at 11:44 still tests a base from before #1932, which is the fix for the master breakage I caused this morning by merging #1929 and #1930 together. So it keeps failing on TestNeighborEdgesBuilderPathTrustExcludesOneByte, which has nothing to do with your change and is already fixed on master.

For #1933 it is less clear-cut. Its run started at 09:31, which is the same minute #1932 merged, so its base may or may not have included the fix. The failure there was setting persists across reload: multibyte toggle should restore checked=true from localStorage, in test-live-multibyte-only-e2e.js.

I said I would look properly if it failed twice rather than send you chasing it, so: I read your public/map.js change and it cannot cause that failure. It adds makeRepeaterLabelIcon to the existing window.__meshcoreMapInternals object and reformats the literal across lines. Nothing in it touches the live view, the multibyte toggle or localStorage. Since it has only genuinely run once, a real fresh run is the right next step before anyone goes digging.

Closing and reopening both, which forces a new merge ref against current master. Nothing needed from you.

[efiten]

Settled, and in your favour: your PR was not the cause.

I re-ran the master pipeline for 859173f1 a few minutes ago, with none of your code in it. It failed on:

✗ setting persists across reload: multibyte toggle should restore checked=true from localStorage

The same assertion that failed on your PR, on plain master. That is as clean a control as we are going to get.

I said earlier that I had read your public/map.js change and could not see how adding makeRepeaterLabelIcon to __meshcoreMapInternals could touch a live-view localStorage toggle. This confirms it.

The cause is in the test: it waits for the toggle element to exist and then reads .checked, but the element existing does not mean the code that restores it from localStorage has run yet. Fixed in #1939 along with a second step of the same shape in the colour-picker suite.

Your #1933 stands on its own merits and I still like it: replacing a source grep with a check on what the function renders is the right direction, and it is what would have spared us both the 312-versus-277-character detour this morning.

[efiten]

Merging. I reproduced both halves of the claim rather than reading the diff, because "this test is fragile" and "this test is worthless" are different verdicts and only the first one justifies the change.

The false positive is real, and it is one assertion. On master I inserted a comment between MB_GLYPHS[...] and shortHash inside makeRepeaterLabelIcon, changing nothing the function emits:

insert	master	with this PR
none	PASS	PASS
100 chars	PASS	PASS
300 chars	FAIL	PASS

The failure is ✗ makeRepeaterLabelIcon prepends MB_GLYPHS glyph to the hash text, V3.e, and nothing else. So the gate is a 200-character proximity window on the source text: I measured 44 characters currently between the two identifiers, which leaves 156 characters of headroom before an unrelated edit trips it. That headroom is not written down anywhere, which is what makes it a trap.

The teeth survive. The thing I actually wanted to know is whether asserting on rendered output loses coverage. It does not. On the merged tree:

mutation	result
break the MB_GLYPHS lookup	FAIL, 3 assertions
drop aria-hidden="true"	FAIL, 2 assertions
add an unused local at the top of the function	PASS

So the new assertions catch more per real regression than the single grep did, and stop firing on edits that change no output. That is the whole trade and it lands on the right side of it.

Against the master that now includes #1912, which touched the same test file: the merge is textually clean and the full file goes 44 passed, 0 failed. I ran it on the merged tree, not on your branch, since your branch predates that merge.

Exposing makeRepeaterLabelIcon on __meshcoreMapInternals is the right seam. It already carries createClusterGroup and makeClusterIcon for the same reason, so this is not a new escape hatch, and the comment saying why it is exposed will keep the next person from deleting it as dead.

Worth saying plainly: this PR exists because our review process on #1912 cost you a round trip over an assertion that was never testing the thing it claimed to test. Fixing the gate instead of routing around it was the right response, and the one that took more work.