Wait for the download click instead of a fixed timeout in saveCanvas … - #9031
Open
harshiltewari2004 wants to merge 1 commit into
Open
Wait for the download click instead of a fixed timeout in saveCanvas …#9031harshiltewari2004 wants to merge 1 commit into
harshiltewari2004 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #9030
Changes
The four
saveCanvasdownload tests waited on a fixed sleep before asserting:one 500ms sleep in
test/unit/webgl/p5.Framebuffer.jsand three 100ms sleepsin
test/unit/image/downloading.js. The sleep was a guess about how longencoding 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'thappened yet rather than never.
saveCanvascallshtmlCanvas.toBlob(...)and returns immediately, so therewas nothing to await — hence the sleep. But the test already mocks
document.createElementand hands back a fake anchor, andlink.click()isexactly the moment the download happens. So the mock can signal it instead:
onDownloadholds the current test'sresolvemockAnchorElement.clickcalls itdownloadHappened()creates the promise and registers the resolveEach test now subscribes first, calls
saveCanvas, and awaits the click. No durationin the test at all.
onDownloadis reset inbeforeEach.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:
full duration on every passing run.
vi.waitFor(). Its default timeout is 5000ms, above this project's3000ms
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 lintpasses