Skip to content

Commit a1837b7

Browse files
obiotclaude
andcommitted
test: correct the harness docs, and stop one flake misreporting its cause
Three documentation claims in the test harness were wrong, and the wrong one was load-bearing: it produced a diagnosis of the intermittent CI failure that does not hold. - `tests/helpers/webgl-context.js` said vitest runs every spec file in one page, so WebGL contexts accumulate across files until the browser's cap force-loses the oldest and a later `beforeAll` stalls. Measurably false: a probe shows a global set in one file is `undefined` in the next, and a context opened in one is already lost by the next. Vitest isolates each spec FILE. Contexts accumulate only WITHIN a file, across describe blocks. - `vitest.config.ts` repeated the same claim. - `src/system/device.ts` carried a JSDoc `@example` calling `me.video.init()` — an API that no longer exists — on an already-deprecated function. That one is user-facing, since it ships in the published docs. All three now say what was measured, and the helper carries an explicit note not to rebuild the starvation theory from it. The helper itself is still worth using, for a different reason: acquisition through a software rasterizer is genuinely slow, so creating fewer contexts saves real time. `application_lifecycle` is the one spec that has failed intermittently here. It built SIX WebGL applications in a loop — the most context churn in the suite — and checked handler identity before checking a handler exists, so a cycle that failed to obtain a renderer reported "reused a previous handler". That message sent this investigation the wrong way. Three cycles prove the property just as well, and the existence check now runs first, so a failure says what actually happened. The cause of the CI flake remains unknown. The `hookTimeout` note — that acquisition can take tens of seconds under load — is the better lead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
1 parent 9f7b373 commit a1837b7

4 files changed

Lines changed: 51 additions & 58 deletions

File tree

packages/melonjs/src/system/device.ts

Lines changed: 3 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -313,43 +313,9 @@ export function setAutoFocus(enable: boolean): void {
313313
* specify a function to execute when the Device is fully loaded and ready
314314
* @param fn - the function to be executed
315315
* @example
316-
* // small game skeleton
317-
* let game = {
318-
* // called by the me.device.onReady function
319-
* onload = function () {
320-
* // init video
321-
* if (!me.video.init('screen', 640, 480, true)) {
322-
* alert("Sorry but your browser does not support html 5 canvas.");
323-
* return;
324-
* }
325-
*
326-
* // initialize the "audio"
327-
* me.audio.init("mp3,ogg");
328-
*
329-
* // set callback for resources loaded event
330-
* me.loader.onload = this.loaded.bind(this);
331-
*
332-
* // set all resources to be loaded
333-
* me.loader.preload(game.assets);
334-
*
335-
* // load everything & display a loading screen
336-
* me.state.change(me.state.LOADING);
337-
* };
338-
*
339-
* // callback when everything is loaded
340-
* loaded = function () {
341-
* // define stuff
342-
* // ....
343-
*
344-
* // change to the menu screen
345-
* me.state.change(me.state.PLAY);
346-
* }
347-
* }; // game
348-
*
349-
* // "bootstrap"
350-
* me.device.onReady(function () {
351-
* game.onload();
352-
* });
316+
* // the modern equivalent is simply to await the Application
317+
* const app = new Application(640, 480, { parent: "screen" });
318+
* await app.init();
353319
* @deprecated since 18.3.0 — no longer needed when using {@link Application} as entry point.
354320
* @category Application
355321
*/

packages/melonjs/tests/application_lifecycle.spec.js

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,27 @@ describe("Application lifecycle: renderer event handlers are unregisterable", ()
128128
// Guards the drift case: were the handler shared on the prototype,
129129
// cycle N would unregister cycle 0's closure and leave every later
130130
// renderer on the bus.
131+
// Three cycles prove the property (a prototype-shared handler collides
132+
// on the second); six only tripled the WebGL contexts this one test
133+
// builds and tore down, which is the most context churn in the suite.
134+
const CYCLES = 3;
131135
const seen = new Set();
132-
for (let i = 0; i < 6; i++) {
136+
for (let i = 0; i < CYCLES; i++) {
133137
const app = await mk(video.WEBGL);
134138
const handler = app.renderer.onGameReset;
139+
// Check this FIRST. A cycle that failed to obtain a real WebGL
140+
// renderer yields no handler, and two `undefined`s land in the set
141+
// as a duplicate — reporting "reused a previous handler" for what is
142+
// actually a context-acquisition failure. This test has failed
143+
// intermittently in CI and that message sent the investigation the
144+
// wrong way; it should say what really happened.
145+
expect(typeof handler, `cycle ${i} produced no handler`).toBe("function");
135146
expect(seen.has(handler), `cycle ${i} reused a previous handler`).toBe(
136147
false,
137148
);
138149
seen.add(handler);
139150
app.destroy();
140151
}
141-
expect(seen.size).toBe(6);
152+
expect(seen.size).toBe(CYCLES);
142153
});
143154
});

