[CUS-10316] Updated the addon to show differences properly.#329
[CUS-10316] Updated the addon to show differences properly.#329ManojTestsigma wants to merge 1 commit intodevfrom
Conversation
|
|
📝 WalkthroughWalkthroughThis PR updates the ExcelCellUtils method signature to accept a generic Sheet interface instead of XSSFSheet, and introduces a new CompareExcelFilesAndLogDifferences WebAction that compares two Excel files (local or remote) and reports detailed differences. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@excelActions_cloud/src/main/java/com/testsigma/addons/web/CompareExcelFilesAndLogDifferences.java`:
- Around line 168-169: The code assumes Paths.get(url.getPath()).getFileName()
is non-null when building fileName; guard against a null return (which occurs
for root or empty paths) by checking the result of getFileName() and providing a
fallback filename (e.g., "index" or use url.getHost()) before calling
toString(); update the URL handling in CompareExcelFilesAndLogDifferences where
URL url and String fileName are set to perform this null-check and fallback so
no NPE occurs.
- Around line 167-181: downloadFile creates a temp File (tempFile) but never
registers or deletes it; modify downloadFile to call tempFile.deleteOnExit()
immediately after creation and update the caller(s) that use
downloadFile/compare logic to explicitly delete the tempFile in a finally block
(or use try-with-resources equivalent) after comparison completes so files are
removed promptly rather than only on JVM exit; reference the downloadFile method
and the tempFile variable when making these changes.
🧹 Nitpick comments (1)
excelActions_cloud/src/main/java/com/testsigma/addons/web/CompareExcelFilesAndLogDifferences.java (1)
55-56: Downloaded temp files should be cleaned up after comparison completes.Beyond
deleteOnExit()indownloadFile, consider deleting the files explicitly after the comparison (in afinallyblock withinexecute()). This avoids accumulation even within a long-lived JVM process.Sketch: explicit cleanup in execute()
+ File excelFile1 = null; + File excelFile2 = null; try { String path1 = filePath1.getValue().toString(); String path2 = filePath2.getValue().toString(); logger.info("File 1: " + path1); logger.info("File 2: " + path2); - File excelFile1 = getExcelFile(path1); - File excelFile2 = getExcelFile(path2); + excelFile1 = getExcelFile(path1); + excelFile2 = getExcelFile(path2); // ... comparison logic ... } catch (Exception e) { // ... error handling ... + } finally { + // Clean up downloaded temp files (safe for local files — delete only temp files) + if (excelFile1 != null && excelFile1.getName().startsWith("excel-compare-")) { + excelFile1.delete(); + } + if (excelFile2 != null && excelFile2.getName().startsWith("excel-compare-")) { + excelFile2.delete(); + } }Also applies to: 133-138
| private File downloadFile(String fileUrl) throws IOException { | ||
| URL url = new URL(fileUrl); | ||
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | ||
| File tempFile = File.createTempFile("excel-compare-", "-" + fileName); | ||
| try (InputStream in = url.openStream(); | ||
| OutputStream out = new FileOutputStream(tempFile)) { | ||
| byte[] buffer = new byte[4096]; | ||
| int bytesRead; | ||
| while ((bytesRead = in.read(buffer)) != -1) { | ||
| out.write(buffer, 0, bytesRead); | ||
| } | ||
| } | ||
| logger.info("Downloaded file to: " + tempFile.getAbsolutePath()); | ||
| return tempFile; | ||
| } |
There was a problem hiding this comment.
Temporary files are never cleaned up — disk leak in long-running suites.
downloadFile creates temp files via File.createTempFile but never schedules them for deletion. In a CI or long-running test session comparing many Excel files, these will accumulate on disk.
At minimum, call deleteOnExit() so the JVM cleans up on shutdown. Ideally, the caller should delete them in a finally block after comparison is complete.
Proposed fix: register temp file for cleanup
File tempFile = File.createTempFile("excel-compare-", "-" + fileName);
+ tempFile.deleteOnExit();
try (InputStream in = url.openStream();📝 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.
| private File downloadFile(String fileUrl) throws IOException { | |
| URL url = new URL(fileUrl); | |
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | |
| File tempFile = File.createTempFile("excel-compare-", "-" + fileName); | |
| try (InputStream in = url.openStream(); | |
| OutputStream out = new FileOutputStream(tempFile)) { | |
| byte[] buffer = new byte[4096]; | |
| int bytesRead; | |
| while ((bytesRead = in.read(buffer)) != -1) { | |
| out.write(buffer, 0, bytesRead); | |
| } | |
| } | |
| logger.info("Downloaded file to: " + tempFile.getAbsolutePath()); | |
| return tempFile; | |
| } | |
| private File downloadFile(String fileUrl) throws IOException { | |
| URL url = new URL(fileUrl); | |
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | |
| File tempFile = File.createTempFile("excel-compare-", "-" + fileName); | |
| tempFile.deleteOnExit(); | |
| try (InputStream in = url.openStream(); | |
| OutputStream out = new FileOutputStream(tempFile)) { | |
| byte[] buffer = new byte[4096]; | |
| int bytesRead; | |
| while ((bytesRead = in.read(buffer)) != -1) { | |
| out.write(buffer, 0, bytesRead); | |
| } | |
| } | |
| logger.info("Downloaded file to: " + tempFile.getAbsolutePath()); | |
| return tempFile; | |
| } |
🤖 Prompt for AI Agents
In
`@excelActions_cloud/src/main/java/com/testsigma/addons/web/CompareExcelFilesAndLogDifferences.java`
around lines 167 - 181, downloadFile creates a temp File (tempFile) but never
registers or deletes it; modify downloadFile to call tempFile.deleteOnExit()
immediately after creation and update the caller(s) that use
downloadFile/compare logic to explicitly delete the tempFile in a finally block
(or use try-with-resources equivalent) after comparison completes so files are
removed promptly rather than only on JVM exit; reference the downloadFile method
and the tempFile variable when making these changes.
| URL url = new URL(fileUrl); | ||
| String fileName = Paths.get(url.getPath()).getFileName().toString(); |
There was a problem hiding this comment.
Potential NullPointerException if URL path is empty or root-only.
Paths.get(url.getPath()).getFileName() returns null when the path is "/" or empty (e.g., https://example.com/). The subsequent .toString() would throw an NPE.
Proposed fix: fallback filename
URL url = new URL(fileUrl);
- String fileName = Paths.get(url.getPath()).getFileName().toString();
+ java.nio.file.Path fileNamePath = Paths.get(url.getPath()).getFileName();
+ String fileName = (fileNamePath != null) ? fileNamePath.toString() : "downloaded.xlsx";
File tempFile = File.createTempFile("excel-compare-", "-" + fileName);📝 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.
| URL url = new URL(fileUrl); | |
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | |
| URL url = new URL(fileUrl); | |
| java.nio.file.Path fileNamePath = Paths.get(url.getPath()).getFileName(); | |
| String fileName = (fileNamePath != null) ? fileNamePath.toString() : "downloaded.xlsx"; | |
| File tempFile = File.createTempFile("excel-compare-", "-" + fileName); |
🤖 Prompt for AI Agents
In
`@excelActions_cloud/src/main/java/com/testsigma/addons/web/CompareExcelFilesAndLogDifferences.java`
around lines 168 - 169, The code assumes Paths.get(url.getPath()).getFileName()
is non-null when building fileName; guard against a null return (which occurs
for root or empty paths) by checking the result of getFileName() and providing a
fallback filename (e.g., "index" or use url.getHost()) before calling
toString(); update the URL handling in CompareExcelFilesAndLogDifferences where
URL url and String fileName are set to perform this null-check and fallback so
no NPE occurs.
4 participants
please review this addon and publish as PUBLIC
Addon name : ExcelActions_cloud
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-10316
fix
Summary by CodeRabbit
New Features
Refactor