Re: [PR] Replace the last native confirm dialogs, preview a role's navigation impact, and make CI's PR feedback useful [fineract-backoffice-ui]

["github-advanced-security[bot] (via GitHub)" <gi...@apache.org>]

github-advanced-security[bot] commented on code in PR #414:
URL: https://github.com/apache/fineract-backoffice-ui/pull/414#discussion_r3822008608


##########
scripts/e2e-summary.mjs:
##########
@@ -21,81 +21,294 @@
  * Renders the Playwright JSON report as Markdown for the run summary and the PR
  * comment. Prints to stdout; the workflow redirects it.
  *
- * Never exits non-zero: this is reporting, and it runs with `if: always()`, so a
- * missing or malformed report must not turn a passing run red or mask a real
- * failure with a crash here.
+ * ## What it reports, and why
+ *
+ * The first version printed totals and, on failure, the failures. That answers "did it pass"
+ * and nothing else — a green run said `188 passed` and left no record of *what* passed, so
+ * a spec that quietly stopped running (renamed, `test.skip`ped, dropped from a project's
+ * `testMatch`) looked exactly like a spec that ran and passed. Sharding makes that worse:
+ * a test can now vanish because a shard died before reaching it, and the total is the only
+ * place that would show it.
+ *
+ * So this reports three things beyond the verdict:
+ *
+ *   - a per-spec-file table, always visible, so a file that produced no tests is obvious;
+ *   - the full per-test list, folded into a <details> block so it does not bury the verdict;
+ *   - the slowest tests, which is what the shard counts should be tuned against.
+ *
+ * ## Size
+ *
+ * A GitHub PR comment caps at 65536 characters and the API rejects anything longer, so a
+ * suite large enough to overflow would silently lose its comment entirely — the workflow
+ * step is `continue-on-error`. `E2E_SUMMARY_MAX_BYTES` bounds the output: sections are
+ * dropped from the least important upward until it fits, and **what was dropped is always
+ * stated**. A summary that silently omits half the suite is worse than one that admits it.
+ *
+ * Never exits non-zero: this is reporting, and it runs with `if: always()`, so a missing or
+ * malformed report must not turn a passing run red or mask a real failure with a crash here.
  */
 
 import { readFileSync } from 'node:fs';
 
 const REPORT = process.env.PLAYWRIGHT_JSON_OUTPUT_NAME ?? 'playwright-results.json';
 
-function collect(suite, out) {
+/** Output budget. The default is generous; the PR-comment invocation passes a smaller one. */
+const MAX_BYTES = Number(process.env.E2E_SUMMARY_MAX_BYTES ?? 60000);
+
+/** How many tests the folded full listing will name before it truncates. */
+const MAX_LISTED = Number(process.env.E2E_SUMMARY_MAX_LISTED ?? 400);
+
+const ICON = {
+  passed: '✅',
+  failed: '❌',
+  timedOut: '⏱️',
+  skipped: '⏭️',
+  interrupted: '⚠️',
+};
+
+/**
+ * Strips ANSI escape sequences.
+ *
+ * Playwright embeds terminal colour codes in `error.message` — an assertion failure arrives as
+ * `Error: \x1b[2mexpect(\x1b[22m...`. They are invisible in a terminal and unreadable
+ * everywhere else, and GitHub renders them literally.
+ */
+// eslint-disable-next-line no-control-regex -- escape sequences are exactly the target
+const ANSI = /\u001B\[[0-9;]*m/g;
+
+/**
+ * Flattens the suite tree, accumulating `describe` titles.
+ *
+ * The root suite's title is the spec's file path, which is already the heading every test is
+ * rendered under, so including it would print `client.spec.ts › client.spec.ts › creates a
+ * client`. Ancestry below that is kept: a nested `describe` is context worth having.
+ */
+function collect(suite, out, titlePath = []) {
+  const file = suite.file ?? '';
+  const isFileSuite = !suite.title || suite.title === file || suite.title === suite.location?.file;
+  const path = isFileSuite ? titlePath : [...titlePath, suite.title];
+
   for (const spec of suite.specs ?? []) {
     for (const test of spec.tests ?? []) {
       const result = test.results?.[test.results.length - 1];
       out.push({
-        title: [...(suite.title ? [suite.title] : []), spec.title].join(' › '),
+        title: [...path, spec.title].join(' › '),
         file: spec.file ?? suite.file ?? '',
+        project: test.projectName ?? '',
         status: test.status === 'skipped' ? 'skipped' : (result?.status ?? test.status),
         expected: test.expectedStatus,
+        // A test that failed and then passed on retry is green overall but worth surfacing:
+        // it is the shape a flake takes, and flakes are what a sharded suite hides best.
+        retries: Math.max(0, (test.results?.length ?? 1) - 1),
         durationMs: result?.duration ?? 0,
-        error: result?.error?.message ?? '',
+        error: (result?.error?.message ?? '').replace(ANSI, ''),
       });
     }
   }
-  for (const child of suite.suites ?? []) collect(child, out);
+  for (const child of suite.suites ?? []) collect(child, out, path);
+}
+
+/** `1.2s`, or `340ms` below a second — a duration nobody has to convert in their head. */
+function humanDuration(ms) {
+  if (ms < 1000) return `${Math.round(ms)}ms`;
+  if (ms < 60000) return `${(ms / 1000).toFixed(1)}s`;
+  const minutes = Math.floor(ms / 60000);
+  return `${minutes}m ${Math.round((ms % 60000) / 1000)}s`;
+}
+
+/** Pipes would break out of the markdown table cell they are rendered into. */
+function cell(text) {
+  return String(text).replace(/\|/g, '\\|');

Review Comment:
   ## CodeQL / Incomplete string escaping or encoding
   
   This does not escape backslash characters in the input.
   
   [Show more details](https://github.com/apache/fineract-backoffice-ui/security/code-scanning/16)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org