ci() Introducing JEST and basic tests #8919

asturur · 2023-05-14T15:45:26Z

Motivation

Today Qunit it is what it is and in order to move to testing functions rather than the whole fabricJS package, we need to refactor our test files.

Is probably possible to better undertand QUnit and just import the specific file we want to test and run scoped tests, but at work we mostly use JEST and we are probably more productive with it.

Now this brings in patterns and decisions and opinions.

I opened this PR with the same pattern i use every day at work

test file located with the code file
files named .test.ts or .spec.ts
simple unit tests when possible
using babel as transpiler compared to node-ts

i followed the jest tutorial to get this setup runnig.

My proposal is to:

new tests are just written in JEST
tests run only in node
priority is given to unit tests that do not need to use the canvas, but i configured jest with jest-JSDOM anyway.
tests that requires rendering and browser check will still run in QUNIT and probably are only the visual test suite
find a way to combine the 2 coverages reports ( or deactivate the one from jest and cover everythin with nyc )
phase out the old unit test suite slowly
remember the name of that Test suite that is fully JEST compatible but way faster and use that instead

PROS:

everyone knows JEST
this is a plain vanilla setup, no overhead
a way like another to migrate way from QUNIT

CONS:

JEST gets slow

github-actions · 2023-05-14T15:47:41Z

Build Stats

file / KB (diff)	bundled	minified
fabric	921.566 (0)	303.056 (0)

asturur · 2023-05-14T15:52:14Z

This is here for discussion and best practices.
I know Jest get slow fast, and there is an alternative that uses esbuild i would like to check on that, or verify what es-build can do instead of babel.

asturur · 2023-05-14T16:34:36Z

Ok this is up for discussion.
I m not attached to it but i would like something like this with a pretty much vanilla setup for ts and node.
It doesn matter is Jest as long as i don't need a new runner and as long as the api is popular and estabilished ( no new shills )

asturur · 2023-05-14T16:36:54Z

Well coverage didn't work. i suppose runInBand? no just doesn't work.

asturur · 2023-05-15T08:00:27Z

i don't know on my local works, coverage bumbs to 83.92 while on github action something is off.
I m picking up coverage in a temp dire and then merging it in the old folder nyc_ouput.

Is not super important before evaluating the approach anyway

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	83.92	74.06	84.44	83.76
fabric.js	14.28	100	0	14.28
index.node.ts	14.28	100	0	14.28	17-35
fabric.js/src	96.46	80.57	92.62	97.14
ClassRegistry.ts	100	100	100	100

ShaMan123

I am not for jest because it is falling behind in esm support.
I am worried it will reach its course because of this issue.
IMO this is a no go for us.

Slow when analyzing what to run?

Can you, for the sake of discussion think of a need to mock other modules in fabric for some test? Because that is one of the thigs jest will not be able to do if I understand correctly. Or does babel fix that?
At least we should have a test that mocks some class methods w/o using mocks and not mutatuion.

ShaMan123 · 2023-05-15T14:30:28Z

src/brushes/CircleBrush.test.ts

This is very nice

asturur · 2023-05-16T08:24:27Z

So our worry is that we can't mock internal fabricjs files because of some kind of missing support from jest?
Or external modules?

Which would be a better unit test framework then?
they suggested me this: https://vitest.dev/ but when i have see 0.31 i wasn't really happy to adopt it.

ShaMan123 · 2023-05-16T23:43:11Z

I heard of vitest this week (asked people at work since they use jest heavily with mocking etc.)
Have no idea of its worth.
I can't find from the back of my mind a test that needs/uses stubbing. Might be since it wasn't an option so we did other stuff instead.
Looked around a bit. This list is not what you asked for unfortunately.

A simple test: test/unit/cache.js mocking importing cache
Mocking a method on an instance:

fabric.js/test/unit/canvas_static.js

Line 443 in 9e856bc

canvas.requestRenderAll = countRenderAll;

Spying a class method (search for sinon in the tests):

fabric.js/test/visual/text.js

Lines 616 to 623 in 9e856bc

    
           // sinon could have spied this w/o effort and in one line 
        
           class TestTextbox extends fabric.Textbox { 
        
             __calledInitDimensions = 0; 
        
             initDimensions() { 
        
               super.initDimensions(); 
        
               this.initialized && this.__calledInitDimensions++; 
        
             } 
        
           }

