Android screenshot tests 2#2809
Conversation
…a issues in screenshots
…ility in the pipelines
…h which android can mess with)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a cross-platform screenshot testing framework for jMonkeyEngine, adding dedicated modules for Android, Desktop, and shared test scenarios. It refactors existing tests into reusable scenarios and implements a reporting system using ExtentReports. The reviewer identified several technical issues and improvement opportunities, including a bug in path resolution for image attachments, inaccuracies in the custom timer logic, and performance concerns regarding frequent report serialization and asset manager instantiation. Additionally, it was recommended to use a stable version of Jackson and to replace fragile stack-trace-based test discovery with a more robust mechanism.
| report.addTest(testInProgress); | ||
| ExtentReportLogCapture.restore(); | ||
| testInProgress = null; | ||
| persistReport(); // it sucks that we do this every test |
There was a problem hiding this comment.
Calling persistReport() inside the finished() method of the test watcher causes the entire accumulated report to be serialized to JSON after every test method. This leads to O(N^2) I/O overhead as the number of tests increases. It would be more efficient to persist the report once after all tests in a class or suite have completed.
References
- Issues found in test code should be reported with a reduced priority, at most medium.
| try (FileOutputStream fileOutBuf = new FileOutputStream(reportPath().toPath().resolve(fileName).toFile())) { | ||
| JmeSystem.writeImageFile(fileOutBuf, "png",originalImage.getData(0),originalImage.getWidth(), originalImage.getHeight()); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| currentTest.addScreenCaptureFromPath(fileName, title); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The path resolution logic here is incorrect. reportPath() returns the path to the HTML report file (build/reports/ScreenshotDiffReport.html). Calling resolve(fileName) on its Path representation treats the HTML file as a directory, which will cause a FileNotFoundException when attempting to create the FileOutputStream. The images should be saved in the parent directory of the report file.
@Override
public void attachImage(String title, String fileName, Image originalImage) {
try (FileOutputStream fileOutBuf = new FileOutputStream(new File(reportPath().getParentFile(), fileName))) {
JmeSystem.writeImageFile(fileOutBuf, "png",originalImage.getData(0),originalImage.getWidth(), originalImage.getHeight());
} catch (IOException e) {
throw new RuntimeException(e);
}
currentTest.addScreenCaptureFromPath(fileName, title);
}References
- Issues found in test code should be reported with a reduced priority, at most medium.
| private Image readImageFromClasspath(String location) { | ||
| AssetManager assetManager = new DesktopAssetManager(); | ||
| assetManager.registerLocator("", ClasspathLocator.class); | ||
| assetManager.registerLoader(StbImageLoader.class, "png", "jpg", "jpeg"); |
There was a problem hiding this comment.
Instantiating a new DesktopAssetManager for every image load is resource-intensive. Since reference images are loaded frequently during test execution, consider using a shared AssetManager or a more lightweight image loading mechanism.
References
- Issues found in test code should be reported with a reduced priority, at most medium.
…t-tests-2 # Conflicts: # gradle/libs.versions.toml # settings.gradle
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: 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". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
1 participant
No description provided.