Skip to content

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

Open
harshiltewari2004 wants to merge 1 commit into
processing:mainfrom
harshiltewari2004:fix-flaky-download-tests
Open

Wait for the download click instead of a fixed timeout in saveCanvas …#9031
harshiltewari2004 wants to merge 1 commit into
processing:mainfrom
harshiltewari2004:fix-flaky-download-tests

Conversation

@harshiltewari2004

Copy link
Copy Markdown
Contributor

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

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.

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

1 participant