Mocking an object for a specific purpose (svg output):

fabric.js/test/unit/canvas_static.js

Lines 130 to 132 in 9e856bc

    
           function createImageStub() { 
        
             return new fabric.Image(_createImageElement(), { width: 0, height: 0 }); 
        
           }

test/unit/env.js
Stubbing canvas:

fabric.js/test/unit/object_interactivity.js

Lines 207 to 209 in 9e856bc

cObj.canvas = {

_activeObject: cObj

};
In the visual cb so not a good candidate for jest to test:

fabric.js/test/lib/visualTestLoop.js

Line 137 in 9e856bc

fabric.StaticCanvas.prototype.requestRenderAll = fabric.StaticCanvas.prototype.renderAll;

ShaMan123 · 2023-05-16T23:49:20Z

So maybe it is non blocking...

ShaMan123 · 2023-05-16T23:52:08Z

could you (or should I) make #8738 pick these tests up as well and see it works.
I understand that you want to create a separation between pure logical stuff to visual/interaction stuff but for the sake of the POC and in case we decide otherwise.

ShaMan123 · 2023-05-16T23:53:17Z

importing and snapshots in tests is extremely exciting

ShaMan123 · 2023-05-17T00:00:05Z

Also I would us to come up with a convention for visual test etc.
Should they remain in different files? Since now we can use imports they don't have to. However for test separation they should. But it can be nice to have logical, visual and interaction tests related to the same subject being in the same folder/file.
Can expose a global describe.visual and/or test.visual instead of how visual tests are written today? Should it be a simple exported test util?
Should we have a different ext name? e.g. .visual.test.js?
Should we write tests in TS? Will it have any value for type checking? I think we can write in TS but NOT enforce it so we stay sane. What does that mean regarding type tests?

asturur · 2023-05-17T10:02:35Z

could you (or should I) make #8738 pick these tests up as well and see it works. I understand that you want to create a separation between pure logical stuff to visual/interaction stuff but for the sake of the POC and in case we decide otherwise.

You can, but i don't want change test runner.
I want remove test runners.
JEST + PLAYWRIGTH would cover both bases and avoid us having to deal with a test runner

asturur · 2023-05-17T10:03:57Z

importing and snapshots in tests is extremely exciting

Yes another important point is that we want to test functions without having to put them in the bundle or test them from the bundle.
While current visual tests will be our sort of higher level test, that run from the package

asturur · 2023-05-17T10:06:54Z

i have no idea what types test are in general, because i don't understand why they are needed on top of TSC.
If you think are important, you can totally add them as long as the setup is simple.
There is limited amount of information my brain can process per day and i don't want to focus on those, but you can add them if you want them.

asturur · 2023-05-17T10:14:10Z

The issue that remains with visual tests is that we need to run them in 2 different suites.
In jest for node, and in playwright for the browser.
This makes it harder because the tests are written in a different way.
In that regard qunit is still useful but the intention is to phase it away.

With this PR i don't want to settle the conventions for all tests, i just want to see if JEST is good to replace QUNIT for all tests that are logic only and do not relate to events or canvas, and remove those from the browser.

This is the scope with which i opened this PR.

ShaMan123 · 2023-05-17T13:54:44Z

I am ok with merging this and moving forward.
We handle whatever comes next.

ShaMan123 · 2023-05-17T13:55:46Z

Regarding visual tests, I am hoping we will find a decent way to solve it and believe we will.

asturur · 2023-05-22T13:13:58Z

In order to merge this i need to understand what is going on with coverage.
Where is stored the action @ShaMan123 so i can look at what it does?

ShaMan123 · 2023-05-22T22:07:19Z

.github you mean? In the build action I think

ShaMan123 · 2023-05-23T03:18:30Z

fabric.js/.github/workflows/build.yml

