test() When a test fails in node, dump the result in cli_output #8206

asturur · 2022-08-28T21:03:24Z

No description provided.

github-actions · 2022-08-28T21:05:45Z

Coverage after merging node-dump-diffs into master will be

81.06%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
dist
fabric.js	79.40%	75.56%	84.72%	81.06%	10006, 10025–10027, 10029–10030, 10092, 10146, 10161, 10217, 10253–10254, 10260, 10264–10265, 10280, 10314, 10345–10346, 10370, 10378, 10465, 10497–10498, 10573–10574, 10577, 10582, 10604, 10604, 10604, 10604, 10604, 10604, 10604–10605, 10607–10608, 10608, 10608–10609, 10614, 10616, 10622, 10622, 10622, 10622, 10622–10623, 10625–10629, 10629, 10629–10631, 10633–10634, 10643, 10654, 10665, 10676, 10686–10689, 10697–10698, 10698, 10698–10699, 10701–10702, 10709, 10713, 1085, 11310, 11315, 11357–11359, 11359, 11359, 11359, 11359, 11359–11360, 11380, 11385–11386, 11426, 11426, 11426, 11426, 11454, 11454, 11454, 11456, 116, 1160, 11604–11605, 11616, 1162–1163, 1165–1166, 1173, 1177, 11773–11774, 11781, 11824, 11831, 11831, 11840–11841, 1185, 11852, 11892, 119, 11900, 11919–11921, 11948–11949, 11952, 11952, 11969–11971, 120, 12074, 121, 12159–12160, 12162–12163, 12170, 12194, 1221, 1223, 1223, 1223, 1223, 1223–1224, 12259–12260, 12264, 12344, 12355, 12360, 12397, 12498, 12498, 12498, 12522–12523, 12631, 12634, 12696, 12702, 12709, 12716, 12722, 12728, 12735, 12742, 12748–12749, 12749, 12749, 12764–12765, 12773, 12782, 12782, 128, 12807–12810, 12849, 12892–12893, 129, 129, 129, 129, 129, 129, 129, 12933–12934, 130, 13067–13068, 131, 13213, 133, 13305, 13368, 13371, 13424–13425, 13425, 13425, 13428, 13443, 13457, 13469–13470, 13472, 13484–13485, 13487, 13502, 13517–13518, 13520–13521, 13523–13524, 13534, 13564, 13564, 13564, 13564, 13564, 13564, 13564, 13564, 13589, 13591, 13591–13593, 13646–13648, 13671, 13679, 13685–13686, 13727, 13790–13791, 13825, 13846–13847, 1390, 1390, 13906, 13906, 13906, 13906, 13906, 13911–13919, 1392, 13961, 14087–14088, 14124, 14133, 14154, 14154, 14154–14155, 14155, 14155, 14155, 14155–14156, 14162–14164, 14167–14168, 14181, 14181, 14181–14182, 14182, 14182, 14182, 14182–14183, 14189–14191, 14194–14195, 14208–14209, 14283, 14332, 14336, 1442, 14461, 14491–14492, 14492, 14492–14493, 14493, 14493–14494, 14496, 14496, 14496–14497, 14500, 14507, 14592, 14705, 14754, 14824, 14828, 14898, 14955–14956, 14970, 15020–15021, 15023–15024, 15116, 15176, 15179, 15254, 15257, 15272–15273, 15282, 15324, 15328, 15353–15354, 15388, 15392, 1562, 15658, 15662, 15826, 15829, 15836–15837, 15837, 15837–15839, 15841, 15841, 15841–15843, 15843, 15843–15845, 15935, 15940–15942, 15970–15971, 16038, 16041, 16041, 16041–16042, 16044–16045, 16045, 16045–16046, 16046, 16046, 16048–16049, 16051, 16054, 16081, 16081–16083, 16132, 16132, 16167, 16202, 16202, 16202, 16205–16206, 16208, 16208, 16212, 16215, 16218, 16220–16221, 1623, 16230, 16235, 16235–16237, 16237, 16248–16249, 16258, 16258, 16258, 16258, 16258–16259, 16259, 16259–16261, 16261–16262, 16262, 16279–16280, 16280, 16280, 16302–16303, 16303, 16303, 16303, 16303, 16303, 16315, 16315, 16318–16322, 16322, 16361, 16400, 16432, 16435–16438, 16448, 16461, 16476, 16493, 16502, 16506, 16558, 16560–16562, 16576, 16578–16579, 16595, 16647, 16670, 16803, 16866–16868, 16868–16869, 16869, 16897, 16942–16945, 16952, 16954–16955, 16970–16972, 16982, 16982, 16982, 17045, 17057, 17057, 17104–17105, 17123–17124, 17145, 17186, 17186–17187, 17204–17207, 17207, 17207–17208, 17210, 17210, 17210–17211, 17213–17214, 17214, 17214–17215, 17217, 17217, 17217–17218, 17220–17221, 17225–17226, 17273, 17317, 17386, 17391, 17415–17416, 17425–17429, 17441–17444, 17449, 17458–17459, 17461, 17470, 17470, 17470, 17470, 17470–17471, 17473–17474, 17490–17491, 17493–17494, 17501–17504, 17507, 17510, 17512–17513, 17513, 17513, 17513, 17513, 17513, 17513–17514, 17516, 17518–17519, 17519, 17519–17522, 17524, 17531–17539, 17539, 17539–17541, 17544, 17552–17555, 17561–17562, 17562, 17562–17563, 17565, 17565, 17565–17566, 17568, 17570–17571, 17586, 17588, 17588, 17588–17589, 17591–17592, 17592–17593, 17593, 17599, 17599, 1760, 17601, 17601–17602, 17602, 1761, 17611–17613, 17613, 17613–17621, 17627, 17627, 17627–17629, 17631, 17637–17638, 17652–17658, 17658, 17658–17659, 17662, 17664, 17676, 17676, 17676–17677, 17680–17682, 17692, 17692, 17692–17694, 17706, 17706, 17706–17707, 17709–17710, 17710, 17710–17711, 17713–17714, 17714, 17714–17717, 17717, 17717–17718, 17720, 17720, 17720–17721, 17724–17725, 17728, 17730–17731, 17731, 17731, 17731, 17731–17733, 17747–17749, 17751–17752, 17763, 17765, 17767–17770, 17827, 17893,

