Skip to content

Commit 432e684

Browse files
yaRnMcDonutsyaRnMcDonuts
authored
Merge pull request #2413 from richardTingle/create-additional-screenshot-tests
Create additional screenshot tests and add an auto message on screenshot test failure
2 parents d128d96 + dff1c33 commit 432e684

10 files changed

+576
-2
lines changed

‎.github/workflows/screenshot-test-comment.yml

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
name: Screenshot Test PR Comment
2+
3+
# This workflow is designed to safely comment on PRs from forks
4+
# It uses pull_request_target which has higher permissions than pull_request
5+
# Security note: This workflow does NOT check out or execute code from the PR
6+
# It only monitors the status of the ScreenshotTests job and posts comments
7+
# (If this commenting was done in the main worflow it would not have the permissions
8+
# to create a comment)
9+
10+
on:
11+
pull_request_target:
12+
types: [opened, synchronize, reopened]
13+
14+
jobs:
15+
monitor-screenshot-tests:
16+
name: Monitor Screenshot Tests and Comment
17+
runs-on: ubuntu-latest
18+
timeout-minutes: 60
19+
permissions:
20+
pull-requests: write
21+
contents: read
22+
steps:
23+
- name: Wait for GitHub to register the workflow run
24+
run: sleep 15
25+
26+
- name: Wait for Screenshot Tests to complete
27+
uses: lewagon/[email protected]
28+
with:
29+
ref: ${{ github.event.pull_request.head.sha }}
30+
check-name: 'Run Screenshot Tests'
31+
repo-token: ${{ secrets.GITHUB_TOKEN }}
32+
wait-interval: 10
33+
allowed-conclusions: success,skipped,failure
34+
- name: Check Screenshot Tests status
35+
id: check-status
36+
uses: actions/github-script@v6
37+
with:
38+
github-token: ${{ secrets.GITHUB_TOKEN }}
39+
script: |
40+
const { owner, repo } = context.repo;
41+
const ref = '${{ github.event.pull_request.head.sha }}';
42+
43+
// Get workflow runs for the PR
44+
const runs = await github.rest.actions.listWorkflowRunsForRepo({
45+
owner,
46+
repo,
47+
head_sha: ref
48+
});
49+
50+
// Find the ScreenshotTests job
51+
let screenshotTestRun = null;
52+
for (const run of runs.data.workflow_runs) {
53+
if (run.name === 'Build jMonkeyEngine') {
54+
const jobs = await github.rest.actions.listJobsForWorkflowRun({
55+
owner,
56+
repo,
57+
run_id: run.id
58+
});
59+
60+
for (const job of jobs.data.jobs) {
61+
if (job.name === 'Run Screenshot Tests') {
62+
screenshotTestRun = job;
63+
break;
64+
}
65+
}
66+
67+
if (screenshotTestRun) break;
68+
}
69+
}
70+
71+
if (!screenshotTestRun) {
72+
console.log('Screenshot test job not found');
73+
return;
74+
}
75+
76+
// Check if the job failed
77+
if (screenshotTestRun.conclusion === 'failure') {
78+
core.setOutput('failed', 'true');
79+
} else {
80+
core.setOutput('failed', 'false');
81+
}
82+
- name: Find Existing Comment
83+
uses: peter-evans/find-comment@v3
84+
id: existingCommentId
85+
with:
86+
issue-number: ${{ github.event.pull_request.number }}
87+
comment-author: 'github-actions[bot]'
88+
body-includes: Screenshot tests have failed.
89+
90+
- name: Comment on PR if tests fail
91+
if: steps.check-status.outputs.failed == 'true'
92+
uses: peter-evans/create-or-update-comment@v4
93+
with:
94+
issue-number: ${{ github.event.pull_request.number }}
95+
body: |
96+
🖼️ **Screenshot tests have failed.**
97+
98+
The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests.
99+
100+
📄 **Where to find the report:**
101+
- Go to the (failed run) > Summary > Artifacts > screenshot-test-report
102+
- Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html
103+
104+
⚠️ **If you didn't expect to change anything visual:**
105+
Fix your changes so the screenshot tests pass.
106+
107+
✅ **If you did mean to change things:**
108+
Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.
109+
110+
✨ **If you are creating entirely new tests:**
111+
Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.
112+
113+
**Note;** it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar".
114+
115+
See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information
116+
edit-mode: replace
117+
comment-id: ${{ steps.existingCommentId.outputs.comment-id }}

‎jme3-screenshot-tests/README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
# jme3-screenshot-tests
22

3-
This module contains tests that compare screenshots of the JME3 test applications to reference images. The tests are run using
4-
the following command:
3+
This module contains tests that compare screenshots of the JME3 test applications to reference images. Think of these like visual unit tests
4+
5+
The tests are run using the following command:
56