Lines 92 to 109 in 9414434

    
           coverage: 
        
             if: ${{ always() }} 
        
             runs-on: ubuntu-latest 
        
             steps: 
        
               - uses: actions/checkout@v2 
        
               - name: Code Coverage Report Updater 
        
                 uses: actions/setup-node@v1 
        
                 with: 
        
                   node-version: 18.x 
        
               - run: npm ci 
        
               - run: npm run build -- -f 
        
               - run: npm run test:coverage && npm run test:visual:coverage 
        
               - run: npm run coverage:report 
        
               - uses: ShaMan123/lcov-reporter-action@v1.1.1 
        
                 with: 
        
                   github-token: ${{ secrets.GITHUB_TOKEN }} 
        
                   delete-old-comments: true 
        
                   update-comment: true

ShaMan123 · 2023-05-23T03:19:45Z

This action is wasteful and not precise (node only - many tests are skipped). We can use the existing test actions and upload the coverage outputs as an artifact and then run the report to combine both browser and node coverage

ShaMan123 · 2023-05-23T03:22:20Z

jest.config.js

+  clearMocks: true,
+
+  // Indicates whether the coverage information should be collected while executing the test
+  collectCoverage: false,


why is this false?

becasue we collect coverage with nyc for qunit, and is the only way to mix it it seems. So i disabled the internal coverage reporter of Jest

ShaMan123 · 2023-05-23T03:22:47Z

jest.config.js

