Wait for map and image loads in tests - #58
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes production map/image load-lifecycle behavior and reworks settled()-dependent test semantics across seven files plus a runtime dependency promotion, which warrants human verification even though no defects were found.
Review effort: Balanced
Findings: None
What changed in this PR
This PR improves test-waiter hygiene so that await render()/await settled() reliably wait for a map (and <map.image> images) to finish loading, removing the many manual waitUntil(() => find(...)) calls scattered across the integration tests. It introduces a small beginWait helper that opens an @ember/test-waiters token with a 10s safety timeout, wires it into the maplibre-gl and maplibre-gl-image components' load/error/destroy lifecycles, and moves @ember/test-waiters from devDependencies to dependencies since it is now imported by shipped source.
Changes:
- Add
src/-private/wait.ts(beginWait) and registermap-load/image-loadwaiters in the map and image components, ending them on load, style failure, construction failure, replacement, or destroy. - Remove now-unnecessary
waitUntilcalls across seven integration test files and add dedicated waiter tests plus a data-URL raster/SVG image test. - Promote
@ember/test-waitersto a runtime dependency and document the testing behavior indocs/components/map.md.
| File | Description |
|---|---|
| src/-private/wait.ts | New idempotent beginWait helper with a 10s timeout safety net. |
| src/components/maplibre-gl.gts | Begin/end map-load waiter across load, style-failure error, WebGL construction failure, and destructor. |
| src/components/maplibre-gl-image.gts | Begin/end image-load waiter for SVG and raster paths, replacement loads, and destructor. |
| package.json | Move @ember/test-waiters from devDependencies to dependencies. |
| pnpm-lock.yaml | Reflect the dependency section move. |
| docs/components/map.md | Add a Testing section describing the waiter behavior. |
| tests/integration/components/maplibre-gl-test.gts | Drop waitUntil; add nested test waiter suite with a ManualMap stub. |
| tests/integration/components/maplibre-gl-image-test.gts | Drop waitUntil, add data-URL image test, and rework stale-load tests around the waiter. |
| tests/integration/components/maplibre-gl-source-test.gts | Remove waitUntil/find import churn now covered by the waiter. |
| tests/integration/components/maplibre-gl-layer-test.gts | Remove waitUntil calls now covered by the waiter. |
| tests/integration/components/maplibre-gl-marker-test.gts | Remove waitUntil (including marker-count polling) now covered by the waiter. |
| tests/integration/components/maplibre-gl-popup-test.gts | Remove waitUntil calls now covered by the waiter. |
| tests/integration/components/maplibre-gl-control-test.gts | Remove waitUntil calls now covered by the waiter. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
82235a7 to
8f3e37d
Compare
8f3e37d to
f95e2e3
Compare
Registers test waiters, so
await render()andawait visit()return after the map has firedload, and after images loaded by<map.image>are on the map. Tests no longer needwaitUntilfor either — 69 such calls are removed from this suite, and 4 are kept where a test deliberately holds a load open.@ember/test-waitersand@embroider/macrosmove from devDependencies to dependencies. The waiters, and the timers below, exist only in test and development builds.A wait ends when:
load, or the image is addederrorarrives before the style has loadedsettled()then continues as if the work had finishedThe last three matter. Without them, one request that never answers holds
settled()open for the rest of the suite. The addon's own tests stubloadImagewith a promise that never resolves, and hung exactly that way while this was built.The waiters cover the map's
loadevent and<map.image>. Sources, layers, markers and popups act on the map while the block renders, soawait render()already covers them too. Anything after that — a popup opened by a marker click,setStyle, tile loading — is still yours to wait for.New tests cover the map load, a style that 404s, a tile error after the style has loaded, destroying a map before it loads, and raster plus SVG images being on the map when
render()returns.Also fixed
A reused map (
@reuseMaps) that was pooled while its tiles were still loading never rendered its block on remount. The code waited forstyle.load, which MapLibre fires only when it parses a style, never again for one it has already parsed. The pool only accepts maps whose style has loaded, so the remount now signals load directly. A test covers it — before this PR the bug was a silent hang, so the new wait would only have turned it into a 10-second one.For apps
@mapLibmust fireload, or each test waits 10 seconds.render()andvisit()wait for it.No component API changes.