NO-ISSUE: ci(e2e): add structured test reporting to KinD e2e workflow - #1254
Conversation
|
@jbpratt: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughE2E workflow updated to create /tmp/e2e-reports, run make with Chainsaw JUnit-STEP output, always collect diagnostics (non-fatal), post-process and publish JUnit XML (rewrite testcase classname), write a job summary (warn if XML missing), upload reports; .gitignore updated. Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
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)
.github/workflows/e2e-kind.yaml (1)
69-96:⚠️ Potential issue | 🟠 MajorMake the diagnostics step resilient to early failures.
This
always()step can still abort before collecting useful output. If anything fails before Line 55 creates/tmp/e2e-reports, Line 72 cannot opendiagnostics.log; and if no namespace matchesgrep chainsaw, the namespace loop can fail instead of just producing no per-namespace logs. That weakens the exact failure path this step is supposed to preserve.🔧 Suggested change
- name: Collect diagnostic logs if: always() run: | + mkdir -p /tmp/e2e-reports exec > >(tee /tmp/e2e-reports/diagnostics.log) 2>&1 echo "=== Pod status (all namespaces) ===" kubectl get pods -A -o wide || true echo "" echo "=== QuayRegistry status ===" kubectl get quayregistries -A -o yaml 2>/dev/null || true echo "" echo "=== Events ===" kubectl get events -A --sort-by='.lastTimestamp' | tail -50 || true echo "" echo "=== Operator logs (last 100 lines) ===" tail -100 /tmp/operator.log 2>/dev/null || true echo "" echo "=== Chainsaw namespace resources ===" - for ns in $(kubectl get ns -o name | grep chainsaw | sed 's|namespace/||'); do + while IFS= read -r ns; do + ns="${ns#namespace/}" echo "--- $ns: pod describe ---" kubectl describe pods -n "$ns" 2>/dev/null || true echo "--- $ns: pod logs ---" for pod in $(kubectl get pods -n "$ns" -o name 2>/dev/null); do echo "--- $ns/$pod ---" kubectl logs -n "$ns" "$pod" --all-containers --tail=50 2>/dev/null || true echo "--- $ns/$pod (previous) ---" kubectl logs -n "$ns" "$pod" --all-containers --previous --tail=50 2>/dev/null || true done - done + done < <(kubectl get ns -o name 2>/dev/null | grep chainsaw || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 69 - 96, Create the /tmp/e2e-reports directory and touch /tmp/e2e-reports/diagnostics.log before the exec > >(tee ...) redirection so the redirection cannot fail (update the diagnostics step to run mkdir -p /tmp/e2e-reports && : >/tmp/e2e-reports/diagnostics.log then exec ...). Make the Chainsaw namespace loop tolerant of no matches by sourcing namespaces with a safe construct (e.g., use kubectl get ns -o name | grep chainsaw || true and iterate via while read -r ns; do [ -z "$ns" ] && continue; ... done < <(...)) or guard the for-loop with an if that checks for non-empty output; also ensure kubectl get pods and inner pod list commands use "|| true" so missing pods/namespaces don’t cause the diagnostics step to abort (references: diagnostics.log, /tmp/e2e-reports, the Chainsaw namespace loop that uses kubectl get ns | grep chainsaw, and the pod/pod-logs kubectl commands).
🧹 Nitpick comments (1)
.github/workflows/e2e-kind.yaml (1)
98-107: Disable the action’s built-in job summary if you want the custom one to be authoritative.
mikepenz/action-junit-reportalready publishes a job summary by default, so the next step will likely duplicate the report in$GITHUB_STEP_SUMMARY. If the Python summary is the intended UX, setjob_summary: falsehere. (github.com)🔧 Suggested change
- name: Publish JUnit test report if: always() uses: mikepenz/action-junit-report@bccf2e31636835cf0874589931c4116687171386 # v6.4.0 with: report_paths: '/tmp/e2e-reports/chainsaw-report.xml' check_name: 'E2E Test Results (KinD)' fail_on_failure: false require_tests: false include_passed: true detailed_summary: true + job_summary: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 98 - 107, The Publish JUnit test report step (uses: mikepenz/action-junit-report@bccf2e31636835cf0874589931c4116687171386) is producing a built-in job summary that may duplicate your custom Python summary; set the action input job_summary: false in that step so the action does not write to $GITHUB_STEP_SUMMARY and only your custom summary is shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 54-59: The workflow invocation of the make target test-e2e-kind
doesn't set CHAINSAW_REPORT_NAME but later assumes
/tmp/e2e-reports/chainsaw-report.xml; update the run block that calls make
test-e2e-kind to explicitly pass CHAINSAW_REPORT_NAME=chainsaw-report (alongside
CHAINSAW_REPORT_FORMAT, CHAINSAW_REPORT_PATH, and CHAINSAW_EXTRA_ARGS) so the
produced report filename matches the later hardcoded references and avoids
relying on Chainsaw defaults.
---
Outside diff comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 69-96: Create the /tmp/e2e-reports directory and touch
/tmp/e2e-reports/diagnostics.log before the exec > >(tee ...) redirection so the
redirection cannot fail (update the diagnostics step to run mkdir -p
/tmp/e2e-reports && : >/tmp/e2e-reports/diagnostics.log then exec ...). Make the
Chainsaw namespace loop tolerant of no matches by sourcing namespaces with a
safe construct (e.g., use kubectl get ns -o name | grep chainsaw || true and
iterate via while read -r ns; do [ -z "$ns" ] && continue; ... done < <(...)) or
guard the for-loop with an if that checks for non-empty output; also ensure
kubectl get pods and inner pod list commands use "|| true" so missing
pods/namespaces don’t cause the diagnostics step to abort (references:
diagnostics.log, /tmp/e2e-reports, the Chainsaw namespace loop that uses kubectl
get ns | grep chainsaw, and the pod/pod-logs kubectl commands).
---
Nitpick comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 98-107: The Publish JUnit test report step (uses:
mikepenz/action-junit-report@bccf2e31636835cf0874589931c4116687171386) is
producing a built-in job summary that may duplicate your custom Python summary;
set the action input job_summary: false in that step so the action does not
write to $GITHUB_STEP_SUMMARY and only your custom summary is shown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e889ebbd-924c-4d24-a5bc-c348e8cd18d5
📒 Files selected for processing (3)
.github/workflows/e2e-kind.yaml.gitignoretest/chainsaw/Makefile
484744f to
65d8c94
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/e2e-kind.yaml (1)
72-75:⚠️ Potential issue | 🟠 MajorCreate
/tmp/e2e-reportsin thealways()diagnostics step too.If any earlier step fails before Lines 58-61 run, this block still executes but Line 75 tries to write
diagnostics.loginto a directory that was never created. That can drop the very diagnostics this step is meant to preserve.🔧 Suggested change
- name: Collect diagnostic logs if: always() run: | + mkdir -p /tmp/e2e-reports exec > >(tee /tmp/e2e-reports/diagnostics.log) 2>&1 echo "=== Pod status (all namespaces) ===" kubectl get pods -A -o wide || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 72 - 75, The "Collect diagnostic logs" job step currently redirects output into /tmp/e2e-reports/diagnostics.log using the line exec > >(tee /tmp/e2e-reports/diagnostics.log) 2>&1 but never ensures the /tmp/e2e-reports directory exists; add a mkdir -p /tmp/e2e-reports (or equivalent) immediately before that exec line in the "Collect diagnostic logs" step so the directory is created even if earlier steps failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 101-109: The "Publish JUnit test report" GitHub Actions step
(uses: mikepenz/action-junit-report) can fail on forked PRs due to read-only
GITHUB_TOKEN; update that step to avoid failing the workflow by adding
continue-on-error: true to the step (or alternatively set annotate_only: true in
the action inputs to avoid needing checks: write), ensuring the step name
"Publish JUnit test report" and the existing inputs (report_paths, check_name,
fail_on_failure, include_passed) remain unchanged.
- Around line 122-173: Wrap the ET.parse(report) call in a try/except that
catches ET.ParseError and OSError so malformed/truncated JUnit XML doesn't crash
the reporting step; if an exception occurs, open the summary_file (from
os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null')) and append a warning line
(e.g. note that parsing report failed and include the exception message and the
report path), then exit the script gracefully (skip the rest of the processing).
Ensure you reference ET.parse and the report variable in the change so the error
handling is directly around the parse operation.
---
Duplicate comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 72-75: The "Collect diagnostic logs" job step currently redirects
output into /tmp/e2e-reports/diagnostics.log using the line exec > >(tee
/tmp/e2e-reports/diagnostics.log) 2>&1 but never ensures the /tmp/e2e-reports
directory exists; add a mkdir -p /tmp/e2e-reports (or equivalent) immediately
before that exec line in the "Collect diagnostic logs" step so the directory is
created even if earlier steps failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b28e5fd-ace7-4e95-a062-d9a7eb7c979d
📒 Files selected for processing (3)
.github/workflows/e2e-kind.yaml.gitignoretest/chainsaw/Makefile
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- test/chainsaw/Makefile
Generate JUnit reports from chainsaw, publish to PR checks tab, write job summary, and upload diagnostic artifacts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brady Pratt <bpratt@redhat.com>
65d8c94 to
b449089
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/e2e-kind.yaml (1)
174-196: Consider using a context manager for file handling.The file is opened on line 174 and closed on line 196. Using a
withstatement would be more Pythonic and ensure the file is closed even if an exception occurs.♻️ Suggested refactor
- out = open(os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null'), 'a') + with open(os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null'), 'a') as out: icon = ':white_check_mark:' if failed == 0 else ':x:' # ... (indent the following lines) - out.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 174 - 196, Replace the manual open/close pattern for the step-summary file with a context manager: wrap the open(os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null'), 'a') call in a with statement so all subsequent writes (the header writes using out.write, the loop over suite_data and step writes, and the conditional failed-step messages) execute inside the with block, and remove the explicit out.close() at the end; maintain the same variables/icons/formatting (failed, total, passed, skipped, suite_data, fmt, icons) and identical output content and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 101-113: The inline Python snippet that calls ET.parse on REPORT
can raise xml.etree.ElementTree.ParseError for truncated/malformed XML and
currently will crash the step; wrap the parse and subsequent processing in a
try/except that catches ET.ParseError (and optionally Exception), write a clear
warning to stderr including the REPORT path and the exception text, and exit
cleanly (return code 0) so the workflow doesn't crash this post-step; update
references to the REPORT variable and the ET.parse call in the inline script
accordingly.
---
Nitpick comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 174-196: Replace the manual open/close pattern for the
step-summary file with a context manager: wrap the
open(os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null'), 'a') call in a with
statement so all subsequent writes (the header writes using out.write, the loop
over suite_data and step writes, and the conditional failed-step messages)
execute inside the with block, and remove the explicit out.close() at the end;
maintain the same variables/icons/formatting (failed, total, passed, skipped,
suite_data, fmt, icons) and identical output content and ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97ef0e4a-a5ca-49ea-b9b5-9e286d244aaa
📒 Files selected for processing (3)
.github/workflows/e2e-kind.yaml.gitignoretest/chainsaw/Makefile
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- test/chainsaw/Makefile
…eports Add checks:write permission for JUnit check creation, post-process chainsaw XML to populate empty classnames for suite grouping, redesign job summary with per-suite collapsible tables, and guard XML parsing against malformed reports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-kind.yaml (1)
89-99:⚠️ Potential issue | 🟡 MinorMake namespace discovery tolerant of “no matches”.
On GitHub Actions' default
bash -eo pipefail,grep chainsawreturning 1 can abort this diagnostics step when no Chainsaw namespace exists yet. That turns a best-effort collector into another failure source.🔧 Suggested change
- for ns in $(kubectl get ns -o name | grep chainsaw | sed 's|namespace/||'); do + mapfile -t chainsaw_namespaces < <( + kubectl get ns -o name 2>/dev/null | sed -n 's|^namespace/\(chainsaw.*\)$|\1|p' || true + ) + for ns in "${chainsaw_namespaces[@]}"; do echo "--- $ns: pod describe ---" kubectl describe pods -n "$ns" 2>/dev/null || true echo "--- $ns: pod logs ---"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 89 - 99, The namespace discovery pipeline (the for ns in $(kubectl get ns -o name | grep chainsaw | sed 's|namespace/||'); do ...) can fail under bash -eo pipefail when grep returns no matches; make it tolerant by ensuring grep's non-zero exit doesn't break the command substitution — e.g., change the pipeline so grep chainsaw cannot fail the shell (for example append "|| true" to the grep stage or otherwise guard the entire substitution), leaving the rest of the loop (kubectl describe pods -n "$ns", kubectl logs -n "$ns" ...) unchanged.
♻️ Duplicate comments (2)
.github/workflows/e2e-kind.yaml (2)
118-126:⚠️ Potential issue | 🟠 MajorThis can still fail on fork PRs.
checks: writeis not available to the default token on forkedpull_requestruns, so this action can still error for external contributors. Please keep the existing behavior but add a safeguard likecontinue-on-error: trueor switch toannotate_only: true.For GitHub Actions workflows triggered by pull_request from forks, does GITHUB_TOKEN get checks: write, and can mikepenz/action-junit-report create a Check Run without that permission?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 118 - 126, The "Publish JUnit test report" step using mikepenz/action-junit-report can still fail on forked PRs due to missing checks: write; update that step (the step named "Publish JUnit test report" that uses mikepenz/action-junit-report) to guard against permission errors by either adding continue-on-error: true at the step level or setting annotate_only: true in the action inputs (with:), keeping report_paths, check_name, fail_on_failure, require_tests, and include_passed unchanged.
145-146:⚠️ Potential issue | 🟡 MinorHandle malformed XML in the summary step too.
The classname fixer is already best-effort, but this second
ET.parse(report)is still unguarded and can crash the always-run summary step on a truncated report.🔧 Suggested change
report = sys.argv[1] - tree = ET.parse(report) - root = tree.getroot() + summary_path = os.environ.get('GITHUB_STEP_SUMMARY', '/dev/null') + try: + tree = ET.parse(report) + root = tree.getroot() + except (OSError, ET.ParseError) as exc: + with open(summary_path, 'a') as out: + out.write('## E2E Test Results (KinD)\n\n') + out.write(f'> **Warning:** Failed to parse `{report}`: {exc}\n') + raise SystemExit(0)What exceptions does Python's xml.etree.ElementTree.parse raise for malformed XML and for missing files?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-kind.yaml around lines 145 - 146, The second unguarded ET.parse(report) in the summary step can raise xml.etree.ElementTree.ParseError for malformed XML and FileNotFoundError (or OSError/IOError on I/O issues); wrap the ET.parse(report) call in a try/except that catches ET.ParseError and FileNotFoundError (and optionally OSError) and handle failures gracefully (e.g., log the error and skip summary generation or set tree/root to None and guard subsequent logic). Locate the ET.parse(report) usage (the tree = ET.parse(report) / root = tree.getroot() lines) and add the try/except and null-guarding to avoid crashing the always-run summary step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 72-75: The "Collect diagnostic logs" always-run step currently
pipes output into /tmp/e2e-reports/diagnostics.log but may fail if the
/tmp/e2e-reports directory wasn't created earlier; update this step (named
"Collect diagnostic logs") to ensure the directory exists before the exec/tee
line by running mkdir -p /tmp/e2e-reports (or equivalent) at the start of the
step so tee can open /tmp/e2e-reports/diagnostics.log reliably.
- Around line 109-113: The current loop unconditionally overwrites testcase
'classname' attributes; change it to only set classname when the testcase has no
existing 'classname' (i.e., if not tc.get('classname') or tc.get('classname') ==
''). In the block that iterates suites and testcases (symbols: tree, suite, tc),
add a guard that checks tc.get('classname') and only call tc.set('classname',
suite.get('name', '')) when the classname is missing/empty so existing non-empty
classnames are preserved.
---
Outside diff comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 89-99: The namespace discovery pipeline (the for ns in $(kubectl
get ns -o name | grep chainsaw | sed 's|namespace/||'); do ...) can fail under
bash -eo pipefail when grep returns no matches; make it tolerant by ensuring
grep's non-zero exit doesn't break the command substitution — e.g., change the
pipeline so grep chainsaw cannot fail the shell (for example append "|| true" to
the grep stage or otherwise guard the entire substitution), leaving the rest of
the loop (kubectl describe pods -n "$ns", kubectl logs -n "$ns" ...) unchanged.
---
Duplicate comments:
In @.github/workflows/e2e-kind.yaml:
- Around line 118-126: The "Publish JUnit test report" step using
mikepenz/action-junit-report can still fail on forked PRs due to missing checks:
write; update that step (the step named "Publish JUnit test report" that uses
mikepenz/action-junit-report) to guard against permission errors by either
adding continue-on-error: true at the step level or setting annotate_only: true
in the action inputs (with:), keeping report_paths, check_name, fail_on_failure,
require_tests, and include_passed unchanged.
- Around line 145-146: The second unguarded ET.parse(report) in the summary step
can raise xml.etree.ElementTree.ParseError for malformed XML and
FileNotFoundError (or OSError/IOError on I/O issues); wrap the ET.parse(report)
call in a try/except that catches ET.ParseError and FileNotFoundError (and
optionally OSError) and handle failures gracefully (e.g., log the error and skip
summary generation or set tree/root to None and guard subsequent logic). Locate
the ET.parse(report) usage (the tree = ET.parse(report) / root = tree.getroot()
lines) and add the try/except and null-guarding to avoid crashing the always-run
summary step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9684b2e0-a5a8-4504-a1bd-a4c585df0538
📒 Files selected for processing (1)
.github/workflows/e2e-kind.yaml
Set job_summary: false on mikepenz/action-junit-report to prevent its auto-generated table, and replace the top-level totals table with a compact pass count in the heading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The custom job summary step provides better per-suite reporting. The third-party action added no value beyond a check run that failed on fork PRs due to permissions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use Unicode emoji instead of shortcodes, wrap all suites in <details> with failed suites using <details open>, drop redundant Status column from passing suites, and remove bold markdown that doesn't render inside <summary> tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generate JUnit reports from chainsaw, publish to PR checks tab,
write job summary, and upload diagnostic artifacts.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com