MOSIP-43907: Admin UI testrig revert back old emailable report #804
Conversation
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
WalkthroughThe EmailableReport.generateReport signature was renamed (outputDir → outputDirectory). Report writer handling moved from try-with-resources to explicit field-managed writer with IOException handling. Report filename is recomputed using aggregate test counts, optional S3 upload added, durations shown in raw format, and several helper Javadoc blocks removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
249-297: Duration values are in milliseconds but headers still say “Time (min)”Both the suite summary and scenario summary now write raw
duration/scenarioDuration:
- Line 272:
writeTableData(decimalFormat.format(duration), "num");- Line 296:
writeTableHeader(decimalFormat.format(totalDuration), "num");- Line 394:
append("<td>").append(scenarioDuration).append("</td>");However, the column headers remain
"Time (min)"(lines 249 and 310). Sincedurationis computed asend - startin milliseconds, this will display milliseconds under a “min” label.Either convert to minutes (e.g.,
duration / 60000.0) or update the headers to reflect the actual unit (e.g., “Time (ms)”).Also applies to: 383-395
🧹 Nitpick comments (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
66-81: Guard writer lifecycle with try-with-resources or finally
writeris only closed at the end ofgenerateReport, so any unchecked exception fromwriteBody()/write*()will leak the underlying stream and leave a partially written report. Consider restoring try-with-resources (or afinallyblock) around the whole write sequence:-@Override -public void generateReport(List<XmlSuite> xmlSuites, List<ISuite> suites, String outputDirectory) { - try { - writer = createWriter(outputDirectory); - } catch (IOException e) { - logger.error("Unable to create output file", e); - return; - } - for (ISuite suite : suites) { - suiteResults.add(new SuiteResult(suite)); - } - writeDocumentStart(); - writeHead(); - writeBody(); - writeDocumentEnd(); - writer.close(); +@Override +public void generateReport(List<XmlSuite> xmlSuites, List<ISuite> suites, String outputDirectory) { + try (PrintWriter pw = createWriter(outputDirectory)) { + this.writer = pw; + for (ISuite suite : suites) { + suiteResults.add(new SuiteResult(suite)); + } + writeDocumentStart(); + writeHead(); + writeBody(); + writeDocumentEnd(); + } catch (IOException e) { + logger.error("Unable to create output file", e); + return; + }
570-617: Clarify intent ofRESULT_COMPARATOR = nulland its impact on grouping
RESULT_COMPARATORis now explicitly set tonullbut still used in:Collections.sort(resultsList, RESULT_COMPARATOR);With a null comparator,
Collections.sortfalls back to the natural ordering ofITestResult(itsComparableimplementation), whereas TestNG’s stockEmailableReporter2uses a comparator that orders by class name and then method name. That change can alter howgroupResultsclusters results intoClassResult/MethodResult, especially with parallel runs or data providers.If you want grouping consistent with the standard EmailableReporter2, consider restoring an explicit comparator, e.g.:
-protected static final Comparator<ITestResult> RESULT_COMPARATOR = null; +protected static final Comparator<ITestResult> RESULT_COMPARATOR = + Comparator.comparing((ITestResult r) -> r.getTestClass().getName()) + .thenComparing(r -> r.getMethod().getMethodName());If the new ordering is intentional, a short comment on
RESULT_COMPARATORwould help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (3)
244-296: Duration column now shows milliseconds but header still says “Time (min)”In
writeSuiteSummary,durationandtotalDurationare in raw milliseconds:long duration = testResult.getDuration(); // ms ... writeTableData(decimalFormat.format(duration), "num"); ... writeTableHeader(decimalFormat.format(totalDuration), "num");Yet the column header remains
Time (min). This makes the report misleading.Either convert to minutes before formatting, or update the header to reflect milliseconds. For example, to keep minutes:
- long duration = testResult.getDuration(); + long duration = testResult.getDuration(); // milliseconds @@ - writeTableData(decimalFormat.format(duration), "num"); + double durationMinutes = duration / 60000.0; + writeTableData(decimalFormat.format(durationMinutes), "num"); @@ - writeTableHeader(decimalFormat.format(totalDuration), "num"); + double totalDurationMinutes = totalDuration / 60000.0; + writeTableHeader(decimalFormat.format(totalDurationMinutes), "num");This aligns the values with the “Time (min)” label and the previous semantics.
310-311: Scenario duration units also mismatch the “Time (min)” headerIn the scenario summary table, the header is:
writer.print("<th>Time (min)</th>");But
scenarioDurationis computed and printed in raw milliseconds:long scenarioStart = result.getStartMillis(); long scenarioDuration = result.getEndMillis() - scenarioStart; // ms ... .append("<td>").append(scenarioDuration).append("</td></tr>");So the rendered value does not match the unit advertised in the column header.
As with the suite summary, either compute minutes or adjust the label. To keep minutes:
- long scenarioStart = result.getStartMillis(); - long scenarioDuration = result.getEndMillis() - scenarioStart; + long scenarioStart = result.getStartMillis(); + long scenarioDuration = result.getEndMillis() - scenarioStart; // milliseconds + double scenarioDurationMinutes = scenarioDuration / 60000.0; @@ - .append("<td>").append(scenarioDuration).append("</td></tr>"); // Table cell with + .append("<td>").append(scenarioDurationMinutes).append("</td></tr>"); // Table cell withThis keeps the UI consistent with the “Time (min)” heading.
Also applies to: 383-394
570-659: RESULT_COMPARATOR = null causes runtime failure in groupResults
RESULT_COMPARATORis currently defined as:protected static final Comparator<ITestResult> RESULT_COMPARATOR = null;But
groupResultscalls:Collections.sort(resultsList, RESULT_COMPARATOR);Passing
nullhere causesCollections.sortto fall back to the elements’ natural ordering; sinceITestResultis notComparable, this will throw aClassCastExceptionas soon as there is at least one result, breaking report generation.You need a real comparator that groups results by class and method (and optionally start time) so that
groupResultscan buildClassResultandMethodResultcorrectly. For example:- protected static final Comparator<ITestResult> RESULT_COMPARATOR = null; + protected static final Comparator<ITestResult> RESULT_COMPARATOR = new Comparator<ITestResult>() { + @Override + public int compare(ITestResult r1, ITestResult r2) { + int cmp = r1.getTestClass().getName().compareTo(r2.getTestClass().getName()); + if (cmp != 0) { + return cmp; + } + cmp = r1.getMethod().getMethodName() + .compareTo(r2.getMethod().getMethodName()); + if (cmp != 0) { + return cmp; + } + return Long.compare(r1.getStartMillis(), r2.getStartMillis()); + } + };This restores a stable ordering and prevents the
ClassCastExceptionwhile preserving the grouping logic ingroupResults.
♻️ Duplicate comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
82-128: Fix report rename paths, property typo, and simplify S3 upload flagsThis block still has several correctness and robustness issues:
orignialReportFileusesoutputDirectory, butnewReportFileand the log messages rebuild a different path usingSystem.getProperty("user.dir") + "/" + System.getProperty("testng.outpur.dir"), so rename can silently fail due to mismatched directories.- The system property key
testng.outpur.dirappears to be a typo fortestng.output.dir; as written it will likely benull, producing bogus paths in logs and fornewReportFile.- Logging paths via
System.getProperty(...)instead of the actualFileinstances makes debugging harder and can easily get out of sync with how the files are really created.isStoreSuccess2is a hardcodedtrueflag that doesn’t add value anymore; the success condition is effectively justisStoreSuccess.- The S3 error handling only logs
e.getMessage(), dropping stack trace and context.A more robust approach is to stick to
outputDirectoryconsistently and rely on theFileobjects for both renaming and logging, and simplify the S3 success flagging:- String reportDir = outputDirectory; // or System.getProperty("testng.output.dir") - File orignialReportFile = new File(reportDir, oldString); - logger.info("reportFile is::" + System.getProperty("user.dir") + "/" + System.getProperty("testng.outpur.dir") - + "/" + System.getProperty("emailable.report2.name")); - - File newReportFile = new File( - System.getProperty("user.dir") + "/" + System.getProperty("testng.outpur.dir") + "/" + newString); - logger.info("New reportFile is::" + System.getProperty("user.dir") + "/" - + System.getProperty("testng.outpur.dir") + "/" + newString); + File orignialReportFile = new File(outputDirectory, oldString); + File newReportFile = new File(outputDirectory, newString); + logger.info("Report file is:: " + orignialReportFile.getAbsolutePath()); + logger.info("New report file is:: " + newReportFile.getAbsolutePath()); @@ - if (ConfigManager.getPushReportsToS3().equalsIgnoreCase("yes")) { + String pushToS3 = ConfigManager.getPushReportsToS3(); + if ("yes".equalsIgnoreCase(pushToS3)) { S3Adapter s3Adapter = new S3Adapter(); boolean isStoreSuccess = false; - boolean isStoreSuccess2 = true; // or remove this flag until a second upload is added try { isStoreSuccess = s3Adapter.putObject(ConfigManager.getS3Account(), "Adminui", null, null, newString, newReportFile); logger.info("isStoreSuccess:: " + isStoreSuccess); @@ - } catch (Exception e) { - logger.error("error occured while pushing the object" + e.getMessage()); - } - if (isStoreSuccess && isStoreSuccess2) { + } catch (Exception e) { + logger.error("Error occurred while pushing the report to S3", e); + } + if (isStoreSuccess) { logger.info("Pushed report to S3"); } else { logger.error("Failed while pushing file to S3"); }This keeps file handling consistent, avoids the property typo, removes the unused flag, and logs full exception context.
🧹 Nitpick comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
66-81: Ensure writer is always closed even when report generation fails
generateReportnow assignswriterto a field and closes it only at the end. If any ofwriteDocumentStart/head/body/end()throws (e.g., due to data issues), the method exits without closing the writer, whereas a try‑with‑resources or try/finally block would guarantee cleanup.Consider restoring try‑with‑resources around
createWriter(outputDirectory)and the write calls, or at least wrapping the write section in atry { ... } finally { writer.close(); }to avoid leaking the file handle on failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
|
closed this PR as we already raised another pr in release branch |
3 participants
MOSIP-43907: Admin UI testrig revert back old emailable report
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.