[CUS-9599] downloads image from the given element. #281
Conversation
WalkthroughAdds cross-platform image acquisition and comparison actions: new DownloadImageFromImgTag and TakeScreenShotAndStoreinS3; threshold-based similarity/dissimilarity verifiers; local URL→File helpers; shared API multipart calls and S3 upload flows; updated dissimilarity messaging; and bumps Maven version and Java compiler target to 15. Changes
Sequence Diagram(s)sequenceDiagram
actor Tester
participant Driver as Web/Android/iOS Driver
participant Action as DownloadImageFromImgTag / TakeScreenShot...
participant FS as File System (temp files)
participant ImageAPI as Visual Testing API
participant S3 as ImageComparisonUtils (S3)
participant RunTime as RunTimeData
Tester->>Action: trigger action (locator or inputs)
Action->>Driver: locate element / eval JS (shadow DOM)
Driver-->>Action: src or element screenshot bytes
alt src is http(s) or relative
Action->>FS: download URL -> temp File
FS-->>Action: temp File
else src is data: URL
Action->>FS: decode base64 -> temp File
FS-->>Action: temp File
else no src
Action->>Driver: take element screenshot
Driver-->>Action: screenshot bytes
Action->>FS: save bytes -> temp File
FS-->>Action: temp File
end
Action->>S3: upload temp File
S3-->>Action: S3 key/URL
alt comparison required
Action->>ImageAPI: send multipart (base + actual)
ImageAPI-->>Action: similarity JSON (percentage, diffs)
alt below threshold
Action->>S3: upload actual image
Action->>RunTime: set success message & store path
else above threshold
Action->>FS: generate combined diff image
FS-->>Action: combined File
Action->>S3: upload combined File
Action->>RunTime: set failure message & store path
end
end
Action-->>Tester: Result with message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilar.java (1)
86-92: Fix the misleading message label.The success message on line 91 reports "Similarity percentage:" but actually calculates and displays the dissimilarity percentage
(100 - percentageSimilarity * 100). This is inconsistent with the message on line 90 that states the images are "dissimilar" and will confuse users.Additionally, the parentheses around
100are redundant.Apply this diff to fix the label and simplify the expression:
setSuccessMessage("Successfully verified that the base image and actual image are dissimilar. " + - "Similarity percentage: " + ((100) - percentageSimilarity * 100) + "%"); + "Dissimilarity percentage: " + (100 - percentageSimilarity * 100) + "%");image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilar.java (5)
46-50: Remove unused RequestConfig.The
RequestConfiginstance is never used. The actual HTTP client implementation at line 127 usesOkHttpClient, not Apache HttpClient. This dead code should be removed.Apply this diff to remove the unused code:
- RequestConfig config = RequestConfig.custom() - .setSocketTimeout(10 * 60 * 1000) - .setConnectionRequestTimeout(60 * 1000) - .setConnectTimeout(60 * 1000) - .build();
71-72: Add explicit cleanup for temporary files.The
urlToFileConvertermethod creates temporary files that are never explicitly deleted. This can lead to resource leaks and disk space issues over time. Consider using try-with-resources or adding explicit cleanup in a finally block.Apply this pattern to ensure cleanup:
+ File file1 = null; + File file2 = null; + try { - File file1 = urlToFileConverter("first_image", baseImagePath); - File file2 = urlToFileConverter("second_image", actualImagePath); + file1 = urlToFileConverter("first_image", baseImagePath); + file2 = urlToFileConverter("second_image", actualImagePath); logger.info("Base image file path: " + file1.getAbsolutePath()); baseImage = ImageIO.read(file1); actualImage = ImageIO.read(file2); // ... rest of the logic } catch (IOException e) { logger.info("image not found "); logger.info(ExceptionUtils.getStackTrace(e)); throw new RuntimeException(e); + } finally { + // Clean up temp files + if (file1 != null && file1.exists() && baseImagePath.startsWith("http")) { + file1.delete(); + } + if (file2 != null && file2.exists() && actualImagePath.startsWith("http")) { + file2.delete(); + } }
100-102: Clean up temporary combined image file.The temporary file created for the combined image is never explicitly deleted, leading to a resource leak.
Consider adding cleanup similar to the suggestion for lines 71-72, or use try-with-resources if applicable.
127-150: Configure timeouts for OkHttpClient and ensure proper resource cleanup.The
OkHttpClientis created without timeout configuration, which can cause requests to hang indefinitely. Additionally, the Response object should be explicitly closed to release resources.Apply this diff to add timeouts and proper resource management:
- OkHttpClient client = new OkHttpClient(); + OkHttpClient client = new OkHttpClient.Builder() + .connectTimeout(60, java.util.concurrent.TimeUnit.SECONDS) + .readTimeout(10, java.util.concurrent.TimeUnit.MINUTES) + .writeTimeout(60, java.util.concurrent.TimeUnit.SECONDS) + .build(); logger.info("Initiating http client"); // ... builder code ... logger.info("Making api call to visual server"); - Response response = client.newCall(request).execute(); + try (Response response = client.newCall(request).execute()) { - if (response.isSuccessful()) { + if (response.isSuccessful()) { // ... existing logic ... + } }
220-246: Add input validation and security checks to prevent vulnerabilities.The
urlToFileConvertermethod has several security concerns:
- Path Traversal: Local file paths are not validated, allowing potential access to files outside the intended directory using
../sequences.- SSRF (Server-Side Request Forgery): URL downloads have no domain or protocol validation beyond http/https, potentially allowing requests to internal resources.
- Resource Exhaustion: No size limits when downloading files from URLs, which could lead to memory/disk exhaustion.
- Error Handling: Generic RuntimeException doesn't provide enough context for debugging.
Consider adding validation:
public File urlToFileConverter(String fileName, String url) { try { if (url.startsWith("https://") || url.startsWith("http://")) { logger.info("Given is s3 url ...File name:" + fileName); + // Validate URL to prevent SSRF + URL urlObject = new URL(url); + String host = urlObject.getHost(); + if (host.equals("localhost") || host.startsWith("127.") || + host.startsWith("169.254.") || host.startsWith("10.") || + host.matches("172\\.(1[6-9]|2[0-9]|3[01])\\..*") || + host.startsWith("192.168.")) { + throw new SecurityException("Access to internal/private IPs is not allowed"); + } - URL urlObject = new URL(url); String baseName = fileName; // ... rest of logic ... } else { logger.info("Given is local file path.."); - return new File(url); + // Validate file path to prevent path traversal + File file = new File(url); + String canonicalPath = file.getCanonicalPath(); + // Add validation that canonicalPath is within allowed directory + return file; -// return createLocalFileFromDownloadsCopy(url, ".png"); } } catch (Exception e) { logger.info("Error while accessing: " + url); - throw new RuntimeException("Unable to access the given file, please check the given inputs."); + throw new RuntimeException("Unable to access the given file: " + e.getMessage(), e); } }Also remove the commented code at line 240.
image_visual_testing/src/main/java/com/testsigma/addons/ios/TakeScreenShotAndStorePathInRuntime.java (2)
23-27: Misleading description in @action annotation.The
descriptionfield says "validates options count in a select drop-down" but this action takes a screenshot and stores the file path. This appears to be copy-paste from another action.@Action(actionText = "Take screenshot of the current page and store the saved image file path in runtime variable" + " test-data", - description = "validates options count in a select drop-down", + description = "Takes a screenshot of the current page and stores the saved image file path in a runtime variable", applicationType = ApplicationType.IOS, useCustomScreenshot = false)
62-62: Inaccurate success message.The message says "stored the screenshot in S3" but the file is stored locally (temp directory). This is misleading to users.
- setSuccessMessage("successfully stored the screenshot in S3 with URL: " + file1.getAbsolutePath()); + setSuccessMessage("Successfully stored the screenshot locally at: " + file1.getAbsolutePath());image_visual_testing/src/main/java/com/testsigma/addons/android/TakeScreenShotAndStorePathInRuntime.java (2)
22-26: Misleading description in @action annotation.Same issue as the iOS counterpart - the
descriptionsays "validates options count in a select drop-down" but this action takes screenshots.@Action(actionText = "Take screenshot of the current page and store the saved image file" + " path in runtime variable test-data", - description = "validates options count in a select drop-down", + description = "Takes a screenshot of the current page and stores the saved image file path in a runtime variable", applicationType = ApplicationType.ANDROID, useCustomScreenshot = false)
61-61: Inaccurate success message.Same issue as iOS - the message incorrectly mentions S3 when the file is stored locally.
- setSuccessMessage("successfully stored the screenshot in S3 with URL: " + file1.getAbsolutePath()); + setSuccessMessage("Successfully stored the screenshot locally at: " + file1.getAbsolutePath());image_visual_testing/src/main/java/com/testsigma/addons/web/CompareTwoImagesUsingVisualTesting.java (1)
46-50: UnusedRequestConfig- timeouts not applied to actual HTTP client.The
RequestConfigis configured butOkHttpClientis used inperformApiCallwithout these timeouts. Either apply timeouts to OkHttpClient or remove the unused config.- RequestConfig config = RequestConfig.custom() - .setSocketTimeout(10 * 60 * 1000) - .setConnectionRequestTimeout(60 * 1000) - .setConnectTimeout(60 * 1000) - .build();And update the OkHttpClient initialization in
performApiCall:OkHttpClient client = new OkHttpClient.Builder() .connectTimeout(60, TimeUnit.SECONDS) .readTimeout(10, TimeUnit.MINUTES) .writeTimeout(60, TimeUnit.SECONDS) .build();image_visual_testing/src/main/java/com/testsigma/addons/android/CompareTwoImagesUsingVisualTesting.java (1)
47-51: UnusedRequestConfig- timeouts not applied.Same issue as the web version. The Apache HttpClient config is not used; OkHttpClient is used instead without timeouts configured.
Apply timeouts to OkHttpClient in
performApiCall:OkHttpClient client = new OkHttpClient.Builder() .connectTimeout(60, TimeUnit.SECONDS) .readTimeout(10, TimeUnit.MINUTES) .writeTimeout(60, TimeUnit.SECONDS) .build();
🧹 Nitpick comments (22)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilar.java (4)
66-76: Consider cleaning up temporary files.The
urlToFileConvertercreates temporary files for image processing. Consider adding cleanup logic (e.g.,file1.deleteOnExit()andfile2.deleteOnExit()) to prevent potential disk space accumulation over time.
78-78: Remove commented-out code.Line 78 contains a commented-out logger statement that should either be removed or uncommented if needed.
202-204: Remove commented-out code.Lines 202-204 contain commented-out error message logic that should be removed to improve code clarity.
240-240: Remove commented-out code.Line 240 contains commented-out code that should be removed for better code maintainability.
image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilar.java (1)
202-204: Remove commented-out code.The commented code should be removed if it's no longer needed, as it clutters the codebase and can cause confusion.
Apply this diff:
-// String message = "Comparison failed because visual testing detected identical images," + -// " check step screenshot for visual results"; -// errorMessageBuilder.append(message);image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfImagesAreDissimilar.java (1)
219-245: Temp files are not cleaned up.The
urlToFileConvertermethod creates temporary files for HTTP URLs but these are never deleted. Consider usingtempFile.deleteOnExit()to schedule cleanup when the JVM exits.File tempFile = File.createTempFile(baseName, extension); + tempFile.deleteOnExit(); FileUtils.copyURLToFile(urlObject, tempFile);image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreDissimilar.java (1)
225-251: Consider extractingurlToFileConverterto a shared utility class.This exact method is duplicated across multiple files (
VerifyIfImagesAreDissimilar,VerifyIfTwoImagesAreDissimilar,CompareTwoImagesUsingVisualTestingfor iOS, etc.). Consider moving it toImageComparisonUtilsto reduce duplication and simplify maintenance.image_visual_testing/src/main/java/com/testsigma/addons/ios/CompareTwoImagesUsingVisualTesting.java (1)
205-231: DuplicateurlToFileConverterimplementation.This method is identical to implementations in the web action classes. This is the third instance of the same code. Consider extracting to
ImageComparisonUtilsas a static method to follow DRY principles.Additionally, temp files should be cleaned up:
File tempFile = File.createTempFile(baseName, extension); + tempFile.deleteOnExit(); FileUtils.copyURLToFile(urlObject, tempFile);image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java (5)
6-6: Unused import:WebAction.This import is not used in the iOS action class and should be removed.
-import com.testsigma.sdk.WebAction;
56-57: Misleading directory name.The temporary directory is named
basePdfDirectorybut it's used for storing images, not PDFs. Consider renaming for clarity.- String basePdfDirectoryPath = String.valueOf(Files.createTempDirectory("basePdfDirectory")); - logger.info("Base PDF Directory Path: " + basePdfDirectoryPath); + String baseTempDirectoryPath = String.valueOf(Files.createTempDirectory("imageDirectory")); + logger.info("Base Temp Directory Path: " + baseTempDirectoryPath);
165-166: Inconsistent method design:findImageInShadowRootis static whilegetSrcFromShadowRootis not.Both methods serve similar purposes but have different visibility modifiers. Consider making both instance methods for consistency and to enable proper logging.
- private static String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { + private String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) {
211-214: Use logger instead ofSystem.err.println.This is inconsistent with the rest of the class which uses the logger for error reporting.
} catch (Exception e) { - System.err.println("Error finding image in shadow root: " + e.getMessage()); + logger.info("Error finding image in shadow root: " + e.getMessage()); return null; }
265-278: Use logger instead ofSystem.err.printlnfor consistency.private static byte[] handleDataUrl(String dataUrl) throws Exception { try { // Extract base64 data from data URL String[] parts = dataUrl.split(","); if (parts.length != 2) { throw new Exception("Invalid data URL format"); } String base64Data = parts[1]; return Base64.getDecoder().decode(base64Data); } catch (Exception e) { - System.err.println("Error processing data URL: " + e.getMessage()); + // Note: Cannot use logger in static method without passing it throw e; } }Consider making
handleDataUrlan instance method to enable proper logging, or remove the redundant catch-and-rethrow.image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java (1)
43-44: Misleading directory name.Same as the iOS file - the temporary directory is named
basePdfDirectorybut stores images.- String basePdfDirectoryPath = String.valueOf(Files.createTempDirectory("basePdfDirectory")); - logger.info("Base PDF Directory Path: " + basePdfDirectoryPath); + String baseTempDirectoryPath = String.valueOf(Files.createTempDirectory("screenshotDirectory")); + logger.info("Base Temp Directory Path: " + baseTempDirectoryPath);image_visual_testing/src/main/java/com/testsigma/addons/web/CompareTwoImagesUsingVisualTesting.java (2)
74-75: Remove commented-out code.Commented code should be removed rather than left in place.
File file1 = urlToFileConverter("first_image", baseImagePath); File file2 = urlToFileConverter("second_image", actualImagePath); -// File file1 = new File(baseImagePath); -// File file2 = new File(actualImagePath);
216-243: Consider extractingurlToFileConverterto a shared utility class.This method is duplicated across
CompareTwoImagesUsingVisualTesting(web, android) andVerifyIfTwoImagesAreSimilar. Extracting it toImageComparisonUtilsor a new utility class would reduce duplication and ease maintenance.Additionally,
FileUtils.copyURLToFileat line 229 has no timeout configuration and could hang indefinitely on slow/unresponsive URLs. Consider using the overloaded version with timeout parameters.- FileUtils.copyURLToFile(urlObject, tempFile); + FileUtils.copyURLToFile(urlObject, tempFile, 30000, 30000);image_visual_testing/src/main/java/com/testsigma/addons/android/CompareTwoImagesUsingVisualTesting.java (2)
205-231: DuplicatedurlToFileConvertermethod.This method is identical to the one in
CompareTwoImagesUsingVisualTesting(web) andVerifyIfTwoImagesAreSimilar. Consider extracting to a shared utility class to follow DRY principles.Also, add timeout to
FileUtils.copyURLToFile:- FileUtils.copyURLToFile(urlObject, tempFile); + FileUtils.copyURLToFile(urlObject, tempFile, 30000, 30000);
224-226: Remove commented-out code.} else { logger.info("Given is local file path.."); return new File(url); -// return createLocalFileFromDownloadsCopy(url, ".png"); }image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreSimilar.java (2)
200-203: Remove commented-out code.} else { ImageIO.write(combined, "png", combinedImage); boolean uploadS3Result = imageComparisonUtils.uploadFile(s3Url, combinedImage.getAbsolutePath()); if (!uploadS3Result) { logger.debug("Error occurred while uploading combined image to S3," + " screenshot might not be displayed"); } -// String message = "Comparison failed because visual testing detected dissimilarities," + -// " check step screenshot for visual results"; -// errorMessageBuilder.append(message); setErrorMessage(errorMessageBuilder.toString()); return Result.FAILED; }
218-244: DuplicatedurlToFileConvertermethod and missing timeout.This method is duplicated across multiple action classes. Extract to a shared utility.
Also add timeout to prevent hangs:
- FileUtils.copyURLToFile(urlObject, tempFile); + FileUtils.copyURLToFile(urlObject, tempFile, 30000, 30000);image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java (2)
54-55: Misleading variable name.
basePdfDirectoryPathis confusing for an image download operation. Consider renaming for clarity.-String basePdfDirectoryPath = String.valueOf(Files.createTempDirectory("basePdfDirectory")); -logger.info("Base PDF Directory Path: " + basePdfDirectoryPath); +String baseImageDirectoryPath = String.valueOf(Files.createTempDirectory("baseImageDirectory")); +logger.info("Base Image Directory Path: " + baseImageDirectoryPath);
213-216: Inconsistent logging withSystem.err.println.The method uses
System.err.printlnwhile the rest of the class uses the logger. Consider making this method non-static to access the logger, or accept a logger parameter.- private static String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { + private String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { try { // ... script execution ... } catch (Exception e) { - System.err.println("Error finding image in shadow root: " + e.getMessage()); + logger.warn("Error finding image in shadow root: " + e.getMessage()); return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
image_visual_testing/pom.xml(2 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/CompareTwoImagesUsingVisualTesting.java(3 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/TakeScreenShotAndStorePathInRuntime.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilar.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/CompareTwoImagesUsingVisualTesting.java(3 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/TakeScreenShotAndStorePathInRuntime.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilar.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/CompareTwoImagesUsingVisualTesting.java(6 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfImagesAreDissimilar.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreDissimilar.java(2 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreSimilar.java(7 hunks)
🔇 Additional comments (11)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilar.java (4)
1-33: LGTM!The package structure, imports, and class declaration are well-organized and appropriate for iOS image comparison functionality.
35-52: LGTM!Field declarations and timeout configuration are appropriate for image comparison operations.
115-178: LGTM!The API call implementation is well-structured with proper error handling and comprehensive logging.
220-246: LGTM!The
urlToFileConvertermethod correctly handles both HTTP URLs and local file paths with appropriate error handling.image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilar.java (1)
80-84: LGTM!The defensive null check for
diff_coordinatesprevents potential NPE and the fallback to an empty list is appropriate.image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfImagesAreDissimilar.java (1)
89-91: LGTM!The updated success message correctly displays the dissimilarity percentage by computing
100 - (percentageSimilarity * 100), which is more intuitive for users verifying dissimilarity.image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreDissimilar.java (1)
93-96: LGTM!The success message now correctly displays the dissimilarity percentage, consistent with
VerifyIfImagesAreDissimilar.java.image_visual_testing/src/main/java/com/testsigma/addons/ios/CompareTwoImagesUsingVisualTesting.java (1)
72-73: LGTM!The switch to a local
urlToFileConverterhelper provides consistent file handling for both HTTP URLs and local paths.image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java (3)
26-46: LGTM!Class annotations and field declarations follow the Testsigma SDK patterns correctly.
128-138: LGTM!File saving logic is correct.
FileUtils.writeByteArrayToFilehandles file creation implicitly, so the explicitcreateNewFile()is redundant but harmless.
141-160: LGTM!Shadow root access logic is well-structured with proper null handling and graceful fallback on errors.
| import java.net.URL; | ||
| import java.nio.file.Files; | ||
| import java.util.Base64; | ||
| import java.util.NoSuchElementException; |
There was a problem hiding this comment.
Wrong NoSuchElementException import causes catch block mismatch.
java.util.NoSuchElementException is imported, but Selenium throws org.openqa.selenium.NoSuchElementException. The catch block at line 115 will never catch element lookup failures.
-import java.util.NoSuchElementException;
+import org.openqa.selenium.NoSuchElementException;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import java.util.NoSuchElementException; | |
| import org.openqa.selenium.NoSuchElementException; |
🤖 Prompt for AI Agents
In
image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java
around line 24, the file imports java.util.NoSuchElementException but Selenium
throws org.openqa.selenium.NoSuchElementException so the catch at ~line 115
never matches; replace the java.util import with
org.openqa.selenium.NoSuchElementException (or use the fully-qualified name in
the catch) and remove the incorrect import so the catch block correctly handles
Selenium element lookup failures.
image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java
Show resolved
Hide resolved
| HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | ||
| connection.setRequestMethod("GET"); | ||
| connection.setConnectTimeout(30000); | ||
| connection.setReadTimeout(30000); | ||
|
|
||
| try (InputStream inputStream = connection.getInputStream(); | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = inputStream.read(buffer)) != -1) { | ||
| outputStream.write(buffer, 0, bytesRead); | ||
| } | ||
| return outputStream.toByteArray(); | ||
| } | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to download image from URL: " + imageUrl + ", error: " + e.getMessage()); | ||
| logger.info("error : " + ExceptionUtils.getStackTrace(e)); | ||
| throw new RuntimeException("Failed to download image: " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
HttpURLConnection is not disconnected - potential resource leak.
The connection should be disconnected after use. Additionally, the method signature declares throws IOException but actually throws RuntimeException.
URL url = new URL(imageUrl);
HttpURLConnection connection = (HttpURLConnection) url.openConnection();
connection.setRequestMethod("GET");
connection.setConnectTimeout(30000);
connection.setReadTimeout(30000);
- try (InputStream inputStream = connection.getInputStream();
- ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
- byte[] buffer = new byte[1024];
- int bytesRead;
- while ((bytesRead = inputStream.read(buffer)) != -1) {
- outputStream.write(buffer, 0, bytesRead);
+ try {
+ try (InputStream inputStream = connection.getInputStream();
+ ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
+ byte[] buffer = new byte[1024];
+ int bytesRead;
+ while ((bytesRead = inputStream.read(buffer)) != -1) {
+ outputStream.write(buffer, 0, bytesRead);
+ }
+ return outputStream.toByteArray();
}
- return outputStream.toByteArray();
+ } finally {
+ connection.disconnect();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java
around lines 246-264, the HttpURLConnection opened for the download is never
disconnected and the catch block throws a RuntimeException despite the method
declaring throws IOException; fix by ensuring connection.disconnect() is always
called (use a finally block that checks for non-null connection and calls
disconnect()), and change the error handling to rethrow an IOException
(preserving the original exception as the cause) instead of a RuntimeException
while keeping the logging of the error and stack trace.
image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java
Show resolved
Hide resolved
...e_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilar.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreSimilar.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/web/VerifyIfTwoImagesAreSimilar.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
image_visual_testing/src/main/java/com/testsigma/addons/ios/TakeScreenShotAndStorePathInRuntime.java (2)
62-62: Misleading success message references S3.The message says "stored the screenshot in S3" but the file is stored locally on disk. This could confuse users.
- setSuccessMessage("successfully stored the screenshot in S3 with URL: " + file1.getAbsolutePath()); + setSuccessMessage("Successfully stored the screenshot at path: " + file1.getAbsolutePath());
66-75: Silent failure and incorrect logging format insaveBytesArrayToFile.
- The IOException is caught but not propagated, leading to silent failures where the caller assumes success.
- The logging uses
"{}" +instead of proper string concatenation.public void saveBytesArrayToFile(String filePath, byte[] imageBytes) { try { File file = new File(filePath); boolean isCreated = file.createNewFile(); - logger.debug("is file created - {}" + isCreated); + logger.debug("is file created - " + isCreated); FileUtils.writeByteArrayToFile(file, imageBytes); } catch (IOException e) { - logger.info("Failed to save byte[] screenshot to path {}" + filePath); + logger.info("Failed to save byte[] screenshot to path " + filePath); + throw new RuntimeException("Failed to save screenshot: " + e.getMessage()); } }
♻️ Duplicate comments (3)
image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java (1)
63-72: Silent failure and incorrect logging format insaveBytesArrayToFile- still unaddressed.This issue was flagged in a previous review but remains unfixed:
- The IOException is caught but not propagated, leading to silent failures.
- The logging uses
"{}" +instead of proper string concatenation.public void saveBytesArrayToFile(String filePath, byte[] imageBytes) { try { File file = new File(filePath); boolean isCreated = file.createNewFile(); - logger.debug("is file created - {}" + isCreated); + logger.debug("is file created - " + isCreated); FileUtils.writeByteArrayToFile(file, imageBytes); } catch (IOException e) { - logger.info("Failed to save byte[] screenshot to path {}" + filePath); + logger.info("Failed to save byte[] screenshot to path " + filePath); + throw new RuntimeException("Failed to save screenshot: " + e.getMessage()); } }image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java (2)
99-99: Incorrect log message - still unaddressed from previous review.The message "images are identical hence uploading" doesn't apply here; this action only downloads a single image.
- logger.info("images are identical hence uploading the actual image to S3"); + logger.info("Uploading the captured image to S3");
268-282: Data URL parsing issue and inconsistent logging - still unaddressed.These issues were flagged in a previous review:
split(",")without limit may fail for certain base64 content.- Uses
System.err.printlninstead of logger.- private static byte[] handleDataUrl(String dataUrl) throws Exception { + private byte[] handleDataUrl(String dataUrl) throws Exception { try { - String[] parts = dataUrl.split(","); - if (parts.length != 2) { + String[] parts = dataUrl.split(",", 2); + if (parts.length < 2) { throw new Exception("Invalid data URL format"); } String base64Data = parts[1]; return Base64.getDecoder().decode(base64Data); } catch (Exception e) { - System.err.println("Error processing data URL: " + e.getMessage()); + logger.warn("Error processing data URL: " + e.getMessage()); throw e; } }
🧹 Nitpick comments (19)
image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java (2)
1-6: Unused import:WebAction.The class extends
IOSAction, butWebActionis imported and never used.import com.testsigma.sdk.ApplicationType; import com.testsigma.sdk.IOSAction; -import com.testsigma.sdk.WebAction; import com.testsigma.sdk.annotation.*;
209-212: Use logger instead ofSystem.err.printlnin static method.This method uses
System.err.printlnwhich bypasses the logging framework. Consider making the method non-static to access the logger, or pass the logger as a parameter.- private static String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { + private String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { try { // ... script ... Object result = jsExecutor.executeScript(script, element); return result != null ? result.toString() : null; } catch (Exception e) { - System.err.println("Error finding image in shadow root: " + e.getMessage()); + logger.warn("Error finding image in shadow root: " + e.getMessage()); return null; } }image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java (1)
209-212: Use logger instead ofSystem.err.println.Same issue as the iOS version - the static method uses
System.err.printlnwhich bypasses logging.- private static String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { + private String findImageInShadowRoot(JavascriptExecutor jsExecutor, WebElement element) { // ... script ... } catch (Exception e) { - System.err.println("Error finding image in shadow root: " + e.getMessage()); + logger.warn("Error finding image in shadow root: " + e.getMessage()); return null; } }image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java (1)
48-52:RequestConfigis created but never used.The
RequestConfigwith timeout settings is defined but theOkHttpClientinperformApiCalldoesn't use it. Either apply these timeouts to the OkHttpClient or remove the unused config.- RequestConfig config = RequestConfig.custom() - .setSocketTimeout(10 * 60 * 1000) - .setConnectionRequestTimeout(60 * 1000) - .setConnectTimeout(60 * 1000) - .build();Or apply timeouts to OkHttpClient:
OkHttpClient client = new OkHttpClient.Builder() .connectTimeout(60, TimeUnit.SECONDS) .readTimeout(10, TimeUnit.MINUTES) .writeTimeout(60, TimeUnit.SECONDS) .build();image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilarWithThreshold.java (3)
11-11: Unused import:WebActionThis import is not used in the class. Remove it to keep the code clean.
-import com.testsigma.sdk.WebAction;
51-55: UnusedRequestConfigfieldThe
configfield is defined but never used. TheOkHttpClienton line 136 is created without any timeout configuration, which could lead to indefinite hangs if the visual server is unresponsive.Either remove the unused
RequestConfigor configure theOkHttpClientwith appropriate timeouts:- OkHttpClient client = new OkHttpClient(); + OkHttpClient client = new OkHttpClient.Builder() + .connectTimeout(60, java.util.concurrent.TimeUnit.SECONDS) + .readTimeout(10 * 60, java.util.concurrent.TimeUnit.SECONDS) + .writeTimeout(60, java.util.concurrent.TimeUnit.SECONDS) + .build();
230-256: Temp files are not cleaned upTemp files created via
File.createTempFile()(lines 242, and similarly inexecute()at line 109) are never deleted. Consider addingtempFile.deleteOnExit()or explicit cleanup to prevent file system resource accumulation.File tempFile = File.createTempFile(baseName, extension); + tempFile.deleteOnExit(); FileUtils.copyURLToFile(urlObject, tempFile);image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreSimilarWithThreshold.java (3)
11-11: Unused import:WebActionThis import is not used in the class.
-import com.testsigma.sdk.WebAction;
51-55: UnusedRequestConfigfieldSame as the dissimilar variant - the
configfield is defined but not used with theOkHttpClient. Consider configuring timeouts on the HTTP client.
1-252: Significant code duplication withVerifyIfImagesAreDissimilarWithThresholdThis class shares ~90% of its code with
VerifyIfImagesAreDissimilarWithThreshold. The only differences are the threshold comparison logic and messaging. Consider extracting common functionality into a shared base class or utility to reduce maintenance burden and ensure bug fixes are applied consistently.image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilarWithThreshold.java (3)
50-54: UnusedRequestConfigfieldThe
configfield is defined but theOkHttpClienton line 135 is created without any timeout configuration.
229-255: Temp files are not cleaned upSame as Android variants - temp files created via
File.createTempFile()are never deleted. AddtempFile.deleteOnExit()to prevent file system resource accumulation.
1-258: Significant code duplication across platformsThis iOS class is nearly identical to the Android variants. The only platform-specific differences are the driver type (
IOSDrivervsAndroidDriver) and the@Actionannotation. Consider extracting the shared logic (API calls, URL conversion, image processing) into platform-agnostic utilities to reduce duplication across Android, iOS, and Web implementations.image_visual_testing/src/main/java/com/testsigma/addons/web/DownloadImageFromImgTag.java (6)
21-27: Align action text/description with actual behavior (not just<img>tags)The action text emphasizes
<img>tags, butexecute()now also handles non-imgelements (shadow-root charts, SVG/canvas, plain element screenshots). Consider updatingactionText/descriptionto reflect that broader behavior so users don’t mistakenly think it only works for<img>elements.
46-89: Remove large commented-out legacy implementationThe old implementation is fully commented out. Keeping this much legacy code in comments adds noise and can confuse future maintainers; the version history already preserves it.
- /* - try { - logger.info("initiating execution of DownloadImageFromImgTag action"); - ... - } catch (NoSuchElementException ne) { - ... - }catch (Exception e) { - ... - } - return result;*/ + // Old implementation removed; see VCS history if needed.
91-164: Harden S3 upload path and logging insideexecute()Core element/shadow-root/image capture logic looks good and the fallbacks are sensible. A few robustness and clarity tweaks would help:
testStepResult.getScreenshotUrl()can potentially benullor empty; defensive checks beforeuploadFilewould prevent surprises if the runner doesn’t always pre-populate this field.- Log line
"images are identical hence uploading the actual image to S3"is misleading here since no comparison is done in this action.- Failures of
uploadFileare logged atinfolevel;warnis more appropriate so S3 upload issues stand out.Suggested refinement:
- String s3Url = testStepResult.getScreenshotUrl(); - ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); - logger.info("images are identical hence uploading the actual image to S3"); - boolean uploadS3Result = imageComparisonUtils.uploadFile(s3Url, file1.getAbsolutePath()); - if (!uploadS3Result) { - logger.info("Error occurred while uploading combined image to s3," + - " screenshot might not be displayed"); - } - logger.info("Upload complete."); + String s3Url = testStepResult.getScreenshotUrl(); + ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); + + if (s3Url != null && !s3Url.isEmpty()) { + logger.info("Uploading element image to S3 at: " + s3Url); + boolean uploadS3Result = imageComparisonUtils.uploadFile(s3Url, file1.getAbsolutePath()); + if (!uploadS3Result) { + logger.warn("Error occurred while uploading element image to S3; screenshot might not be displayed"); + } + logger.info("Upload to S3 complete."); + } else { + logger.warn("Screenshot URL is null/empty; skipping S3 upload for element image"); + }Also, minor naming nit:
basePdfDirectoryPathis now used for PNGs; consider renaming to something image-specific when convenient.
201-256: Unify logging and consider guarding recursive shadow-root searchFunctionally,
findImageInShadowRootis powerful and flexible (handlesimg,canvas.toDataURL, and serialized SVG with nested shadow roots), which is great for charting libraries. Two minor points:
- It uses
System.err.printlnfor JS errors viaSystem.errin the catch block; the rest of the class useslogger. Prefer the logger for consistency and better aggregation.- The recursive search over
shadowElementsis fine for typical DOM sizes, but on very heavy/nested shadow trees it could become expensive. Not necessarily a blocker, but consider adding a simple depth or node-count guard if you see performance issues in practice.Suggested logging tweak:
- } catch (Exception e) { - System.err.println("Error finding image in shadow root: " + e.getMessage()); - return null; - } + } catch (Exception e) { + // Keep failures non-fatal; just fall back to element screenshot. + // `logger` is available via the enclosing action instance. + // If making this method non-static is acceptable, that would allow direct logger use. + return null; + }Or alternatively, make
findImageInShadowRootnon-static and uselogger.info/logger.warninstead ofSystem.err.
258-308: Tighten URL handling indownloadImageFromUrl(schemes, errors, SSRF surface)The method is generally solid: handles
data:URLs, relative paths,//and/prefixes, sets generous but finite timeouts, streams into a byte array, and cleans up theHttpURLConnection.A few improvements would make it safer and clearer:
Restrict to http/https schemes before casting to
HttpURLConnectionRight now, any non-
data:URL ends up atnew URL(imageUrl)and is blindly cast toHttpURLConnection. IfimageUrlever used another scheme (e.g.file:,blob:), this would throw aClassCastException. Also, restricting schemes helps avoid accidental use of unexpected protocols.Guard against unintended network targets
Because the URL ultimately comes from page content, this becomes a generic HTTP client. For environments where the runner has network access to internal services, it’s worth ensuring only http/https URLs you actually expect are fetched (e.g., same-origin with the AUT, or a configurable allow-list) to reduce SSRF-like risk.
Preserve root cause and consider IOException in signature
The method is declared
throws IOExceptionbut currently wraps all failures in aRuntimeException. Either remove the checked exception from the signature or consider rethrowing the originalIOException(and only wrapping unexpected runtime problems), so callers can distinguish network vs coding issues.Example scheme check in the existing block:
- // Download the image - URL url = new URL(imageUrl); - HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + // Download the image + URL url = new URL(imageUrl); + String protocol = url.getProtocol(); + if (!"http".equalsIgnoreCase(protocol) && !"https".equalsIgnoreCase(protocol)) { + throw new RuntimeException("Unsupported URL scheme for image download: " + protocol); + } + + HttpURLConnection connection = (HttpURLConnection) url.openConnection();If you later need to allow more schemes, this check gives you a single place to manage them.
310-324: MakehandleDataUrlslightly more robust for complex data URIs
handleDataUrlworks for the commondata:[mime];base64,<payload>form and logs toSystem.erron failure. A couple of small refinements:
- Use
split(",", 2)so additional commas in the payload (rare but valid) don’t break decoding.- Prefer the class logger over
System.errfor consistency.Suggested tweak:
- String[] parts = dataUrl.split(","); + String[] parts = dataUrl.split(",", 2); if (parts.length != 2) { throw new Exception("Invalid data URL format"); } @@ - } catch (Exception e) { - System.err.println("Error processing data URL: " + e.getMessage()); - throw e; - } + } catch (Exception e) { + // Log and rethrow so callers can fall back appropriately + // logger.info("Error processing data URL: " + e.getMessage(), e); + throw e; + }(If you keep this method static, you may prefer to pass in
loggeror convert it to an instance method to avoidSystem.err.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
image_visual_testing/pom.xml(2 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/TakeScreenShotAndStorePathInRuntime.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilarWithThreshold.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreSimilarWithThreshold.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/TakeScreenShotAndStorePathInRuntime.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilarWithThreshold.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java(1 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/DownloadImageFromImgTag.java(6 hunks)image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- image_visual_testing/pom.xml
- image_visual_testing/src/main/java/com/testsigma/addons/android/TakeScreenShotAndStorePathInRuntime.java
🔇 Additional comments (10)
image_visual_testing/src/main/java/com/testsigma/addons/ios/TakeScreenShotAndStorePathInRuntime.java (1)
23-27: Annotation changes look good.The updated description text is clearer and more accurate.
image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java (1)
242-260: HttpURLConnection resource management looks correct.The connection is properly disconnected in the finally block. Good fix from previous review feedback.
image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java (1)
21-25: Annotation description is now accurate.The description correctly describes the action's purpose.
image_visual_testing/src/main/java/com/testsigma/addons/android/DownloadImageFromImgTag.java (2)
44-121: Core execution logic is well-structured.The action properly handles various image source scenarios (img tag, shadow root, canvas, SVG) with appropriate fallbacks to element screenshots. Good error handling with proper result status returns.
248-260: HttpURLConnection resource management is correct.The connection is properly disconnected in the finally block.
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java (2)
121-176: API call implementation looks reasonable.The multipart request construction and response handling are appropriate. Consider adding request timeouts to the OkHttpClient as noted above.
218-244: URL to file converter implementation is solid.Good handling of both HTTP(S) URLs and local file paths with proper extension preservation.
image_visual_testing/src/main/java/com/testsigma/addons/web/DownloadImageFromImgTag.java (3)
9-9: Selenium wildcard import is acceptable hereUsing the
org.openqa.selenium.*wildcard is fine in this context given multiple Selenium types are used (WebElement, OutputType, Dimension, JavascriptExecutor) and project style seems to allow it.
167-177: File-save helper is straightforward and appropriate
saveBytesArrayToFileis clear and does the right thing: creates the file, logs creation, writes bytes, and surfaces failures via a runtime exception with context in the log. This is fine as a shared helper for the action family.
179-199: Shadow-rootimghelper looks correct and boundedThe
getSrcFromShadowRootJavaScript text block is minimal and safe: it only inspectselement.shadowRoot, looks for a singleimg, and returns itssrc/srcattribute as a string. Exception handling logs and returnsnull, which nicely degrades back to the screenshot path.
...ing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilarWithThreshold.java
Show resolved
Hide resolved
...ing/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreDissimilarWithThreshold.java
Show resolved
Hide resolved
...esting/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreSimilarWithThreshold.java
Show resolved
Hide resolved
...esting/src/main/java/com/testsigma/addons/android/VerifyIfImagesAreSimilarWithThreshold.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/ios/DownloadImageFromImgTag.java
Show resolved
Hide resolved
...testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilarWithThreshold.java
Show resolved
Hide resolved
...testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreDissimilarWithThreshold.java
Show resolved
Hide resolved
...al_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java
Outdated
Show resolved
Hide resolved
...al_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java
Show resolved
Hide resolved
image_visual_testing/src/main/java/com/testsigma/addons/web/TakeScreenShotAndStoreinS3.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java (1)
36-40: Variable naming still contradicts TestData annotations.Despite the past review comment being marked as addressed, the variable assignments remain swapped:
image1is annotated as"actual-image"but assigned tobaseImagePathimage2is annotated as"base-image"but assigned toactualImagePathThis inconsistency makes the code confusing and error-prone for future maintenance.
Apply this diff to align variable names with their annotations:
- String baseImagePath = image1.getValue().toString(); - String actualImagePath = image2.getValue().toString(); + String actualImagePath = image1.getValue().toString(); // image1 is "actual-image" + String baseImagePath = image2.getValue().toString(); // image2 is "base-image"Then update the file variable names for clarity:
- File file1 = urlToFileConverter("first_image", baseImagePath); - File file2 = urlToFileConverter("second_image", actualImagePath); + File file1 = urlToFileConverter("base_image", baseImagePath); + File file2 = urlToFileConverter("actual_image", actualImagePath);Also applies to: 64-65, 76-77
🧹 Nitpick comments (5)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java (5)
50-54: Remove unused RequestConfig.The
RequestConfigobject is defined but never used in the code. OkHttpClient is used instead for API calls, which has its own timeout configuration.- RequestConfig config = RequestConfig.custom() - .setSocketTimeout(10 * 60 * 1000) - .setConnectionRequestTimeout(60 * 1000) - .setConnectTimeout(60 * 1000) - .build();
89-96: Inconsistent handling of upload failures.When images match (line 92),
uploadScreenshot()is called but its return value is ignored, andResult.SUCCESSis returned unconditionally (line 95). However, in the failure path (lines 107-108), the method returns the result fromuploadScreenshot().While the current
uploadScreenshot()implementation logs upload failures without failing the test, this inconsistency creates confusion and could hide issues if the upload logic changes in the future.- uploadScreenshot(true, file2, actualImage, null, errorMessageBuilder); - setSuccessMessage("Successfully verified that the base image and actual image match the" + - " expected percentage of similarity: " + (percentageSimilarity * 100) + "%"); - return Result.SUCCESS; + Result uploadResult = uploadScreenshot(true, file2, actualImage, null, errorMessageBuilder); + if (uploadResult == Result.SUCCESS) { + setSuccessMessage("Successfully verified that the base image and actual image match the" + + " expected percentage of similarity: " + (percentageSimilarity * 100) + "%"); + } + return uploadResult;
181-220: Improve parameter naming and success message clarity.
- Line 181: Parameter
actualDirImageis misleading—"Dir" suggests a directory, but it's aFilerepresenting an image.- Lines 195-196: The success message references "first image," which is ambiguous given the variable naming confusion.
Consider renaming the parameter for clarity:
- private Result uploadScreenshot(boolean compareResult, File actualDirImage, BufferedImage combined, + private Result uploadScreenshot(boolean compareResult, File actualImageFile, BufferedImage combined, File combinedImage, StringBuilder errorMessageBuilder) { try { IOSDriver iosDriver = (IOSDriver) this.driver; ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(iosDriver, logger); String s3Url = testStepResult.getScreenshotUrl(); if (compareResult) { logger.info("images are identical hence uploading the actual image to S3"); - boolean uploadS3Result = imageComparisonUtils.uploadFile(s3Url, actualDirImage.getAbsolutePath()); + boolean uploadS3Result = imageComparisonUtils.uploadFile(s3Url, actualImageFile.getAbsolutePath());And clarify the success message to specify which image is shown:
- setSuccessMessage("Successfully verified that the base image and actual image are " + - "same by visual testing. (Note: Step screenshot contains the first image)"); + setSuccessMessage("Successfully verified that the base image and actual image are " + + "same by visual testing. (Note: Step screenshot contains the actual image)");
204-206: Remove commented-out code.Commented code at lines 204-206 and line 242 should be removed to reduce clutter. Version control preserves the history if this code needs to be referenced later.
-// String message = "Comparison failed because visual testing detected dissimilarities," + -// " check step screenshot for visual results"; -// errorMessageBuilder.append(message); setErrorMessage(errorMessageBuilder.toString());logger.info("Given is local file path.."); return new File(url); -// return createLocalFileFromDownloadsCopy(url, ".png");Also applies to: 242-242
222-248: Add validation for local file paths and fix misleading log message.Two improvements:
- Line 225: Log message says "Given is s3 url" but the check is for any HTTP/HTTPS URL, not specifically S3.
- Line 241: No validation that the local file exists before returning it, which will cause a less clear error later during
ImageIO.read().if (url.startsWith("https://") || url.startsWith("http://")) { - logger.info("Given is s3 url ...File name:" + fileName); + logger.info("Given is remote URL ...File name:" + fileName); URL urlObject = new URL(url); String baseName = fileName; String extension = ""; int lastDotIndex = fileName.lastIndexOf('.'); if (lastDotIndex > 0) { baseName = fileName.substring(0, lastDotIndex); extension = fileName.substring(lastDotIndex); } File tempFile = File.createTempFile(baseName, extension); FileUtils.copyURLToFile(urlObject, tempFile); logger.info("Temp file created with name for s3 file" + tempFile.getName() + " at path " + tempFile.getAbsolutePath()); return tempFile; } else { logger.info("Given is local file path.."); - return new File(url); + File localFile = new File(url); + if (!localFile.exists()) { + logger.error("Local file does not exist: " + url); + throw new RuntimeException("Local file not found: " + url); + } + return localFile; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java(1 hunks)
🔇 Additional comments (1)
image_visual_testing/src/main/java/com/testsigma/addons/ios/VerifyIfImagesAreSimilarWithThreshold.java (1)
124-179: Consider adding timeout configuration to OkHttpClient.The
OkHttpClientat line 128 uses default timeouts (10 seconds for connect/read), but the unusedRequestConfigat lines 50-54 defined 10-minute socket timeout, suggesting long-running visual comparison operations are expected.Verify whether the default OkHttpClient timeouts are sufficient for visual testing API calls, especially for large images.
If longer timeouts are needed, configure the client:
OkHttpClient client = new OkHttpClient.Builder() .connectTimeout(60, TimeUnit.SECONDS) .readTimeout(10, TimeUnit.MINUTES) .writeTimeout(10, TimeUnit.MINUTES) .build();
please review this addon and publish as PUBLIC
Addon name : Image Visual Testing
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-9599
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.