[CUS-10699] CREATED AN ADDON TO DISPLAY STRING AS OUTPUT SCREENSHOT..#339
Open
ManojTestsigma wants to merge 1 commit intodevfrom
Open
[CUS-10699] CREATED AN ADDON TO DISPLAY STRING AS OUTPUT SCREENSHOT..#339ManojTestsigma wants to merge 1 commit intodevfrom
ManojTestsigma wants to merge 1 commit intodevfrom
Conversation
|
|
📝 WalkthroughWalkthroughIntroduces a new Testsigma addon module for converting test data strings to images and uploading them as step result screenshots. Includes Maven project configuration, text-to-image conversion utility, S3 file upload utility, and a web action that orchestrates these components. Changes
Sequence DiagramsequenceDiagram
participant TestRunner as Test Runner
participant Action as DisplayTestDataAsStepResult<br/>ScreenShot
participant Converter as StringToImageConverter
participant Uploader as ImageComparisonUtils
participant S3 as S3 (via HTTP)
TestRunner->>Action: execute()
activate Action
Action->>Action: Read `@TestData`
Action->>Converter: convertToFile(testData)
activate Converter
Converter->>Converter: Render text to BufferedImage
Converter->>Converter: Write to temporary PNG file
Converter-->>Action: Return File
deactivate Converter
alt Screenshot URL exists
Action->>Uploader: uploadFile(s3SignedURL, filePath)
activate Uploader
Uploader->>Uploader: Build HTTP PUT request
Uploader->>S3: Upload file via HTTP
S3-->>Uploader: 200 OK
Uploader-->>Action: Return true
deactivate Uploader
Action->>Action: Set success result
else No screenshot URL
Action->>Action: Set error result
end
Action->>Action: Schedule file cleanup
Action-->>TestRunner: Return Result
deactivate Action
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
string_to_image_action/pom.xml (1)
71-90:okhttpandpoiappear to be unused — consider removing them.Neither
com.squareup.okhttp3:okhttpnororg.apache.poi:poiare imported or referenced in any source file in this module. Dead dependencies inflate the shaded JAR size and unnecessarily expand the dependency attack surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@string_to_image_action/pom.xml` around lines 71 - 90, The pom currently declares unused dependencies com.squareup.okhttp3:okhttp and org.apache.poi:poi; remove those two <dependency> blocks (identify by groupId/artifactId okhttp and poi) from string_to_image_action/pom.xml, then run a quick check (mvn dependency:analyze or a project-wide import/search) to confirm no code references remain and rebuild to ensure the shaded JAR size shrinks and no compile errors occur.string_to_image_action/src/main/java/com/testsigma/addons/util/StringToImageConverter.java (3)
44-44:Math.max(40, ...)forimageHeightis dead code — the floor can never be reached.
2 * PADDING = 80, solines.size() * lineHeight + 80is always ≥ 80 + (minimum line height) > 40. The literal40floor is never effective and may confuse future readers.♻️ Proposed fix
- int imageHeight = Math.max(40, lines.size() * lineHeight + 2 * PADDING); + int imageHeight = lines.size() * lineHeight + 2 * PADDING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@string_to_image_action/src/main/java/com/testsigma/addons/util/StringToImageConverter.java` at line 44, The Math.max floor of 40 in the imageHeight calculation is unreachable because 2 * PADDING already makes the expression > 40; update the computation in StringToImageConverter to remove the dead Math.max and compute imageHeight directly as lines.size() * lineHeight + 2 * PADDING (referencing the imageHeight variable, lines, lineHeight, and PADDING), or if a minimum height is desired use a meaningful threshold > 2 * PADDING; adjust only the expression that sets imageHeight.
143-143:List.of(" ")fallback returns an immutable list inconsistent with the normal mutableArrayListpath.The guard
lines.isEmpty()appears unreachable:text.isEmpty()at Line 102 already returns anArrayListfor the empty-string case, solinesis never empty when execution reaches Line 143. If the guard is kept for safety,List.of(" ")should be replaced with a mutablenew ArrayList<>containing" "so callers receive a consistent type.♻️ Proposed fix
- return lines.isEmpty() ? List.of(" ") : lines; + if (lines.isEmpty()) { lines.add(" "); } // defensive; should be unreachable + return lines;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@string_to_image_action/src/main/java/com/testsigma/addons/util/StringToImageConverter.java` at line 143, The current return uses an immutable List.of(" ") when lines.isEmpty(), causing an inconsistent immutable type compared to the usual mutable ArrayList returned earlier (see variable lines and the text.isEmpty() check in StringToImageConverter); change the fallback to return a mutable ArrayList containing a single " " element instead of List.of(" "), and consider removing or documenting the unreachable guard since text.isEmpty() already handles the empty-string path in this class/method.
81-89:Math.max(MIN_FONT_SIZE, 14)is always14—MIN_FONT_SIZEguard is redundant.
MIN_FONT_SIZE = 12 < 14, soMath.max(12, 14)always returns 14. If the intent is to allow the minimum to be tuned via the constant, the literal14should be replaced withMIN_FONT_SIZEand theMath.maxremoved, or the constant should be updated to reflect the actual minimum.♻️ Proposed fix
- return Math.max(MIN_FONT_SIZE, 14); + return MIN_FONT_SIZE; // update MIN_FONT_SIZE to 14 if that is the intended lower bound🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@string_to_image_action/src/main/java/com/testsigma/addons/util/StringToImageConverter.java` around lines 81 - 89, The computeFontSize method uses Math.max(MIN_FONT_SIZE, 14) which is redundant because MIN_FONT_SIZE is 12 < 14; update the tail case in computeFontSize to use the intended configurable minimum: replace the expression Math.max(MIN_FONT_SIZE, 14) with MIN_FONT_SIZE (or, if you intended the minimum to be 14, update the MIN_FONT_SIZE constant to 14) so the final return respects the constant; refer to the computeFontSize method and the MIN_FONT_SIZE constant when making the change.string_to_image_action/src/main/java/com/testsigma/addons/util/ImageComparisonUtils.java (1)
3-24: Strip unused imports and unuseddriverfield.All AWT/IO/Selenium imports (Lines 13–24) and
FileUtils,Base64,Listare unused — likely copied from another utility. Similarly,WebDriver driveris injected and stored (Lines 27–33) but never referenced in any method of this class.♻️ Proposed cleanup
package com.testsigma.addons.util; import com.testsigma.sdk.Logger; -import org.apache.commons.io.FileUtils; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; import org.apache.http.client.config.RequestConfig; import org.apache.http.client.entity.EntityBuilder; import org.apache.http.client.methods.HttpPut; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClients; -import org.openqa.selenium.JavascriptExecutor; -import org.openqa.selenium.WebDriver; -import org.openqa.selenium.WebElement; -import java.awt.*; -import java.awt.image.BufferedImage; import java.io.File; -import java.net.URL; -import java.nio.file.Files; -import java.nio.file.Paths; -import java.util.Base64; -import java.util.List; public class ImageComparisonUtils { - WebDriver driver; Logger logger; - public ImageComparisonUtils(WebDriver driver, Logger logger) { - this.driver = driver; + public ImageComparisonUtils(Logger logger) { this.logger = logger; }Note: Removing
driverfrom the constructor is a breaking change for callers —DisplayTestDataAsStepResultScreenShotconstructs this asnew ImageComparisonUtils(driver, logger)and would need to be updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@string_to_image_action/src/main/java/com/testsigma/addons/util/ImageComparisonUtils.java` around lines 3 - 24, Remove unused imports (AWT classes: java.awt.*, java.awt.image.BufferedImage; IO/NIO: java.io.File, java.net.URL, java.nio.file.Files, java.nio.file.Paths; org.apache.commons.io.FileUtils; java.util.Base64; java.util.List; Selenium: org.openqa.selenium.*, and org.apache.http.* classes not used) from ImageComparisonUtils and delete the unused WebDriver driver field and its assignment in the ImageComparisonUtils constructor; then update all call sites that instantiate ImageComparisonUtils (e.g., DisplayTestDataAsStepResultScreenShot which does new ImageComparisonUtils(driver, logger)) to pass only the remaining constructor parameters (remove driver argument) so callers compile after the signature change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@string_to_image_action/pom.xml`:
- Around line 42-46: The TestNG dependency (org.testng:testng version 6.14.3) is
declared without a test scope and will be packaged into the shaded JAR; update
the dependency declaration to add <scope>test</scope> so it is only used for
tests and not bundled, and bump the version to a maintained release (e.g., a 7.x
TestNG) to avoid an ancient 2018 release; also verify the Maven Shade plugin or
any dependencyManagement/exclusions do not inadvertently include test-scoped
dependencies when building the shaded artifact.
- Around line 59-80: The pom declares jackson-annotations at 2.13.0 but
jackson-databind at 2.12.3 which will cause runtime Jackson incompatibilities;
update the pom so all Jackson artifacts (e.g., jackson-annotations and
jackson-databind) use the same minor version (pick 2.13.0 or a single newer
matching version) and verify any other Jackson dependencies (groupId
com.fasterxml.jackson.core) are aligned to that same version to avoid
JsonMappingException/annotation-processing failures.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/util/ImageComparisonUtils.java`:
- Around line 41-64: The uploadFile method currently ignores the HttpResponse
from httpclient.execute(httpPut) and always returns true; update uploadFile to
inspect the HttpResponse status (response.getStatusLine().getStatusCode()) after
executing the HttpPut and only return true for successful 2xx codes, otherwise
log an error with the status code and any response body or reason phrase and
return false; ensure exception handling still returns false and logs the stack
trace via ExceptionUtils.getStackTrace(e).
In
`@string_to_image_action/src/main/java/com/testsigma/addons/web/DisplayTestDataAsStepResultScreenShot.java`:
- Around line 43-55: The code silently skips processing when
testStepResult.getScreenshotUrl() returns null/empty; update the branch that
handles s3Url to explicitly treat null/empty as an error by logging a message,
calling setErrorMessage("No screenshot URL provided" or similar), setting result
= Result.FAILED, and returning/short-circuiting so the action does not continue
to delete the image and report success; modify the logic around the s3Url
variable and the existing upload block (references:
testStepResult.getScreenshotUrl(), s3Url, ImageComparisonUtils.uploadFile,
setErrorMessage, setSuccessMessage, result/Result.FAILED) to enforce this
failure path.
- Around line 39-56: The temp file created by
StringToImageConverter.convertToFile (screenshotFile) can leak if an exception
occurs before deleteOnExit() is registered and relying solely on deleteOnExit()
is insufficient for long‑running processes; wrap the creation and upload logic
in a try/finally so the file is registered immediately (keep deleteOnExit() as a
fallback) and ensure explicit deletion in the finally block (call
screenshotFile.delete() and check the result), i.e., create screenshotFile, then
immediately call deleteOnExit(), perform the
ImageComparisonUtils.uploadFile(testStepResult.getScreenshotUrl(),
screenshotFile.getAbsolutePath()) work inside try, and in finally attempt
screenshotFile.delete() to remove the temp file even on exceptions (handle null
checks for testStepResult and upload failure paths accordingly).
- Around line 57-60: The catch block in DisplayTestDataAsStepResultScreenShot
currently swallows the exception by logging it at DEBUG and not setting a UI
error message; change the logger call to logger.error(...) and call
setErrorMessage(...) (passing a concise message or e.getMessage() plus context)
so the test result UI shows the failure reason; keep setting result =
Result.FAILURE and include the stack trace via ExceptionUtils.getStackTrace(e)
in the error log for diagnostics.
- Around line 50-58: The exception handler in
DisplayTestDataAsStepResultScreenShot sets result = Result.FAILURE but the
add-on should use Result.FAILED for step results; update the catch block to
assign result = Result.FAILED (the same enum used earlier) so the Result enum
value is consistent for failures when thrown in the try/catch around the
screenshot/upload logic in DisplayTestDataAsStepResultScreenShot.
In `@string_to_image_action/src/main/resources/testsigma-sdk.properties`:
- Line 1: Revoke the exposed JWT immediately and remove the hardcoded property
testsigma-sdk.api.key from the committed resource; delete the value from the
testsigma-sdk.properties and purge it from git history (use git rm / filter-repo
or BFG) so it is not present in past commits or built artifacts, then change the
code that reads testsigma-sdk.api.key to load the key from a runtime secret
(environment variable or system property) instead of the resource file and
document the new env var name for CI/CD secret injection.
---
Nitpick comments:
In `@string_to_image_action/pom.xml`:
- Around line 71-90: The pom currently declares unused dependencies
com.squareup.okhttp3:okhttp and org.apache.poi:poi; remove those two
<dependency> blocks (identify by groupId/artifactId okhttp and poi) from
string_to_image_action/pom.xml, then run a quick check (mvn dependency:analyze
or a project-wide import/search) to confirm no code references remain and
rebuild to ensure the shaded JAR size shrinks and no compile errors occur.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/util/ImageComparisonUtils.java`:
- Around line 3-24: Remove unused imports (AWT classes: java.awt.*,
java.awt.image.BufferedImage; IO/NIO: java.io.File, java.net.URL,
java.nio.file.Files, java.nio.file.Paths; org.apache.commons.io.FileUtils;
java.util.Base64; java.util.List; Selenium: org.openqa.selenium.*, and
org.apache.http.* classes not used) from ImageComparisonUtils and delete the
unused WebDriver driver field and its assignment in the ImageComparisonUtils
constructor; then update all call sites that instantiate ImageComparisonUtils
(e.g., DisplayTestDataAsStepResultScreenShot which does new
ImageComparisonUtils(driver, logger)) to pass only the remaining constructor
parameters (remove driver argument) so callers compile after the signature
change.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/util/StringToImageConverter.java`:
- Line 44: The Math.max floor of 40 in the imageHeight calculation is
unreachable because 2 * PADDING already makes the expression > 40; update the
computation in StringToImageConverter to remove the dead Math.max and compute
imageHeight directly as lines.size() * lineHeight + 2 * PADDING (referencing the
imageHeight variable, lines, lineHeight, and PADDING), or if a minimum height is
desired use a meaningful threshold > 2 * PADDING; adjust only the expression
that sets imageHeight.
- Line 143: The current return uses an immutable List.of(" ") when
lines.isEmpty(), causing an inconsistent immutable type compared to the usual
mutable ArrayList returned earlier (see variable lines and the text.isEmpty()
check in StringToImageConverter); change the fallback to return a mutable
ArrayList containing a single " " element instead of List.of(" "), and consider
removing or documenting the unreachable guard since text.isEmpty() already
handles the empty-string path in this class/method.
- Around line 81-89: The computeFontSize method uses Math.max(MIN_FONT_SIZE, 14)
which is redundant because MIN_FONT_SIZE is 12 < 14; update the tail case in
computeFontSize to use the intended configurable minimum: replace the expression
Math.max(MIN_FONT_SIZE, 14) with MIN_FONT_SIZE (or, if you intended the minimum
to be 14, update the MIN_FONT_SIZE constant to 14) so the final return respects
the constant; refer to the computeFontSize method and the MIN_FONT_SIZE constant
when making the change.
Comment on lines
+42
to
+46
| <dependency> | ||
| <groupId>org.testng</groupId> | ||
| <artifactId>testng</artifactId> | ||
| <version>6.14.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
testng is missing <scope>test</scope> and will be bundled into the shaded JAR.
TestNG is a test framework and should not be a compile/runtime dependency. Additionally, version 6.14.3 is from 2018; if TestNG is required at all, a current version should be used.
♻️ Proposed fix
<dependency>
<groupId>org.testng</groupId>
<artifactId>testng</artifactId>
- <version>6.14.3</version>
+ <version>7.9.0</version>
+ <scope>test</scope>
</dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@string_to_image_action/pom.xml` around lines 42 - 46, The TestNG dependency
(org.testng:testng version 6.14.3) is declared without a test scope and will be
packaged into the shaded JAR; update the dependency declaration to add
<scope>test</scope> so it is only used for tests and not bundled, and bump the
version to a maintained release (e.g., a 7.x TestNG) to avoid an ancient 2018
release; also verify the Maven Shade plugin or any
dependencyManagement/exclusions do not inadvertently include test-scoped
dependencies when building the shaded artifact.
Comment on lines
+59
to
+80
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-annotations</artifactId> | ||
| <version>2.13.0</version> | ||
| </dependency> | ||
|
|
||
| <!-- > Additional dependencies --> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-lang3</artifactId> | ||
| <version>3.12.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.squareup.okhttp3</groupId> | ||
| <artifactId>okhttp</artifactId> | ||
| <version>4.9.1</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.12.3</version> <!-- Make sure to use the latest version --> | ||
| </dependency> |
There was a problem hiding this comment.
Jackson version mismatch will cause runtime failures.
jackson-annotations is pinned to 2.13.0 while jackson-databind is 2.12.3. Jackson components must share the same minor version; mismatched versions are a documented source of JsonMappingException and annotation-processing failures at runtime.
🐛 Proposed fix: align to the same minor version
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
- <version>2.13.0</version>
+ <version>2.13.5</version>
</dependency>
...
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.12.3</version>
+ <version>2.13.5</version>
</dependency>📝 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.
Suggested change
| <dependency> | |
| <groupId>com.fasterxml.jackson.core</groupId> | |
| <artifactId>jackson-annotations</artifactId> | |
| <version>2.13.0</version> | |
| </dependency> | |
| <!-- > Additional dependencies --> | |
| <dependency> | |
| <groupId>org.apache.commons</groupId> | |
| <artifactId>commons-lang3</artifactId> | |
| <version>3.12.0</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.squareup.okhttp3</groupId> | |
| <artifactId>okhttp</artifactId> | |
| <version>4.9.1</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.fasterxml.jackson.core</groupId> | |
| <artifactId>jackson-databind</artifactId> | |
| <version>2.12.3</version> <!-- Make sure to use the latest version --> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.fasterxml.jackson.core</groupId> | |
| <artifactId>jackson-annotations</artifactId> | |
| <version>2.13.5</version> | |
| </dependency> | |
| <!-- > Additional dependencies --> | |
| <dependency> | |
| <groupId>org.apache.commons</groupId> | |
| <artifactId>commons-lang3</artifactId> | |
| <version>3.12.0</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.squareup.okhttp3</groupId> | |
| <artifactId>okhttp</artifactId> | |
| <version>4.9.1</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.fasterxml.jackson.core</groupId> | |
| <artifactId>jackson-databind</artifactId> | |
| <version>2.13.5</version> <!-- Make sure to use the latest version --> | |
| </dependency> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@string_to_image_action/pom.xml` around lines 59 - 80, The pom declares
jackson-annotations at 2.13.0 but jackson-databind at 2.12.3 which will cause
runtime Jackson incompatibilities; update the pom so all Jackson artifacts
(e.g., jackson-annotations and jackson-databind) use the same minor version
(pick 2.13.0 or a single newer matching version) and verify any other Jackson
dependencies (groupId com.fasterxml.jackson.core) are aligned to that same
version to avoid JsonMappingException/annotation-processing failures.
Comment on lines
+41
to
+64
| public boolean uploadFile(String s3SignedURL, String localPath) { | ||
| logger.debug("s3SignedURL - " + s3SignedURL); | ||
| logger.debug("localPath - " + localPath); | ||
| boolean localUrlExists = new File(localPath).exists(); | ||
| if (localUrlExists) { | ||
| logger.info(String.format("Uploading test asset to storage, presigned-URL:%s, localFilePath:%s", s3SignedURL, localPath)); | ||
| try (CloseableHttpClient httpclient = HttpClients.custom().setDefaultRequestConfig(config).build()) { | ||
| HttpPut httpPut = new HttpPut(s3SignedURL); | ||
|
|
||
| File file = new File(localPath); | ||
| HttpEntity entity = EntityBuilder.create().setFile(file).build(); | ||
| httpPut.setEntity(entity); | ||
| HttpResponse response = httpclient.execute(httpPut); | ||
| logger.info("Upload completed"); | ||
| return true; | ||
| } catch (Exception e) { | ||
| logger.info("Exception while uploading custom screenshot to s3: " + ExceptionUtils.getStackTrace(e)); | ||
| return false; | ||
| } | ||
| } else { | ||
| logger.info("Local path does not exist"); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
S3 upload response status is never checked — silent false-positive success.
httpclient.execute(httpPut) returns an HttpResponse whose status code is discarded. If S3 responds with a 403 (e.g., expired presigned URL) or 400 (content-type mismatch), the method logs "Upload completed" and returns true, causing the caller to report SUCCESS when the image was never actually stored.
🐛 Proposed fix: inspect the response status code
HttpResponse response = httpclient.execute(httpPut);
-logger.info("Upload completed");
-return true;
+int statusCode = response.getStatusLine().getStatusCode();
+logger.info("Upload completed with HTTP status: " + statusCode);
+if (statusCode >= 200 && statusCode < 300) {
+ return true;
+} else {
+ logger.info("Upload failed with unexpected HTTP status: " + statusCode);
+ return false;
+}📝 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.
Suggested change
| public boolean uploadFile(String s3SignedURL, String localPath) { | |
| logger.debug("s3SignedURL - " + s3SignedURL); | |
| logger.debug("localPath - " + localPath); | |
| boolean localUrlExists = new File(localPath).exists(); | |
| if (localUrlExists) { | |
| logger.info(String.format("Uploading test asset to storage, presigned-URL:%s, localFilePath:%s", s3SignedURL, localPath)); | |
| try (CloseableHttpClient httpclient = HttpClients.custom().setDefaultRequestConfig(config).build()) { | |
| HttpPut httpPut = new HttpPut(s3SignedURL); | |
| File file = new File(localPath); | |
| HttpEntity entity = EntityBuilder.create().setFile(file).build(); | |
| httpPut.setEntity(entity); | |
| HttpResponse response = httpclient.execute(httpPut); | |
| logger.info("Upload completed"); | |
| return true; | |
| } catch (Exception e) { | |
| logger.info("Exception while uploading custom screenshot to s3: " + ExceptionUtils.getStackTrace(e)); | |
| return false; | |
| } | |
| } else { | |
| logger.info("Local path does not exist"); | |
| return false; | |
| } | |
| } | |
| public boolean uploadFile(String s3SignedURL, String localPath) { | |
| logger.debug("s3SignedURL - " + s3SignedURL); | |
| logger.debug("localPath - " + localPath); | |
| boolean localUrlExists = new File(localPath).exists(); | |
| if (localUrlExists) { | |
| logger.info(String.format("Uploading test asset to storage, presigned-URL:%s, localFilePath:%s", s3SignedURL, localPath)); | |
| try (CloseableHttpClient httpclient = HttpClients.custom().setDefaultRequestConfig(config).build()) { | |
| HttpPut httpPut = new HttpPut(s3SignedURL); | |
| File file = new File(localPath); | |
| HttpEntity entity = EntityBuilder.create().setFile(file).build(); | |
| httpPut.setEntity(entity); | |
| HttpResponse response = httpclient.execute(httpPut); | |
| int statusCode = response.getStatusLine().getStatusCode(); | |
| logger.info("Upload completed with HTTP status: " + statusCode); | |
| if (statusCode >= 200 && statusCode < 300) { | |
| return true; | |
| } else { | |
| logger.info("Upload failed with unexpected HTTP status: " + statusCode); | |
| return false; | |
| } | |
| } catch (Exception e) { | |
| logger.info("Exception while uploading custom screenshot to s3: " + ExceptionUtils.getStackTrace(e)); | |
| return false; | |
| } | |
| } else { | |
| logger.info("Local path does not exist"); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/util/ImageComparisonUtils.java`
around lines 41 - 64, The uploadFile method currently ignores the HttpResponse
from httpclient.execute(httpPut) and always returns true; update uploadFile to
inspect the HttpResponse status (response.getStatusLine().getStatusCode()) after
executing the HttpPut and only return true for successful 2xx codes, otherwise
log an error with the status code and any response body or reason phrase and
return false; ensure exception handling still returns false and logs the stack
trace via ExceptionUtils.getStackTrace(e).
Comment on lines
+39
to
+56
| File screenshotFile = StringToImageConverter.convertToFile(testData); | ||
| logger.debug("Created screenshot image at: " + screenshotFile.getAbsolutePath()); | ||
|
|
||
| // upload the screenshot to the test step result (same pattern as VerifyIfTwoImagesAreSimilar) | ||
| String s3Url = testStepResult.getScreenshotUrl(); | ||
| if (s3Url != null && !s3Url.isEmpty()) { | ||
| ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); | ||
| boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath()); | ||
| if (!uploadResult) { | ||
| logger.debug("Error uploading custom screenshot to S3; step result may not show the image."); | ||
| setErrorMessage("Error uploading custom screenshot, step result may not show the image."); | ||
| result = Result.FAILED; | ||
| } else { | ||
| logger.debug("Custom screenshot uploaded successfully."); | ||
| setSuccessMessage("Successfully displayed test data as image in step result"); | ||
| } | ||
| } | ||
| screenshotFile.deleteOnExit(); |
There was a problem hiding this comment.
Temp file leaks on the exception path; deleteOnExit() is also insufficient for long-lived processes.
screenshotFile.deleteOnExit() is called at Line 56, but:
- Any exception thrown between Lines 39–55 (e.g., NPE on
testStepResultat Line 43) jumps to thecatchblock before the registration, leaving the temp file on disk for the rest of the JVM lifetime. - Even when registered,
deleteOnExit()only runs at JVM shutdown — in a long-running test-runner process, temp files accumulate indefinitely.
🐛 Proposed fix: register immediately and delete explicitly in finally
File screenshotFile = StringToImageConverter.convertToFile(testData);
+screenshotFile.deleteOnExit(); // fallback: register immediately
logger.debug("Created screenshot image at: " + screenshotFile.getAbsolutePath());
-
-// upload the screenshot to the test step result
-String s3Url = testStepResult.getScreenshotUrl();
-if (s3Url != null && !s3Url.isEmpty()) {
- ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger);
- boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath());
- if (!uploadResult) {
- logger.debug("Error uploading custom screenshot to S3; step result may not show the image.");
- setErrorMessage("Error uploading custom screenshot, step result may not show the image.");
- result = Result.FAILED;
- } else {
- logger.debug("Custom screenshot uploaded successfully.");
- setSuccessMessage("Successfully displayed test data as image in step result");
+try {
+ // upload the screenshot to the test step result
+ String s3Url = testStepResult.getScreenshotUrl();
+ if (s3Url != null && !s3Url.isEmpty()) {
+ ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger);
+ boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath());
+ if (!uploadResult) {
+ logger.debug("Error uploading custom screenshot to S3; step result may not show the image.");
+ setErrorMessage("Error uploading custom screenshot, step result may not show the image.");
+ result = Result.FAILED;
+ } else {
+ logger.debug("Custom screenshot uploaded successfully.");
+ setSuccessMessage("Successfully displayed test data as image in step result");
+ }
}
+} finally {
+ screenshotFile.delete();
}
-screenshotFile.deleteOnExit();📝 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.
Suggested change
| File screenshotFile = StringToImageConverter.convertToFile(testData); | |
| logger.debug("Created screenshot image at: " + screenshotFile.getAbsolutePath()); | |
| // upload the screenshot to the test step result (same pattern as VerifyIfTwoImagesAreSimilar) | |
| String s3Url = testStepResult.getScreenshotUrl(); | |
| if (s3Url != null && !s3Url.isEmpty()) { | |
| ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); | |
| boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath()); | |
| if (!uploadResult) { | |
| logger.debug("Error uploading custom screenshot to S3; step result may not show the image."); | |
| setErrorMessage("Error uploading custom screenshot, step result may not show the image."); | |
| result = Result.FAILED; | |
| } else { | |
| logger.debug("Custom screenshot uploaded successfully."); | |
| setSuccessMessage("Successfully displayed test data as image in step result"); | |
| } | |
| } | |
| screenshotFile.deleteOnExit(); | |
| File screenshotFile = StringToImageConverter.convertToFile(testData); | |
| screenshotFile.deleteOnExit(); // fallback: register immediately | |
| logger.debug("Created screenshot image at: " + screenshotFile.getAbsolutePath()); | |
| try { | |
| // upload the screenshot to the test step result (same pattern as VerifyIfTwoImagesAreSimilar) | |
| String s3Url = testStepResult.getScreenshotUrl(); | |
| if (s3Url != null && !s3Url.isEmpty()) { | |
| ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); | |
| boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath()); | |
| if (!uploadResult) { | |
| logger.debug("Error uploading custom screenshot to S3; step result may not show the image."); | |
| setErrorMessage("Error uploading custom screenshot, step result may not show the image."); | |
| result = Result.FAILED; | |
| } else { | |
| logger.debug("Custom screenshot uploaded successfully."); | |
| setSuccessMessage("Successfully displayed test data as image in step result"); | |
| } | |
| } | |
| } finally { | |
| screenshotFile.delete(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/web/DisplayTestDataAsStepResultScreenShot.java`
around lines 39 - 56, The temp file created by
StringToImageConverter.convertToFile (screenshotFile) can leak if an exception
occurs before deleteOnExit() is registered and relying solely on deleteOnExit()
is insufficient for long‑running processes; wrap the creation and upload logic
in a try/finally so the file is registered immediately (keep deleteOnExit() as a
fallback) and ensure explicit deletion in the finally block (call
screenshotFile.delete() and check the result), i.e., create screenshotFile, then
immediately call deleteOnExit(), perform the
ImageComparisonUtils.uploadFile(testStepResult.getScreenshotUrl(),
screenshotFile.getAbsolutePath()) work inside try, and in finally attempt
screenshotFile.delete() to remove the temp file even on exceptions (handle null
checks for testStepResult and upload failure paths accordingly).
Comment on lines
+43
to
+55
| String s3Url = testStepResult.getScreenshotUrl(); | ||
| if (s3Url != null && !s3Url.isEmpty()) { | ||
| ImageComparisonUtils imageComparisonUtils = new ImageComparisonUtils(driver, logger); | ||
| boolean uploadResult = imageComparisonUtils.uploadFile(s3Url, screenshotFile.getAbsolutePath()); | ||
| if (!uploadResult) { | ||
| logger.debug("Error uploading custom screenshot to S3; step result may not show the image."); | ||
| setErrorMessage("Error uploading custom screenshot, step result may not show the image."); | ||
| result = Result.FAILED; | ||
| } else { | ||
| logger.debug("Custom screenshot uploaded successfully."); | ||
| setSuccessMessage("Successfully displayed test data as image in step result"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent no-op when s3Url is null or empty — should surface an error.
When testStepResult.getScreenshotUrl() returns null or "", the upload block is skipped, the image is deleted, and the action returns SUCCESS with no message. Since the entire purpose of this action is to display a string as a screenshot, having no S3 URL means the action accomplished nothing. At minimum, call setErrorMessage() and return FAILED to alert the user.
🐛 Proposed fix
if (s3Url != null && !s3Url.isEmpty()) {
// ... upload logic ...
+} else {
+ setErrorMessage("No screenshot URL available on the step result; cannot display test data as image.");
+ result = Result.FAILED;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/web/DisplayTestDataAsStepResultScreenShot.java`
around lines 43 - 55, The code silently skips processing when
testStepResult.getScreenshotUrl() returns null/empty; update the branch that
handles s3Url to explicitly treat null/empty as an error by logging a message,
calling setErrorMessage("No screenshot URL provided" or similar), setting result
= Result.FAILED, and returning/short-circuiting so the action does not continue
to delete the image and report success; modify the logic around the s3Url
variable and the existing upload block (references:
testStepResult.getScreenshotUrl(), s3Url, ImageComparisonUtils.uploadFile,
setErrorMessage, setSuccessMessage, result/Result.FAILED) to enforce this
failure path.
Comment on lines
+50
to
+58
| result = Result.FAILED; | ||
| } else { | ||
| logger.debug("Custom screenshot uploaded successfully."); | ||
| setSuccessMessage("Successfully displayed test data as image in step result"); | ||
| } | ||
| } | ||
| screenshotFile.deleteOnExit(); | ||
| } catch (Exception e) { | ||
| result = Result.FAILURE; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Result enum definition and see both FAILED and FAILURE usage
rg "enum Result|FAILED|FAILURE" --type java -A 2 -B 2Repository: testsigmahq/testsigma-addons
Length of output: 1735
🌐 Web query:
Testsigma SDK Result enum FAILED FAILURE
💡 Result:
In the Testsigma Java SDK, com.testsigma.sdk.Result is defined as:
SUCCESSFAILUREFAILEDQUEUED[1]
In add-on (custom NLP) code, Testsigma examples typically return Result.SUCCESS or Result.FAILED from execute() to mark the step outcome. [2]
In the Testsigma REST APIs / run results payloads, the "result" field commonly appears as a string like "FAILURE" (and "SUCCESS"). [3]
Practical takeaway: for add-on step return values, use Result.FAILED (not Result.FAILURE) unless you’re explicitly matching API payload/result text. [1][2][3]
Sources
[1] Testsigma Java SDK Result enum source (GitHub raw)
[2] Testsigma tutorial showing return Result.FAILED;
[3] Testsigma API docs showing "result": "FAILURE" in responses
Use Result.FAILED consistently on line 58 instead of Result.FAILURE.
The Testsigma SDK defines both Result.FAILED and Result.FAILURE as distinct enum values. For add-on step return values, SDK conventions and documentation examples show that Result.FAILED should be used. Line 50 correctly uses Result.FAILED for the upload failure case, but line 58 in the exception handler incorrectly uses Result.FAILURE (which is typically used in REST API payloads, not addon code). Change line 58 to Result.FAILED for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/web/DisplayTestDataAsStepResultScreenShot.java`
around lines 50 - 58, The exception handler in
DisplayTestDataAsStepResultScreenShot sets result = Result.FAILURE but the
add-on should use Result.FAILED for step results; update the catch block to
assign result = Result.FAILED (the same enum used earlier) so the Result enum
value is consistent for failures when thrown in the try/catch around the
screenshot/upload logic in DisplayTestDataAsStepResultScreenShot.
Comment on lines
+57
to
+60
| } catch (Exception e) { | ||
| result = Result.FAILURE; | ||
| logger.debug("Error while displaying test data as image in step result: " + ExceptionUtils.getStackTrace(e)); | ||
| } |
There was a problem hiding this comment.
Exception swallowed silently: no UI message and wrong log level.
In the catch block, the exception stack trace is logged at DEBUG level (invisible under normal logging) and setErrorMessage() is never called, so the test result UI shows a failure with no explanation.
🐛 Proposed fix
} catch (Exception e) {
result = Result.FAILURE;
- logger.debug("Error while displaying test data as image in step result: " + ExceptionUtils.getStackTrace(e));
+ logger.error("Error while displaying test data as image in step result: " + ExceptionUtils.getStackTrace(e));
+ setErrorMessage("Error displaying test data as image: " + e.getMessage());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@string_to_image_action/src/main/java/com/testsigma/addons/web/DisplayTestDataAsStepResultScreenShot.java`
around lines 57 - 60, The catch block in DisplayTestDataAsStepResultScreenShot
currently swallows the exception by logging it at DEBUG and not setting a UI
error message; change the logger call to logger.error(...) and call
setErrorMessage(...) (passing a concise message or e.getMessage() plus context)
so the test result UI shows the failure reason; keep setting result =
Result.FAILURE and include the stack trace via ExceptionUtils.getStackTrace(e)
in the error log for diagnostics.
| @@ -0,0 +1 @@ | |||
| testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiIyNDBkMjhiNS0xNmUwLThlNmYtOWQ0ZS05MjYxMGNiZTcyYzciLCJ1bmlxdWVJZCI6IjU5NzIiLCJpZGVudGl0eUFjY291bnRVVUlkIjoiMzUifQ.iwamnW6L1cwioQ5H8YtV2Px12qJc0kyzLWN6IOhPkCI50GEagGCR52hyJYw0mUU45j_StvkrhI4SmGSqtDBaLA No newline at end of file | |||
There was a problem hiding this comment.
🚨 Critical: Live API key committed to a public repository — revoke immediately.
The value of testsigma-sdk.api.key is a real, active JWT for tenant 2817 on jarvis.testsigma.com. Committing it here exposes it in:
- The public GitHub history (even after removal, it remains retrievable via
git log). - The built/published JAR (in
src/main/resources).
Required actions:
- Revoke this key now in the Testsigma tenant portal.
- Do not merge this file or reference its value anywhere in source.
- Replace runtime usage with an environment variable or CI/CD secret injection (e.g., set the key via a system property or environment variable at runtime, not hardcoded in a resource file).
-testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiIyNDBkMjhiNS0xNmUwLThlNmYtOWQ0ZS05MjYxMGNiZTcyYzciLCJ1bmlxdWVJZCI6IjU5NzIiLCJpZGVudGl0eUFjY291bnRVVUlkIjoiMzUifQ.iwamnW6L1cwioQ5H8YtV2Px12qJc0kyzLWN6IOhPkCI50GEagGCR52hyJYw0mUU45j_StvkrhI4SmGSqtDBaLA
+# Set via environment variable or CI/CD secrets injection
+# testsigma-sdk.api.key=${TESTSIGMA_SDK_API_KEY}📝 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.
Suggested change
| testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiIyNDBkMjhiNS0xNmUwLThlNmYtOWQ0ZS05MjYxMGNiZTcyYzciLCJ1bmlxdWVJZCI6IjU5NzIiLCJpZGVudGl0eUFjY291bnRVVUlkIjoiMzUifQ.iwamnW6L1cwioQ5H8YtV2Px12qJc0kyzLWN6IOhPkCI50GEagGCR52hyJYw0mUU45j_StvkrhI4SmGSqtDBaLA | |
| # Set via environment variable or CI/CD secrets injection | |
| # testsigma-sdk.api.key=${TESTSIGMA_SDK_API_KEY} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@string_to_image_action/src/main/resources/testsigma-sdk.properties` at line
1, Revoke the exposed JWT immediately and remove the hardcoded property
testsigma-sdk.api.key from the committed resource; delete the value from the
testsigma-sdk.properties and purge it from git history (use git rm / filter-repo
or BFG) so it is not present in past commits or built artifacts, then change the
code that reads testsigma-sdk.api.key to load the key from a runtime secret
(environment variable or system property) instead of the resource file and
document the new env var name for CI/CD secret injection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
please review this addon and publish as PUBLIC
Addon name : string to image action
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-10699
fix
CREATED AN NLP WHICH WILL DISPLAY STRING AS STEP RESULT SCREENSHOT...
Summary by CodeRabbit