packages/melonjs/tests/helpers/webgl-context.js

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,29 @@ import WebGLRenderer from "../../src/video/webgl/webgl_renderer.js";
66
*
77
* ## Why this exists
88
*
9-
* Vitest browser mode runs every spec file in one page, and constructing an
10-
* `Application` and awaiting `init()` builds a fresh canvas and GL context
11-
* each time. With a spec
12-
* file per feature that adds up to dozens of live contexts in a single
13-
* session — and browsers cap how many they will keep, force-losing the oldest
14-
* once past the limit. Past that point, creating another context stalls.
15-
*
16-
* The failure that produces is badly misleading: some *unrelated* spec's
17-
* `beforeAll` times out, the suite blames whichever file happened to be late
18-
* in the run, and adding or reordering files moves the casualty around. It
19-
* looks like flakiness in the victim, but it is a resource limit set by
20-
* everything before it.
21-
*
22-
* So: create one context, keep it for the session, and let specs borrow it.
9+
* Constructing an `Application` and awaiting `init()` builds a fresh canvas
10+
* and GL context each time, and on a CI container with no GPU that runs
11+
* through a software rasterizer — slow enough that `vitest.config.ts` raises
12+
* `hookTimeout` to 90s for it. Creating FEWER contexts is therefore worth
13+
* real time, which is what this helper is for.
14+
*
15+
* ## What this is NOT for
16+
*
17+
* This used to claim that vitest runs every spec file in one page, so contexts
18+
* accumulated across files until the browser's cap force-lost the oldest and a
19+
* later `beforeAll` stalled. That is measurably false: vitest isolates each
20+
* spec FILE. A probe confirmed it — a global set in one file is `undefined` in
21+
* the next, and a context opened in one is already lost by the next. Contexts
22+
* accumulate only WITHIN a file, across its describe blocks.
23+
*
24+
* The belief was load-bearing for a while: an intermittent CI failure (an
25+
* unrelated spec's `beforeAll` timing out in `getWebGLRenderer`) was blamed on
26+
* cross-file starvation. It is not that. The cause is still unknown, and the
27+
* `hookTimeout` note above — that acquisition is genuinely slow under load —
28+
* is the better lead. Do not rebuild the starvation theory from this file.
29+
*
30+
* So: create one context per file, keep it for that file, and let its specs
31+
* borrow it.
2332
*
2433
* ## Using it
2534
*

packages/melonjs/vitest.config.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,18 +25,25 @@ export default defineConfig(() =>
2525
// Anchor to this package. `pnpm test` invokes this config from the repo
2626
// root, where an unanchored `include` globs every workspace package —
2727
// so the adapters' and debug-plugin's specs were pulled into this run
28-
// *as well as* being run by their own `pnpm -F ... test` jobs. Besides
29-
// the duplicate work, each extra spec file that calls `video.init`
30-
// creates another WebGL context in the one shared browser session, and
31-
// past the browser's context cap a later `beforeAll` stalls.
28+
// *as well as* being run by their own `pnpm -F ... test` jobs, doubling
29+
// the work.
30+
//
31+
// This comment used to claim the specs also shared one browser session,
32+
// so contexts accumulated across files until the browser's cap stalled a
33+
// later `beforeAll`. That is NOT true here: vitest isolates each spec
34+
// file, verified by a probe — a global set in one file is undefined in
35+
// the next, and a context opened in one is dead by the next. Contexts do
36+
// accumulate WITHIN a file across its describe blocks, which is worth
37+
// releasing, but it is not a cross-file effect.
3238
root: __dirname,
3339
test: {
3440
include: ["tests/**/*.{test,spec}.[jt]s?(x)"],
3541
// Several specs create a WebGL context in `beforeAll`. On a CI
3642
// container with no GPU that runs through a software rasterizer and
3743
// can genuinely take tens of seconds under load, so the default hook
3844
// timeout fails correct suites. Raised for headroom — this is a slow
39-
// hook, not a hanging one.
45+
// hook, not a hanging one. (`video.init` no longer exists; a context
46+
// comes from `new Application(...)` + `await app.init()`.)
4047
hookTimeout: 90000,
4148
browser: {
4249
enabled: true,

0 commit comments

Comments
 (0)