Skip to content

Conversation

@asturur
Copy link
Member

@asturur asturur commented Jun 17, 2023

Motivation

I was trying to remove the extra run of the tests for coverage and i went in a rabbit hole with caching.

Changes

  • added a cached action for the install
  • added a cached action to build fabric ( building fabric is actually very slow in ci )
  • pulled out from the job matrix node 18 and made its own job with coverage
  • added node 20 in the tests

There are some improvements:

This is a standar node test run now:
image

While before:
image

The build part that was taking 32 seconds is gone, while the cached install is taking anywat the same amount of before, meaning that either NPM is super fast, or that the cache is unefficient.

I do prefer to have it cached since at least the bandwidth is internal rather than external

@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2023

Build Stats

file / KB (diff) bundled minified
fabric 922.882 (0) 303.174 (0)

@CODE-AXION
Copy link
Contributor

CODE-AXION commented Jun 17, 2023

@asturur hey did you checked my PR ? #8955

@asturur
Copy link
Member Author

asturur commented Jun 17, 2023

@CODE-AXION this is not how it works.
You can't jump on related PRs bumping up your.
We can check it when we can ckeck it. We are trying to prioritize work to get 6.0 published.

@fabricjs fabricjs deleted a comment from github-actions bot Jun 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 18, 2023

Coverage after merging coverage-action into master will be

83.75%

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%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   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, 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.01%82.35%100%93.75%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, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.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.ts94.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.ts96.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.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.ts5.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.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.ts84.21%77.78%50%100%14, 17
   index.ts100%100%100%100%
   node.ts74.07%33.33%66.67%88.89%

@asturur
Copy link
Member Author

asturur commented Jun 18, 2023

ok this starts to be good.

@asturur asturur changed the title ci(): Coverage runs differently. WIP ci(): Refactor GHA for reuse and caching Jun 18, 2023
@asturur
Copy link
Member Author

asturur commented Jun 19, 2023

I think i m done with this.
The job prime-build is something i m not entirely sold into yet, but i would like to leave it on for a bit.
It makes everything start later, but it avoids building fabricJS more than once for the entire test run.
So we wait a bit more, but on the pipeline we save 11 builds after.

If we let every job start in parallel most o them will get to build time when the build is not cached yet.

uses: ./.github/actions/build-fabric-cached
- name: Run ${{ matrix.target }} ${{ matrix.suite }} headless test
uses: GabrielBB/xvfb-action@v1
uses: coactions/setup-xvfb@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed any more
I will find the comment in one of my ci prs

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

what about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

even better imho. I forgot about the visual artifact prs, we should merge that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is like a function?
I should turn the changelog actions into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

is an action made of more steps that you don't need to publish. If is not reusable outside fabricJS is a good idea to have it here

@asturur asturur merged commit fe813e1 into master Jul 1, 2023
@asturur asturur deleted the coverage-action branch July 1, 2023 21:19
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.

3 participants