+  coverageReporters: [
+    'json',
+    // "text",
+    // 'lcov',


did you try lcov? I think the action uses that

asturur · 2023-06-15T14:46:52Z

i have to add the globals or JEST before being able to merge

github-actions · 2023-06-15T23:10:27Z

Coverage after merging start-jest into master will be

83.74%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	118, 129, 138, 31, 94
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 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.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	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, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.05%	77.54%	83.05%	79.57%	1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1515, 161, 186, 296–297, 300–304, 309, 332–333, 338–343, 36, 363, 363, 363–364, 364, 364–365, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 40, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–618, 624, 631, 668, 668, 668, 670, 672–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
SelectableCanvas.ts	94.42%	92.24%	94.23%	96.15%	1115, 1115–1116, 1119, 1139, 1139, 1188, 1196, 1315, 1317, 1319–1320, 519, 694–695, 697–698, 698, 698, 747–748, 809–810, 863–865, 897, 902–903, 930–931
StaticCanvas.ts	96.73%	92.88%	100%	98.52%	1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1262–1263, 1339, 1348, 1348, 1352, 1352, 1399–1400, 309–310, 326, 694, 706–707
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	100%	100%	100%	100%
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.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.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%

ShaMan123

What is the status with coverge? Did you try lcov as well?
The action needs lcov reports.

ShaMan123 · 2023-06-16T02:56:59Z

.github/workflows/tests.yml

+          node-version: 18
+      - run: npm ci
+      - name: Run Jest unit test
+        run: npm run test:jest


I will follow up with cli PR exposing this and playwright

src/util/internals/cloneDeep.test.ts

ShaMan123 · 2023-06-16T03:02:52Z

src/util/internals/getRandomInt.test.ts

+  beforeAll(() => {
+    global.Math.random = jest.fn(() => 0.1);
+  });
+  afterAll(() => {
+    global.Math.random = originalMathRandom;
+  });
+  it('return a number between min and max', () => {
+    (global.Math.random as jest.Mock).mockReturnValue(0.1);
+    const semiRandom = getRandomInt(0, 10);
+    expect(semiRandom).toBe(1);
+  });


I don't understand why the beforeAll/afterAll if this test is using mocks
I think we should not do that anymore and use only mocks, stubs, spies whatever

i don't think you can mock random differently than this.
Because Math is not an import so it doesn't go in the import cache, so the standrd jest.mock('lib', mockFunction) doesn't work

So i would like to add sinon
Sinon does that so easily and plays nicely everywhere

asturur · 2023-06-16T03:32:51Z

What is the status with coverge? Did you try lcov as well? The action needs lcov reports.

For now we don't have it.
I ll see if i do that or more TS or more website.

…-jest

asturur · 2023-06-16T03:44:05Z

What is the status with coverge? Did you try lcov as well? The action needs lcov reports.

All the coverage is generated with NYC, so the format is the same as before ( LCOV ).
Locally it produces good coverage but in the GHA it doesn't.

introducing jest

e5a7695

add a github step

cb7ee9e

asturur added 7 commits May 14, 2023 18:03

add a github step

278e529

add this in the mix

652458d

tests

cae1e53

wrong script

bebef54

tests

5cf27b4

tests

491d167

added circle brush test just to try if coverage works

025149e

asturur added 3 commits May 14, 2023 18:37

run in band

e764991

alternative

f373f24

should work now

58c27bd

asturur marked this pull request as draft May 14, 2023 20:25

this works locally

9e856bc

ShaMan123 reviewed May 15, 2023

View reviewed changes

src/brushes/CircleBrush.test.ts Outdated

Copy link

Contributor

ShaMan123 May 15, 2023

Choose a reason for hiding this comment

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

This is very nice

ShaMan123 added the CI/CD label May 16, 2023

asturur added 2 commits May 19, 2023 00:51

Merge branch 'master' into start-jest

6ed1370

Merge branch 'master' into start-jest

1d1b2a6

ShaMan123 reviewed May 23, 2023

View reviewed changes

Merge branch 'master' into start-jest

ba9f956

asturur added 2 commits June 15, 2023 16:58

added types-jest

0235988

Jest test ts passing

a8d5288

asturur marked this pull request as ready for review June 15, 2023 15:04

asturur added 3 commits June 15, 2023 17:05

do not try to collect new coverage

8f989cf

removed coverage merge

dc1bcbb

replaced deleted test

1538181

Merge branch 'master' into start-jest

ae9dfee

ShaMan123 reviewed Jun 16, 2023

View reviewed changes

asturur added 2 commits June 16, 2023 05:36

cloneDeep extra assertion

e9fca36

Merge branch 'start-jest' of github.com:fabricjs/fabric.js into start…

57537c7

…-jest

asturur changed the title ~~[DRAFT] Introducing JEST~~ ci() Introducing JEST and basic tests Jun 16, 2023

Merge branch 'master' into start-jest

9095d7e

asturur merged commit 269f67d into master Jun 17, 2023

asturur deleted the start-jest branch June 17, 2023 13:36

ci() Introducing JEST and basic tests #8919

ci() Introducing JEST and basic tests #8919

Conversation

asturur commented May 14, 2023 • edited Loading

Motivation

github-actions bot commented May 14, 2023 • edited Loading

asturur commented May 14, 2023 • edited Loading

asturur commented May 14, 2023

asturur commented May 14, 2023 • edited Loading

asturur commented May 15, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 May 15, 2023

Choose a reason for hiding this comment

asturur commented May 16, 2023

ShaMan123 commented May 16, 2023

ShaMan123 commented May 16, 2023

ShaMan123 commented May 16, 2023

ShaMan123 commented May 16, 2023

ShaMan123 commented May 17, 2023 • edited Loading

asturur commented May 17, 2023

asturur commented May 17, 2023

asturur commented May 17, 2023

asturur commented May 17, 2023

ShaMan123 commented May 17, 2023

ShaMan123 commented May 17, 2023

asturur commented May 22, 2023

ShaMan123 commented May 22, 2023

ShaMan123 commented May 23, 2023

ShaMan123 commented May 23, 2023 • edited Loading

ShaMan123 May 23, 2023

Choose a reason for hiding this comment

asturur Jun 15, 2023

Choose a reason for hiding this comment

ShaMan123 May 23, 2023

Choose a reason for hiding this comment

asturur commented Jun 15, 2023

github-actions bot commented Jun 15, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Jun 16, 2023

Choose a reason for hiding this comment

ShaMan123 Jun 16, 2023

Choose a reason for hiding this comment

asturur Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

asturur commented Jun 16, 2023

asturur commented Jun 16, 2023 • edited Loading

asturur commented May 14, 2023 •

edited

Loading

github-actions bot commented May 14, 2023 •

edited

Loading

asturur commented May 14, 2023 •

edited

Loading

asturur commented May 14, 2023 •

edited

Loading

asturur commented May 15, 2023 •

edited

Loading

ShaMan123 commented May 17, 2023 •

edited

Loading

ShaMan123 commented May 23, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading

asturur Jun 16, 2023 •

edited

Loading

ShaMan123 Jun 16, 2023 •

edited

Loading

asturur commented Jun 16, 2023 •

edited

Loading