67
```
78
./gradlew :jme3-screenshot-test:screenshotTest

‎jme3-screenshot-tests/src/test/java/org/jmonkeyengine/screenshottests/animation/TestIssue2076.java

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
/*
2+
* Copyright (c) 2024 jMonkeyEngine
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are
7+
* met:
8+
*
9+
* * Redistributions of source code must retain the above copyright
10+
* notice, this list of conditions and the following disclaimer.
11+
*
12+
* * Redistributions in binary form must reproduce the above copyright
13+
* notice, this list of conditions and the following disclaimer in the
14+
* documentation and/or other materials provided with the distribution.
15+
*
16+
* * Neither the name of 'jMonkeyEngine' nor the names of its contributors
17+
* may be used to endorse or promote products derived from this software
18+
* without specific prior written permission.
19+
*
20+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
21+
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
22+
* TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
23+
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
24+
* CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
25+
* EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
26+
* PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
27+
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
28+
* LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
29+
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
30+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
31+
*/
32+
package org.jmonkeyengine.screenshottests.animation;
33+
34+
import com.jme3.anim.SkinningControl;
35+
import com.jme3.anim.util.AnimMigrationUtils;
36+
import com.jme3.animation.SkeletonControl;
37+
import com.jme3.app.Application;
38+
import com.jme3.app.SimpleApplication;
39+
import com.jme3.app.state.BaseAppState;
40+
import com.jme3.asset.AssetManager;
41+
import com.jme3.light.AmbientLight;
42+
import com.jme3.math.ColorRGBA;
43+
import com.jme3.math.Vector3f;
44+
import com.jme3.scene.Geometry;
45+
import com.jme3.scene.Mesh;
46+
import com.jme3.scene.Node;
47+
import com.jme3.scene.VertexBuffer;
48+
import org.jmonkeyengine.screenshottests.testframework.ScreenshotTestBase;
49+
import org.junit.jupiter.api.Test;
50+
51+
/**
52+
* Screenshot test for JMonkeyEngine issue #2076: software skinning requires vertex
53+
* normals.
54+
*
55+
* <p>If the issue is resolved, 2 copies of the Jaime model will be rendered in the screenshot.
56+
*
57+
* <p>If the issue is present, then the application will immediately crash,
58+
* typically with a {@code NullPointerException}.
59+
*
60+
* @author Stephen Gold (original test)
61+
* @author Richard Tingle (screenshot test adaptation)
62+
*/
63+
public class TestIssue2076 extends ScreenshotTestBase {
64+
65+
/**
66+
* This test creates a scene with two Jaime models, one using the old animation system
67+
* and one using the new animation system, both with software skinning and no vertex normals.
68+
*/
69+
@Test
70+
public void testIssue2076() {
71+
screenshotTest(new BaseAppState() {
72+
@Override
73+
protected void initialize(Application app) {
74+
SimpleApplication simpleApplication = (SimpleApplication) app;
75+
Node rootNode = simpleApplication.getRootNode();
76+
AssetManager assetManager = simpleApplication.getAssetManager();
77+
78+
// Add ambient light
79+
AmbientLight ambientLight = new AmbientLight();
80+
ambientLight.setColor(new ColorRGBA(1f, 1f, 1f, 1f));
81+
rootNode.addLight(ambientLight);
82+
83+
/*
84+
* The original Jaime model was chosen for testing because it includes
85+
* tangent buffers (needed to trigger issue #2076) and uses the old
86+
* animation system (so it can be easily used to test both systems).
87+
*/
88+
String assetPath = "Models/Jaime/Jaime.j3o";
89+
90+
// Test old animation system
91+
Node oldJaime = (Node) assetManager.loadModel(assetPath);
92+
rootNode.attachChild(oldJaime);
93+
oldJaime.setLocalTranslation(-1f, 0f, 0f);
94+
95+
// Enable software skinning
96+
SkeletonControl skeletonControl = oldJaime.getControl(SkeletonControl.class);
97+
skeletonControl.setHardwareSkinningPreferred(false);
98+
99+
// Remove its vertex normals
100+
Geometry oldGeometry = (Geometry) oldJaime.getChild(0);
101+
Mesh oldMesh = oldGeometry.getMesh();
102+
oldMesh.clearBuffer(VertexBuffer.Type.Normal);
103+
oldMesh.clearBuffer(VertexBuffer.Type.BindPoseNormal);
104+
105+
// Test new animation system
106+
Node newJaime = (Node) assetManager.loadModel(assetPath);
107+
AnimMigrationUtils.migrate(newJaime);
108+
rootNode.attachChild(newJaime);
109+
newJaime.setLocalTranslation(1f, 0f, 0f);
110+
111+
// Enable software skinning
112+
SkinningControl skinningControl = newJaime.getControl(SkinningControl.class);
113+
skinningControl.setHardwareSkinningPreferred(false);
114+
115+
// Remove its vertex normals
116+
Geometry newGeometry = (Geometry) newJaime.getChild(0);
117+
Mesh newMesh = newGeometry.getMesh();
118+
newMesh.clearBuffer(VertexBuffer.Type.Normal);
119+
newMesh.clearBuffer(VertexBuffer.Type.BindPoseNormal);
120+
121+
// Position the camera to see both models
122+
simpleApplication.getCamera().setLocation(new Vector3f(0f, 0f, 5f));
123+
}
124+
125+
@Override
126+
protected void cleanup(Application app) {
127+
}
128+
129+
@Override
130+
protected void onEnable() {
131+
}
132+
133+
@Override
134+
protected void onDisable() {
135+
}
136+
137+
@Override
138+
public void update(float tpf) {
139+
super.update(tpf);
140+
}
141+
}).run();
142+
}
143+
}

0 commit comments

Comments
 (0)