ShaMan123 · 2022-08-29T03:32:43Z

test/lib/visualTestLoop.js

+      const fileName = localPath('../../cli_output', `${testName.replaceAll(' ', '_')}.png`);
+      fs.writeFileSync(fileName.replace('file://', ''), dataUrl, { encoding: 'base64' });
+    }
+    // else if (original) {


leave this please
I use it, git shows a very convenient diff.
And this way I can commit the change to the repo if I need to in a click

I have disabled golden debugging/recreation from node because I understood that it is not accurate enough because of node-canvas

fabric.js/test/node_test_setup.js

Line 14 in 5ead2d6

// QUnit.debugVisual = Number(process.env.QUNIT_DEBUG_VISUAL_TESTS);

Don't you enjoy using it this way?
It is easy to add an option from the cli to dump the diff to cli_output instead of updating the ref.

that is not the one you use. That is a copy that i commented because i didn't write for browser

https://github.com/fabricjs/fabric.js/pull/8206/files#diff-e236a8dadb89494460b0e0c2d0018e76d602cb4bf08e1f52d3e673b7709a03ebR71-R79

this is the one you mean

https://github.com/fabricjs/fabric.js/pull/8206/files#diff-e236a8dadb89494460b0e0c2d0018e76d602cb4bf08e1f52d3e673b7709a03ebR71-R79

The link points me to a strange place. Add a comment in the code so I can see

that is not the one you use. That is a copy that i commented because i didn't write for browser

I wrote that in #8138

I have disabled golden debugging/recreation from node because I understood that it is not accurate enough because of node-canvas

Yes node-canvas is not accurate enough, but i generate the golden from node anyway because they help finding out issues earlier. When a test is too much different in node, we disable the test for node ( we need to do that anyway or it will fail ).

Don't you enjoy using it this way?

I think the old was the best, i deleted a golden and i would get it back just running the tests, no flags.

I think the old was the best, i deleted a golden and i would get it back just running the tests, no flags.

It still works like that.
The -d flag will recreate the golden ONLY if the test failed
The -r flag will recreate the golden regardless

I don't mind switching the -d flag on as default

For me it is annoying running a test, seeing it fails, navigating to the file, deleting it and all over again each time I change something in the code and want to test the diff

I flag -d and it happens.
Then I can discard changes in github desktop.
No hassle.

Perhaps I didn't explain how to use it properly until now.
It should make your life much easier.

ShaMan123 · 2022-08-29T03:34:26Z

test/lib/visualTestLoop.js

