Skip to content

refactor(tests): move StaticCanvas tests from qunit to vitest #10521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 14, 2025
Merged

refactor(tests): move StaticCanvas tests from qunit to vitest #10521

merged 4 commits into from
Apr 14, 2025

Conversation

Smrtnyk
Copy link
Contributor

@Smrtnyk Smrtnyk commented Apr 13, 2025 •
edited
Loading

This moves StaticCanvas unit tests from qunit to vitest

I managed to figure out why jsdom wasn't loading images and now all tests pass in vitest as they were in qunit
There are no skipped tests
Also I removed mocking for image loading from other tests now that we can properly load images in jsdom

StaticCanvas took most time so far to migrate
other ones seem smaller and easier to do
I am glad I got this one out of the way

@codesandbox CodeSandbox
Copy link

codesandbox bot commented Apr 13, 2025

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

Smrtnyk
Smrtnyk commented Apr 13, 2025
View reviewed changes
src/canvas/Canvas.spec.ts
@@ -20,16 +20,6 @@ import {
version,
} from '../../fabric';

vi.mock('../util/misc/objectEnlive', async (importOriginal) => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed because images load just fine now

@github-actions GitHub Actions
Copy link
Contributor

It seems that you opened a PR but you didn't add a line to the changelog file
Please under the ## [next] version tag, add a line describing the changes of your pr with this format:
ci/chore/tests/feature/fix(): Some extended description #10521
Example:

  refactor(tests): move StaticCanvas tests from qunit to vitest [#10521](https://github.com/fabricjs/fabric.js/pull/10521)

Smrtnyk
Smrtnyk commented Apr 13, 2025
View reviewed changes
vitest.config.ts
@@ -18,6 +25,12 @@ export default defineConfig({
'extensions/**/*.spec.{ts,tsx}',
'extensions/**/*.test.{ts,tsx}',
],
environmentOptions: {
jsdom: {
resources: 'usable',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the key change to tell jsdom to load images
and onload handlers for image element work fine now

Smrtnyk
Smrtnyk commented Apr 13, 2025
View reviewed changes
vitest.extend.ts
@@ -32,6 +33,30 @@ export const roundSnapshotOptions = {
};

expect.extend({
toSameImageObject(actual: FabricImage, expected: FabricImage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this matcher is a port from qunit

@Smrtnyk Smrtnyk mentioned this pull request Apr 13, 2025
Merged
@Smrtnyk Smrtnyk force-pushed the refactor(tests)-move-static-canvas-tests-from-qunit-to-vitest branch from 8c2d543 to f3e64b1 Compare April 13, 2025 19:53
@asturur
Copy link
Member

asturur commented Apr 14, 2025

Let me merge this so we avoid conflicts with the other one

@github-actions GitHub Actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 917.033 (0) 301.846 (0)

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 14, 2025 •
edited
Loading

hm, why is it failing in CI
its showing some kind of unhandled errors

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 14, 2025

ah maybe because of this
https://github.com/fabricjs/fabric.js/pull/10521/files#diff-2ee894bf23aa44ff4ce12a3da8af19ab20180474ebf2176dbe5f2eea3f96dc92R6

i am using node 22 locally
maybe it doesn't work this api on node 20 pathToFileURL

@asturur
Copy link
Member

asturur commented Apr 14, 2025

hm, why is it failing in CI its showing some kind of unhandled errors

i would factor out jsdom usable and restore the image mock, and reiterate later on these

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 14, 2025

hm, why is it failing in CI its showing some kind of unhandled errors

i would factor out jsdom usable and restore the image mock, and reiterate later on these

I am currently at work
will be able to do it around 16h
feel free to merge other PRs
I will manage conflicts here later then

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 14, 2025

I don't think current PR is a big deal to solve conflicts

#10519 should be mergeable
since that one will have conflict due to util exports snapshot and some util tests

@asturur
Copy link
Member

asturur commented Apr 14, 2025

hm, why is it failing in CI its showing some kind of unhandled errors

i would factor out jsdom usable and restore the image mock, and reiterate later on these

I am currently at work will be able to do it around 16h feel free to merge other PRs I will manage conflicts here later then

There is no rush.

@asturur
Copy link
Member

asturur commented Apr 14, 2025

On my local it works fine both normally and when collecting coverage. node20 arm64

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 14, 2025

Hm is it working now in CI?
Maybe it was some CI hiccup

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 14, 2025 •
edited
Loading 


> fabric@6.6.2 coverage:report:ci
> nyc report --reporter=text-summary


=============================== Coverage summary ===============================
Statements   : 71.85% ( 20698/28806 )
Branches     : 59.99% ( 3822/6371 )
Functions    : 87.09% ( 1573/1806 )
Lines        : 76.59% ( 15901/20760 )
================================================================================

@asturur
Copy link
Member

asturur commented Apr 14, 2025

now it passes in CI too, consistently

@asturur asturur merged commit 700d443 into fabricjs:master Apr 14, 2025
21 checks passed
@Smrtnyk Smrtnyk deleted the refactor(tests)-move-static-canvas-tests-from-qunit-to-vitest branch April 14, 2025 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur asturur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Smrtnyk @asturur