Skip to content
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.

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

test(): text dnd #9112

Merged
merged 69 commits into from
Aug 30, 2023
Merged

test(): text dnd #9112

merged 69 commits into from
Aug 30, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jul 20, 2023

Motivation

Testing the formerly untestable drag and drop feature.
After a lot of hours I finally managed to get it to work.
Eraser is next
Playwright is powerful - it was a very good decision

Description

Changes

  • removed e2e/imports.ts in favor of a full importmap produced in runtime
  • made babelrc esm
  • added tests

Gist

In Action

ShaMan123 and others added 24 commits July 19, 2023 13:49
abs resolution + default imports
Revert "Update setupApp.ts"

This reverts commit 5dcd615.
commit 1849a03
Author: ShaMan123 <[email protected]>
Date:   Thu Jul 20 01:15:16 2023 +0530

    Update playwright.config.ts

commit fd4102f
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 23:19:45 2023 +0530

    Update setupApp.ts

    Revert "Update setupApp.ts"

    This reverts commit 5dcd615.

commit ec4ac28
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 22:38:04 2023 +0530

    fix transforming error

commit b7edf37
Merge: 3aaa04b 73fa847
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 22:31:09 2023 +0530

    Merge branch 'master' into ci-e2e-rel-imports

commit 3aaa04b
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 22:23:19 2023 +0530

    cleanup

commit ced8451
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 17:21:54 2023 +0530

    abs resolution + default imports

    abs resolution + default imports

commit 4f7bda2
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 15:08:58 2023 +0530

    cleanup

commit 47b954f
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 14:38:09 2023 +0530

    node version - this was supposed to throw

commit 6ccd23c
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 14:33:35 2023 +0530

    try to fix node14 ci error

    actions/setup-node#214 (comment)

    npm/npm#19788

commit cb31f22
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 14:09:57 2023 +0530

    Update package-lock.json

commit 248d0be
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 13:59:48 2023 +0530

    forgot the setup

commit 19e20d3
Merge: b9c0da6 f09a52e
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 13:54:49 2023 +0530

    Merge branch 'ci-e2e-rel-imports' of https://github.com/fabricjs/fabric.js into ci-e2e-rel-imports

commit b9c0da6
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 13:54:25 2023 +0530

    extends

commit f09a52e
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Wed Jul 19 08:22:08 2023 +0000

    update CHANGELOG.md

commit 20b9eaa
Author: ShaMan123 <[email protected]>
Date:   Wed Jul 19 13:49:28 2023 +0530

    ci(e2e): support relative imports
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

Build Stats

file / KB (diff) bundled minified
fabric 915.042 (0) 305.411 (0)

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

  • removed e2e/imports.ts in favor of a full importmap produced in runtime
  • made babelrc esm

@github-actions
Copy link
Contributor

github-actions bot commented Jul 29, 2023

Coverage after merging test-text-dnd into master will be

82.97%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%118, 129, 138, 31, 94
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.12%77.68%83.05%79.61%1000, 1003–1004, 1004, 1004, 1006, 1014, 1014, 1014–1016, 1016, 1016, 1023–1024, 1032–1033, 1033, 1033–1034, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1517, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 437, 445, 449, 449, 449–450, 452, 535–536, 536, 536–537, 543, 543, 543–545, 565, 567, 567, 567–568, 568, 568, 571, 571, 571–572, 575, 584–585, 587–588, 590, 590–591, 593–594, 606–607, 607, 607–608, 610–615, 621, 628, 665, 665, 665, 667, 669–674, 680, 686, 686, 686–687, 689, 692, 697, 710, 738, 738, 796–797, 797, 797–798, 800, 803–804, 804, 804–805, 807–808, 811, 811–813, 816–817, 887, 899, 906, 927, 959, 980–981, 997–998, 998, 998–999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts94.44%92.28%94.23%96.17%1098, 1100, 1102–1103, 301, 477–478, 480–481, 481, 481, 530–531, 592–593, 646–648, 680, 685–686, 713–714, 898, 898–899, 902, 922, 922, 971, 979
   StaticCanvas.ts96.78%93.09%100%98.53%1030, 1040, 1092–1093, 1096, 1131–1132, 1208, 1217, 1217, 1221, 1221, 1268–1269, 185–186, 202, 570, 582–583, 913–914, 914, 914–915
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts84.31%71.43%91.67%88%17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts100%100%100%100%
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts93.33%87.88%91.67%97.78%175, 240, 327, 327, 362
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts6.06%0%0%11.43%103, 117, 117, 117, 117, 117, 119–122, 122, 125, 132, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts<

Comment on lines 133 to 134
// we need to fix font diff
process.env.CI && test.fail();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

disabled the error for now

Comment on lines +38 to +39
!coverageIgnore.some((ignore) =>
pathTo.startsWith(path.resolve(process.cwd(), ignore))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignoring unnecessary coverage data

@ShaMan123
Copy link
Contributor Author

I think we can merge this and get back to the font inconsistency

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Implemented font caching for perf
I can extract the changes to the test suite to another PR if you want

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

READY
Not touching anymore

@asturur
Copy link
Member

asturur commented Aug 30, 2023

@ShaMan123 i'm merging the dnd test but making it font independent.
For font loading we can't rely on network connections to google fonts. For how much stable they can be we need to limit to local assets. We can make the font local assets and load them from localhost as soon as there is a real necessity.

@asturur asturur merged commit 14a4b7b into master Aug 30, 2023
24 checks passed
@asturur
Copy link
Member

asturur commented Aug 30, 2023

great test

@asturur asturur deleted the test-text-dnd branch August 30, 2023 20:37
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Aug 31, 2023

I wish you not delete setupFonts and setupImports
Why remove setupImports? It is super light weight and makes the test files behave as any module
And for setupFonts, can you leave it there but not import it so we don't need to redo it with a comment or whatever for the future?

@asturur
Copy link
Member

asturur commented Aug 31, 2023

i didn't delete them, are in a backup branch.
test-text-dnd-previous

setupImports was added to add fontObserver? The test work without the change, so the change wasn't needed.
there was no explanation of what setupImports did apart removed e2e/imports.ts in favor of a full importmap produced in runtime is completely unclear why to do that since all test were running fine without.
The moment we need a second import outside fabric in a test we can add it.

setupFonts wasn't good imho It used google fonts and used fontFaceObserver, and i don't want to use either of them.
it had network dependencies on tests, fiddling with google css, network intercepting to make custom chaching. we shouldn't deal with those things. As soon as we need a custom font for a test that is not Arial we can add the font to the repo and load it from localhost using the standard browser api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants