Skip to content

refactor(tests): move path tests from qunit to vitest #10552

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 3 commits into from
Apr 20, 2025
Merged

refactor(tests): move path tests from qunit to vitest #10552

merged 3 commits into from
Apr 20, 2025

Conversation

Smrtnyk
Copy link
Contributor

@Smrtnyk Smrtnyk commented Apr 18, 2025

No description provided.

@codesandbox CodeSandbox
Copy link

codesandbox bot commented Apr 18, 2025

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

Smrtnyk
Smrtnyk commented Apr 18, 2025
View reviewed changes
src/shapes/Path.spec.ts

expect(path, 'should be instance of Path').toBeInstanceOf(Path);

// @ts-expect-error -- TODO: check why transformMatrix is not set on the Path as potential type?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asturur should transformMatrix be set as nullable property of the path?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the type should be fixed.
fromElement returns the FabricObject with attached that property that then we remove one line of code after.
Each 'FromElement' should call 'removeTransformMatrix' but i think to avoid duplicated calls it was done in a single place in the parsing logic.

@github-actions GitHub Actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 913.863 (0) 301.232 (0)

@github-actions GitHub Actions
Copy link
Contributor

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


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


=============================== Coverage summary ===============================
Statements   : 75.75% ( 21771/28739 )
Branches     : 61.73% ( 4239/6867 )
Functions    : 86.8% ( 1566/1804 )
Lines        : 80.45% ( 16666/20715 )
================================================================================

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 20, 2025 •
edited
Loading

I have canvas events in my local branch
In case you will want to migrate more to not do double work
There is something off in one test happening there and I need to check if it is just jsdom or it also happens in chromium

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Apr 20, 2025

I would also take webgl test if you dont mind

@asturur
Copy link
Member

asturur commented Apr 20, 2025

I fixed the types for FromElement that indeed could return the transformMatrix.

@asturur
Copy link
Member

asturur commented Apr 20, 2025

I would also take webgl test if you dont mind

Take all you want i have a stack of things to do, i do not need to do more tests. I did a bunch for sharing more than other

@asturur asturur merged commit e13d9d3 into fabricjs:master Apr 20, 2025
@Smrtnyk Smrtnyk deleted the path branch April 20, 2025 12:45
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