Wait for the download click instead of a fixed timeout in saveCanvas …

[harshiltewari2004]

Resolves #9030

Changes

The four saveCanvas download tests waited on a fixed sleep before asserting:
one 500ms sleep in test/unit/webgl/p5.Framebuffer.js and three 100ms sleeps
in test/unit/image/downloading.js. The sleep was a guess about how long
encoding takes — if it would run long, the assertion would arrive before the anchor
exists and the test fails. That's what the failure in #9030 is: two
createElement('canvas') calls and zero 'a' calls, i.e. the download hadn't
happened yet rather than never.

saveCanvas calls htmlCanvas.toBlob(...) and returns immediately, so there
was nothing to await — hence the sleep. But the test already mocks
document.createElement and hands back a fake anchor, and link.click() is
exactly the moment the download happens. So the mock can signal it instead:

• a module-level onDownload holds the current test's resolve
• mockAnchorElement.click calls it
• downloadHappened() creates the promise and registers the resolve

Each test now subscribes first, calls saveCanvas, and awaits the click. No duration
in the test at all. onDownload is reset in beforeEach.

Locally the framebuffer test does ~51ms of real work against the old 500ms
budget, and the three 2D ones 6/18/15ms against 100ms. So CI was running
roughly 10x slower than local, which is plausible on a shared runner with
software WebGL.

Two things I considered and didn't do:

• Raising the timeout. Any fixed number would still have been a guess, and it costs the
  full duration on every passing run.
• vi.waitFor(). Its default timeout is 5000ms, above this project's
  3000ms testTimeout, so it would need hand-tuning to a duration anyway.

The mock wiring is duplicated across the two files itself, rather than extracted to a
shared helper — with two call sites the duplication seemed the right approach than the
indirection, but happy to pull it out if you'd rather.

Note: this touches test/unit/image/downloading.js, which #9012 also modifies
(different suite in the same file). Happy to rebase whichever lands second.

PR Checklist

• npm run lint passes
• [Inline reference] is included / updated
• [Unit tests] are included / updated

[Ayush4958]

Good work by
Removing arbitrary fixed timeouts and Its nice to duplicate the short mock logic for now as its only needed in 2 files.
If needed on future we can expand it by creating a shared helper function. I had tested this on my end, every test is passed.

LGTM

[harshiltewari2004]

Thanks @Ayush4958 !

[harshiltewari2004]

Hi @ksen0 I am very sorry to tag directly and know that you already told me to ping maintainers when its extremely important or after few weeks have passed, but— the three workflows on this PR and on #9012
have been awaiting maintainer approval since early August, so the required
lint check has never run on either. Would you be able to approve the runs, or
point me to whoever handles that?

This one has an approval already and both test files pass locally. Thanks!

[p5-bot]

Continuous Release

CDN link

• https://raw.esm.sh/pr/p5@dcb206f/lib/p5.min.js

Published Packages

• https://pkg.pr.new/p5@dcb206f

Commit hash: dcb206f

Previous deployments

da7dc59

• https://raw.esm.sh/pr/p5@da7dc59/lib/p5.min.js
• https://pkg.pr.new/p5@da7dc59

---

This is an automated message.

[limzykenneth]

@davepagurek Can you have a look when you can to see if it made sense for you. If all looks good, do go ahead and merge. Thanks.

[davepagurek]

Looks good, thanks!