[p5.js 2.0+ Bug Report]: A saveCanvas test is flaky

[davepagurek]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• WebGPU
• p5.strands
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

main branch

Web browser and version

N/A

Operating system

N/A

Steps to reproduce this

Sometimes on PRs, tests will fail with this error:

 FAIL   unit-tests (chrome)  test/unit/webgl/p5.Framebuffer.js > p5.Framebuffer > saveCanvas > should download a png file
AssertionError: expected "createElement" to be called with arguments: [ 'a' ]

Received: 

  1st createElement call:

  [
-   "a",
+   "canvas",
  ]

  2nd createElement call:

  [
-   "a",
+   "canvas",
  ]


Number of calls: 2

 ❯ test/unit/webgl/p5.Framebuffer.js:681:38
    679|       await new Promise(res => setTimeout(res, 500));
    680| 
    681|       expect(document.createElement).toHaveBeenCalledWith('a');
       |                                      ^

    682|       expect(mockAnchorElement.click).toHaveBeenCalledTimes(1);
    683|       assert.equal(mockAnchorElement.download, 'untitled.png');

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed | 67 passed | 4 skipped (72)
      Tests  1 failed | 2032 passed | 12 skipped | 213 todo (2258)
   Start at  17:15:00
   Duration  128.19s (transform 0ms, setup 0ms, import 30.75s, tests 70.80s, environment 0ms)


Error: AssertionError: expected "createElement" to be called with arguments: [ 'a' ]

Received: 

  1st createElement call:

  [
-   "a",
+   "canvas",
  ]

  2nd createElement call:

  [
-   "a",
+   "canvas",
  ]


Number of calls: 2

 ❯ test/unit/webgl/p5.Framebuffer.js:681:38

This isn't a real failure, it's a flaky test. Anyone have ideas on ways we can rewrite it to not be flaky?

[harshiltewari2004]

The failure looks like a timing assumption rather than a flake in the code under test.

p5.Framebuffer.js:679 sleeps for a fixed 500 ms after saveCanvas(fbo) and then asserts on line 681. The received output is two createElement('canvas') calls and zero 'a' calls — which is what you'd see if the anchor hadn't been created yet, not never. On a loaded runner, framebuffer readback plus blob encoding can exceed 500 ms, so the assertion just arrives early.

Raising the timeout would make it rarer but not deterministic — any fixed duration is still a guess about how slow CI can get, and it slows the suite down on every passing run too. I think the better fix is to wait on the actual signal instead of on the clock. The createElement spy at the top of the file already returns mockAnchorElement for 'a', so the test can hold a promise that the mock's click() resolves, and await that instead of a timer. That removes the duration from the test entirely. If that turns out to be awkward, vi.waitFor() on the assertion is a cheaper retrofit — still time-bounded, but it polls rather than assuming, so it's fast when fast and tolerant when slow.

Happy to put up a PR for this if that direction sounds right — I have one other PR open at the moment, so it'd be shortly after that lands.

[harshiltewari2004]

One more thing worth flagging — test/unit/image/downloading.js has three tests in the saveCanvas suite with the same shape (saveCanvas(...) → await wait(100) → assert on the anchor). The WebGL test looks like a copy of the first one with a longer sleep. Those are passing today, but on a 100 ms budget, so they're probably vulnerable to the same thing — 2D encoding is just fast enough to stay under it. Whatever pattern we land on here would apply to those too.

[Vaivaswat2244]

One more thing worth flagging — test/unit/image/downloading.js has three tests in the saveCanvas suite with the same shape (saveCanvas(...) → await wait(100) → assert on the anchor). The WebGL test looks like a copy of the first one with a longer sleep. Those are passing today, but on a 100 ms budget, so they're probably vulnerable to the same thing — 2D encoding is just fast enough to stay under it. Whatever pattern we land on here would apply to those too.

+1, matches what I found too.