+              // var stringa = imageDataToChalk(output);
+              // console.log(stringa);
+              try {
+                dumpFailedTest(testName, renderedCanvas, canvas, output);


you can move this into the if block below so it happens only when -d or -r flag are set

i prefer it to happen every run, in case i can link the results out in PRs

I looked into that as well
But you said that node visual test results are not accurate.
That is why I disabled the option the debug visuals in node tests to begin with and moved to browser outputs.

It can be done if we post the results somewhere.
Github doesn't allow creating a comment with images using actions/bots but we can easily add links to posted images.
Maybe we create a test dumps repo under fabricjs org and commit to there

we create a branch, push images and remove the branch once the pr is closed

How about the following:
CI runs visual tests with -d flag, meaning that failing tests update golden refs.
Then it creates a branch with these changes and posts a comment with the diff between the branches.
It can prefix the branch name with something and delete it once the pr is closed.
Thoughts?

I didn't add a generate golden method, the method is differnet and produces a collage of the 3 images.
This method replaced the imageDataToChalk function that was not really usefull.
I want to take care of this, i felt the need for it, i started it, i can finish it. Then if is really ugly we can improve it.

we create a branch, push images and remove the branch once the pr is closed

This seems an overkill of resourcers.

There are actions that archive artifacts of pr runs, and keep them somewhere in github.

I looked at artifacts...
Am not sure how to display the results though
This on the other hand is simple: ci-visual-test-action...ci-pr#8210-visuals-firefox

Maybe is an overkill, but it is on github actions...
This is the result:
#8210 (comment)

@asturur

@asturur this is what I meant in the comments of #8206

asturur · 2022-08-29T23:33:10Z

.github/workflows/visual-test-chrome.yml

+      uses: actions/upload-artifact@v3
+      with:
+        name: chrome-failing-visual-tests
+        path: cli_output/failed_visual_tests/chrome/*


this doesn't work yet

asturur · 2022-08-29T23:33:22Z

.github/workflows/visual-test-firefox.yml

+      uses: actions/upload-artifact@v3
+      with:
+        name: firefox-failing-visual-tests
+        path: cli_output/failed_visual_tests/firefox/*


this doesn't work yet

ShaMan123 · 2022-09-01T05:38:03Z

I suggest you merge #8227 first.
It will be simpler working on one file/workflow

stale · 2022-09-21T02:40:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ShaMan123 · 2022-10-30T16:22:47Z

I am closing in favor of #8402

If you disagree reopen

asturur added 2 commits August 28, 2022 23:00

dump-files-node

f1eaba8

stupid vscode

67417a9

ShaMan123 suggested changes Aug 29, 2022

View reviewed changes

ShaMan123 added a commit that referenced this pull request Aug 29, 2022

reintroduce node golden generation

ee3388c

@asturur this is what I meant in the comments of #8206

asturur added 5 commits August 29, 2022 23:51

failing tests on purpose

0c89cc0

wrong workflow

1bcf5b9

test this

82cf8ab

uses for all actions

76330fa

test

bdf9b5e

asturur added the CI/CD label Aug 29, 2022

asturur added 4 commits August 30, 2022 00:32

mixed changes

2c25777

try

76930a5

try

ba9ee5f

ok fixed

2b95e53

asturur commented Aug 29, 2022

View reviewed changes

asturur added 2 commits August 30, 2022 01:37

removed console logs

7afce2a

merge master

58d14fd

ShaMan123 mentioned this pull request Sep 1, 2022

ci(): visual test results action #8210

Closed

asturur added 9 commits September 4, 2022 00:20

modified test

890d775

no extra job

da0936c

if remove name, dash uses

119d41b

triple equals are a js thing

88fa03c

restrict to 16.x

4f5315a

restrict to 16.x

397ab04

Update tests.mustache

e91ab52

no quotes

8ea7d9d

Update node_test_setup.js

6175e80

stale bot added the stale Issue marked as stale by the stale bot label Sep 21, 2022

ShaMan123 removed the stale Issue marked as stale by the stale bot label Sep 21, 2022

ShaMan123 mentioned this pull request Oct 30, 2022

ci(): upload failed visuals artifact + perf + cleanup #8402

Open

ShaMan123 closed this Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test() When a test fails in node, dump the result in cli_output #8206

test() When a test fails in node, dump the result in cli_output #8206

asturur commented Aug 28, 2022

github-actions bot commented Aug 28, 2022 •

edited

ShaMan123 Aug 29, 2022 •

edited

asturur Aug 29, 2022

asturur Aug 29, 2022

ShaMan123 Aug 29, 2022 •

edited

asturur Aug 29, 2022 •

edited

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022 •

edited

asturur Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022 •

edited

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

asturur Aug 29, 2022

ShaMan123 Aug 29, 2022

ShaMan123 Aug 29, 2022

asturur Aug 29, 2022

asturur Aug 29, 2022

ShaMan123 commented Sep 1, 2022

stale bot commented Sep 21, 2022

ShaMan123 commented Oct 30, 2022

test() When a test fails in node, dump the result in cli_output #8206

test() When a test fails in node, dump the result in cli_output #8206

Conversation

asturur commented Aug 28, 2022

github-actions bot commented Aug 28, 2022 • edited

ShaMan123 Aug 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Aug 29, 2022 • edited

Choose a reason for hiding this comment

asturur Aug 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Aug 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Aug 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Sep 1, 2022

stale bot commented Sep 21, 2022

ShaMan123 commented Oct 30, 2022

github-actions bot commented Aug 28, 2022 •

edited

ShaMan123 Aug 29, 2022 •

edited

ShaMan123 Aug 29, 2022 •

edited

asturur Aug 29, 2022 •

edited

ShaMan123 Aug 29, 2022 •

edited

ShaMan123 Aug 29, 2022 •

edited