Skip to content

fix(tests): WebGL save/restore assertions for the actual 4x4 matrix layout (#1481)#1483

Merged
obiot merged 1 commit into
masterfrom
fix/1481-renderstate-transform-bug
Jun 1, 2026
Merged

fix(tests): WebGL save/restore assertions for the actual 4x4 matrix layout (#1481)#1483
obiot merged 1 commit into
masterfrom
fix/1481-renderstate-transform-bug

Conversation

@obiot
Copy link
Copy Markdown
Member

@obiot obiot commented Jun 1, 2026

Closes #1481.

Summary

Three tests in `webgl_save_restore.spec.js` were broken since they were committed in `af34dd341` (Mar 2026) — they hardcoded matrix-slot indices that assumed `Matrix3d.val` was a 3×3 column-major matrix. It's actually 4×4 / 16 floats. The save/restore mechanism in `RenderState` is correct; the tests' assertions were just expressing the wrong shape.

For a 4×4 identity:

  • `val[0] = val[5] = val[10] = val[15] = 1`, everything else 0

The failing tests asserted `val[4] = 1` — which would be correct for a 3×3 identity but is 0 for a 4×4.

Why this slipped through CI for months

The spec used the silent-skip anti-pattern:

```js
it("name", () => {
if (!isWebGL) {
return; // ← registers as PASSED, not skipped
}
// assertions...
});
```

Combined with `failIfMajorPerformanceCaveat: true` in the spec's `beforeAll`, chromium headless without GPU flags fails to create a WebGL context → falls back to Canvas → `isWebGL = false` → every test returns immediately → reporter shows "13 passed".

The broken assertions never ran a single time in CI. Three years of green ticks for tests that weren't actually testing anything.

If the author had used `ctx.skip(...)` the reporter would have shown "13 skipped" and someone would have asked "why are our WebGL tests skipped?" months ago.

Changes

  1. The three broken matrix-index assertions → `expect(renderer.currentTransform.isIdentity()).toBe(true)`. Layout-agnostic, says what we actually mean.
  2. All 13 `if (!isWebGL) return` → `requireWebGL(ctx)` helper that calls `ctx.skip("WebGL not available")`. Future skips are visible in the reporter.
  3. `failIfMajorPerformanceCaveat` flipped from `true` to `false` in `beforeAll` so local devs / GPU-backed runners actually exercise these tests under software WebGL instead of falling back to Canvas.

Verification

  • Default headless chromium env: `3969 passed / 13 skipped / 0 failed` — the 13 `webgl_save_restore` tests now correctly show as skipped instead of falsely passing.
  • Under SwiftShader (with the launch flags from the post-mortem): `13 / 0 / 0` for this spec, save/restore works correctly.

Follow-up (separate from this PR)

`webgl_pipeline_adversarial.spec.js` has the same silent-skip anti-pattern at 22 sites. Worth a similar pass — but the fuzz test in that spec needs separate stabilization work under software WebGL, so it's intentionally not bundled here.

🤖 Generated with Claude Code

…ayout (#1481)

Three tests in `webgl_save_restore.spec.js` had been hardcoding indices
that assumed `Matrix3d.val` was a 3x3 column-major matrix (val[4] = 1
for identity). It's actually a 4x4 column-major matrix — 16 floats —
and the 4x4 identity has 1s at val[0], val[5], val[10], val[15], not
val[0] and val[4]. The save/restore mechanism in RenderState was
correct all along; the assertions were just expressing the wrong
shape.

Replaced the three sites with
`expect(renderer.currentTransform.isIdentity()).toBe(true)` — layout-
agnostic and says what we actually mean.

Also converted all 13 `if (!isWebGL) { return; }` skips to
`requireWebGL(ctx)` → `ctx.skip("WebGL not available")`. The bare
`return` registered as `passed` in the reporter — combined with
`failIfMajorPerformanceCaveat: true` in the spec setup, chromium
headless without GPU flags consistently fell back to Canvas, isWebGL
became false, every test returned immediately, the reporter showed
"13 passed". The matrix-index assertions had been broken since
commit `af34dd341` (Mar 2026) and the silent skips hid that from
CI for months. `ctx.skip()` surfaces "13 skipped" in the reporter,
so a future regression of the same shape would actually be visible.

`failIfMajorPerformanceCaveat` flipped from `true` to `false` in the
`beforeAll` so local devs / GPU-backed runners actually run these tests
under software WebGL (chromium's SwiftShader backend) instead of falling
back to Canvas and skipping silently.

3969 / 13 skipped / 0 failed in the default chromium headless env (the
13 webgl_save_restore tests now correctly show as skipped). 13 / 0 /
0 under SwiftShader (verified locally with the launch flags from the
post-mortem).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 1, 2026 02:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrectly-written WebGL save/restore tests by removing brittle assumptions about Matrix3d.val’s internal layout (it’s a 4×4 / 16-float matrix) and by ensuring WebGL-dependent tests report as skipped (not falsely “passed”) when WebGL isn’t available in the test environment.

Changes:

  • Replace hardcoded matrix-slot assertions with renderer.currentTransform.isIdentity() in the three affected tests.
  • Replace the “silent-pass” if (!isWebGL) return; pattern with a requireWebGL(ctx) helper that calls ctx.skip(...).
  • Set failIfMajorPerformanceCaveat: false during test initialization to better allow software WebGL contexts in constrained environments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@obiot obiot merged commit b66e95d into master Jun 1, 2026
7 checks passed
@obiot obiot deleted the fix/1481-renderstate-transform-bug branch June 1, 2026 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebGL save/restore: transform not preserved across save/restore (3 tests fail when WebGL2 runs)

2 participants