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.

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

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

Open
wants to merge 118 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 118 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Oct 30, 2022 •
edited
Loading

Motivation

Continues #8206 with #8311
closes #8206
closes #8311

Decluttering Motivation:
Make tests accessible, readable and refactorable and for the future when we decide to move to other frameworks (I have split up logic for that)

Description

  • Changes to the test suite (parallel testing on different suites/contexts)
  • Optimization of github actions
  • Test result artifact
  • removed GabrielBB/xvfb-action for native xvfb 42c41f7

Changes

Apart from uploading test results and creating a dedicated index file for visual test results, I have optimized perf:

  • used npm caching and a build artifact for tests which reduced total time by 2-3 minutes for tests (14 vs. 17 m)
  • used the build job to cut down another build in the stats step (reduce by ~1 m)
  • introduced parallel testing (can be opt out with -np flag) and consolidated suite matrices, reducing a lot of setup time. This option is enabled locally as well and is blazing fast! I want your input on DX in iOS.
    Once we move to a stronger test framework and use parallel testing in the test runner itself time will drop exponentially.

Decluttered:

  • removed test/lib/pixelmatch.js in favor of node_module
  • refactored test/lib/visualCallbackQunit.js => test/lib/appendTestResults.js (dumpResults is now in charge of creating visuals for the browser and this nodule simply loads them)
  • test/lib/visualTestLoop.js has been split into dedicated modules for browser (test/lib/visualUtil.browser.js), node (test/lib/visualUtil.node.js) and common (test/lib/visualUtil.js) + QUnit.assert.visualEqual that is defined by each
  • test/node_test_setup.js => test/testSetup.node.js, test/qunitSetup.node.js, test/fabricSetup.node.js

Gist

In Action

https://github.com/fabricjs/fabric.js/actions/runs/3384008742

image

This is the visual test results index file after unzipping artifact

image

@ShaMan123
fix(): test with missing golden running from CI should fail
c14e307
@ShaMan123
Update visualTestLoop.js
f98ec94
@ShaMan123
Update visualTestLoop.js
0b44c00
@ShaMan123
Update visualTestLoop.js
69c5663
@ShaMan123
Update visualTestLoop.js
d392ced
@ShaMan123
Update visualTestLoop.js
9b13464
@ShaMan123
Update CHANGELOG.md
7e0ed21
@ShaMan123
Update visualTestLoop.js
51b2590
@ShaMan123
test ci
3d3922c
@ShaMan123
Update visualTestLoop.js
6ee4cf1
@ShaMan123
Revert "test ci"
2d451bc 
This reverts commit 3d3922c.
@ShaMan123
Merge branch 'master' into ci-safegurad-visual-tests
60ec16e
@ShaMan123
Merge branch 'master' into ci-safegurad-visual-tests
b1025c4
@ShaMan123
refactor visual test results
8054180
@ShaMan123
Update testem.config.js
d4c8d6a
@ShaMan123
Update testem.config.js
958dc06
@ShaMan123
better
c47abf0
@ShaMan123
rename
49c7a31
@ShaMan123
Squashed commit of the following:
ab94dd7 
commit 6175e80
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 01:18:18 2022 +0200

    Update node_test_setup.js

commit 8ea7d9d
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 01:12:06 2022 +0200

    no quotes

commit e91ab52
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 01:11:11 2022 +0200

    Update tests.mustache

commit 397ab04
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:58:14 2022 +0200

    restrict to 16.x

commit 4f5315a
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:55:53 2022 +0200

    restrict to 16.x

commit 88fa03c
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:41:03 2022 +0200

    triple equals are a js thing

commit 119d41b
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:38:17 2022 +0200

    if remove name, dash uses

commit da0936c
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:36:25 2022 +0200

    no extra job

commit 890d775
Merge: 58d14fd b4af967
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Sep 4 00:20:58 2022 +0200

    modified test

commit 58d14fd
Merge: 7afce2a 195eeff
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 07:59:20 2022 +0200

    merge master

commit 7afce2a
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 01:37:04 2022 +0200

    removed console logs

commit 2b95e53
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 01:32:11 2022 +0200

    ok fixed

commit ba9ee5f
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:51:21 2022 +0200

    try

commit 76930a5
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:50:56 2022 +0200

    try

commit 2c25777
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:32:51 2022 +0200

    mixed changes

commit bdf9b5e
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:21:18 2022 +0200

    test

commit 76330fa
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:13:13 2022 +0200

    uses for all actions

commit 82cf8ab
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Tue Aug 30 00:08:24 2022 +0200

    test this

commit 1bcf5b9
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Mon Aug 29 23:58:33 2022 +0200

    wrong workflow

commit 0c89cc0
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Mon Aug 29 23:51:32 2022 +0200

    failing tests on purpose

commit 67417a9
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Aug 28 23:05:42 2022 +0200

    stupid vscode

commit f1eaba8
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Aug 28 23:00:23 2022 +0200

    dump-files-node
@ShaMan123
Update tests.yml
1fa1fc5
@ShaMan123
commit test regression
72e5d4e
@ShaMan123
Update tests.yml
788b1a0
@ShaMan123 ShaMan123 mentioned this pull request Oct 30, 2022
Closed
@ShaMan123 ShaMan123 mentioned this pull request Oct 30, 2022
Closed
@ShaMan123 ShaMan123 changed the title ci(): visuals action ci(): upload failed visuals artifact Oct 30, 2022
@ShaMan123 ShaMan123 changed the title ci(): upload failed visuals artifact + perf ci(): upload failed visuals artifact + perf + cleanup Nov 5, 2022
ShaMan123
ShaMan123 commented Nov 5, 2022
View reviewed changes
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.

Decluttered:

  • removed test/lib/pixelmatch.js in favor of node_module
  • refactored test/lib/visualCallbackQunit.js => test/lib/appendTestResults.js (dumpResults is now in charge of creating visuals for the browser and this nodule simply loads them)
  • test/lib/visualTestLoop.js has been split into dedicated modules for browser (test/lib/visualUtil.browser.js), node (test/lib/visualUtil.node.js) and common (test/lib/visualUtil.js) + QUnit.assert.visualEqual that is defined by each
  • test/node_test_setup.js => test/testSetup.node.js, test/qunitSetup.node.js, test/fabricSetup.node.js

Motivation:
Make tests accessible, readable and refactorable for future when we decide to move to other frameworks

@ShaMan123
Copy link
Contributor Author

I may have exaggerated in this PR
If it is too much tell me and I will try to extract some

@ShaMan123 ShaMan123 mentioned this pull request Nov 19, 2022
Merged
@asturur asturur self-assigned this Nov 27, 2022
@ShaMan123 ShaMan123 mentioned this pull request Jun 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur Awaiting requested review from asturur

At least 1 approving review is required to merge this pull request.

Assignees

@asturur asturur

Labels
CI/CD fix-next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ShaMan123 @asturur