Visual regression plugin update #53
Conversation
Used to group POMs together
addPluginWithKey(Class key, Class<T> plugin, Object... args) allows for a plugin to be registered under a second key; Useful for ScreenshotPlugin, which is extended by multiple classes, yet VRT plugin tries to find the screenshot plugin through the super class.
It is once registered as its proper class and once as its super class
The old version doesn't always properly release resources. With the update, this anomaly doesn't occur anymore.
| PluginExecutionEngine.addPlugin(SingletonFactory.getInstance(plugin, args)); | ||
| } | ||
|
|
||
| public <T extends Plugin> void addPluginWithKey(Class key, Class<T> plugin, Object... args) { |
There was a problem hiding this comment.
what is the idea here - so far the framework only had one method for adding plugins and listeners. What is the need for another one?
There was a problem hiding this comment.
what is the idea here - so far the framework only had one method for adding plugins and listeners. What is the need for another one?
ScreenshotPlugin is extended by DesktopScreenshotPlugin, MobileScreenshotPlugin, and WebScreenshotPlugin. Inside the base test classes (AndroidTest, DesktopTest, IOSTest, WebTest), instead of registering the Screenshot plugins for each respective module by themselves (as their class as key), we register them by the base class - ScreenshotPlugin.
This is because VRT plugin doesn't have insight which module calls it; It only expects that the module has an implementation of the ScreenshotPlugin. That's why, it searches inside SingletonFactory's map by the key ScreenshotPlugin.
| package solutions.bellatrix.core.utilities; | ||
|
|
||
|
|
||
| public interface PageObjectModel<MapT, AssertsT> { |
There was a problem hiding this comment.
this is a major change - we need to discuss it, or extract in a separate PR
There was a problem hiding this comment.
this is a major change - we need to discuss it, or extract in a separate PR
All modules (android, ios, desktop, playwright, and web) have POM. It is a major building block of our framework.
From insights from a project which uses BELLATRIX, it seems convenient to search for a baseline by the name of a page object.
Here is the usage of the interface PageObjectModel; It couldn't be abstracted to simply Object, as it will interfere with other overload methods. It couldn't be abstracted to simply String, as it will interfere with other overload methods.
More information could be found at VisualRegressionAssertions file
The change is done due to the assumption that all Page Object Models inside projects would extend the provided "*Section" and "*Page" objects.
Another assumption which was made was that no major changes would occur to the "*Section" and "*Page" objects provided, but would rather be extended inside the project's module(s).
|
|
||
| static { | ||
| try { | ||
| VISUAL_REGRESSION_TRACKER_THREAD_LOCAL = new ThreadLocal<>(); |
There was a problem hiding this comment.
ThreadLocal.withInitial(() -> null); This is an alternative approach to achieving the same result. The key distinction is that it employs a static factory method, which is the standard convention for object creation in the Core Java API.
|
|
||
| public class VisualRegressionService { | ||
| private static float defaultDiffTolerance() { | ||
| return ConfigurationService.get(VisualRegressionSettings.class).getDefaultDiffTolerance(); |
There was a problem hiding this comment.
In my opinion, this should be a static field rather than a method, as the value remains constant and does not change. Repeatedly executing ConfigurationService.get(VisualRegressionSettings.class) for an immutable value introduces unnecessary overhead. By using a static field, we avoid redundant reflection calls, improving efficiency.
|
|
||
| public static String takeSnapshot(String name) { | ||
| var screenshotPlugin = SingletonFactory.getInstance(ScreenshotPlugin.class); | ||
| return Objects.requireNonNull(screenshotPlugin).takeScreenshot(name); |
There was a problem hiding this comment.
In my opinion, it would be beneficial to provide a more descriptive exception message to facilitate easier debugging. For example, ScreenshotPlugin is not registered. or something like that.
| import lombok.NoArgsConstructor; | ||
| import lombok.Setter; | ||
|
|
||
| @Getter @Setter @NoArgsConstructor |
There was a problem hiding this comment.
You can use directly @DaTa annotation instead of Getter and Setter.
No description provided.