refactor(ci): encapsulate test run tracking into lib/test-run-tracker.sh

[gustavolira]

Summary

• Introduces .ci/pipelines/lib/test-run-tracker.sh module that encapsulates the CURRENT_DEPLOYMENT global counter into a clean API (test_run_tracker::register, test_run_tracker::mark_deploy_success, test_run_tracker::mark_deploy_failed, test_run_tracker::mark_test_result)
• Replaces manual counter manipulation in utils.sh and lib/testing.sh with single-call functions, reducing error-prone boilerplate
• Removes the CURRENT_DEPLOYMENT export from reporting.sh — state is now managed internally by the test run tracker module

Test plan

• Verify grep -rn "CURRENT_DEPLOYMENT" .ci/ returns zero matches outside lib/test-run-tracker.sh
• Run shellcheck on modified files (lib/test-run-tracker.sh, lib/testing.sh, reporting.sh, utils.sh)
• Validate source chain: reporting.sh → lib/test-run-tracker.sh loads correctly
• Run CI pipeline to confirm functional equivalence (counter increments identically)

🤖 Generated with Claude Code

[github-actions]

The container image build workflow finished with status: cancelled.

[github-actions]

The container image build workflow finished with status: cancelled.

[github-actions]

The container image build workflow finished with status: cancelled.

[gustavolira]

/review

[rhdh-qodo-merge]

PR Reviewer Guide 🔍

(Review updated until commit fdca364)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🔒 Security concerns Supply chain / remote script execution: .ci/pipelines/install-methods/operator.sh downloads a script from GitHub based on ${RELEASE_BRANCH_NAME} and makes it executable. If that file is later executed (likely), this introduces a risk of running unpinned remote code. Consider pinning to a specific commit/tag, validating ${RELEASE_BRANCH_NAME} against an allowlist, and/or verifying integrity (checksum/signature) before execution.

⚡ Recommended focus areas for review State Consistency The deployment counter is intentionally not exported, while the functions are exported for subshell usage. Please validate that no call sites invoke deployment::* in a subshell (e.g., timeout bash -c ..., pipes, command substitutions) in a way that increments _DEPLOYMENT_COUNTER outside the parent shell, since this can desynchronize IDs from the parent process and lead to statuses being written under unexpected deployment indices. # Internal state _DEPLOYMENT_COUNTER=0 deployment::next_id() { _DEPLOYMENT_COUNTER=$((_DEPLOYMENT_COUNTER + 1)) echo "${_DEPLOYMENT_COUNTER}" } deployment::current_id() { echo "${_DEPLOYMENT_COUNTER}" } deployment::register() { local label="$1" deployment::next_id > /dev/null save_status_deployment_namespace "${_DEPLOYMENT_COUNTER}" "$label" } deployment::mark_deploy_success() { save_status_failed_to_deploy "${_DEPLOYMENT_COUNTER}" false } deployment::mark_deploy_failed() { local label="$1" deployment::register "$label" save_status_failed_to_deploy "${_DEPLOYMENT_COUNTER}" true save_status_test_failed "${_DEPLOYMENT_COUNTER}" true save_overall_result 1 } deployment::mark_test_result() { local passed="$1" local num_failures="${2:-}" if [[ "$passed" == "true" ]]; then save_status_test_failed "${_DEPLOYMENT_COUNTER}" false else save_status_test_failed "${_DEPLOYMENT_COUNTER}" true fi if [[ -n "$num_failures" ]]; then save_status_number_of_test_failed "${_DEPLOYMENT_COUNTER}" "$num_failures" fi } # Export all functions for subshell compatibility. # Note: _DEPLOYMENT_COUNTER is NOT exported because subshells inherit only # the snapshot at fork time — counter updates in the parent would not propagate. export -f deployment::next_id export -f deployment::current_id export -f deployment::register export -f deployment::mark_deploy_success export -f deployment::mark_deploy_failed export -f deployment::mark_test_result Double Registration deployment::mark_deploy_failed calls deployment::register, which always increments the counter. Confirm that failure paths never call deployment::mark_deploy_failed after an earlier deployment::register for the same logical deployment, otherwise you’ll get an extra deployment entry (and the failure will be attributed to the next ID rather than the already-registered one). deployment::register() { local label="$1" deployment::next_id > /dev/null save_status_deployment_namespace "${_DEPLOYMENT_COUNTER}" "$label" } deployment::mark_deploy_success() { save_status_failed_to_deploy "${_DEPLOYMENT_COUNTER}" false } deployment::mark_deploy_failed() { local label="$1" deployment::register "$label" save_status_failed_to_deploy "${_DEPLOYMENT_COUNTER}" true save_status_test_failed "${_DEPLOYMENT_COUNTER}" true save_overall_result 1 }

📚 Focus areas based on broader codebase context Exit Code Failure paths now call deployment::mark_deploy_failed ... but then return 0 to the caller (e.g., when Backstage is not running or a Helm upgrade times out). Validate that the top-level CI entrypoints still reliably fail the job (non-zero exit) when a deployment/test fails, rather than only recording OVERALL_RESULT. (Ref 1, Ref 5) else echo "Backstage is not running. Marking deployment as failed and continuing..." deployment::mark_deploy_failed "$artifacts_subdir" save_all_pod_logs "$namespace" return 0 fi # Collect pod logs only on test failure to speed up successful PR runs. local _current_id _current_id="$(deployment::current_id)" if [[ "${STATUS_TEST_FAILED[$_current_id]:-}" == "true" ]]; then save_all_pod_logs "$namespace" else log::info "Tests passed — skipping pod log collection for namespace: ${namespace}" fi return 0 } # ============================================================================== # Upgrade Verification # ============================================================================== # Check Helm upgrade rollout status # Args: # $1 - deployment_name: The name of the deployment # $2 - namespace: The namespace where the deployment is located # $3 - timeout: Timeout in seconds (default: 600) # Returns: # 0 - Upgrade completed successfully # 1 - Upgrade failed or timed out testing::check_helm_upgrade() { local deployment_name="$1" local namespace="$2" local timeout="${3:-600}" if [[ -z "$deployment_name" || -z "$namespace" ]]; then log::error "${_TESTING_ERR_MISSING_PARAMS}" log::info "Usage: testing::check_helm_upgrade <deployment_name> <namespace> [timeout]" return 1 fi log::info "Checking rollout status for deployment: ${deployment_name} in namespace: ${namespace}..." if oc rollout status "deployment/${deployment_name}" -n "${namespace}" --timeout="${timeout}s" -w; then log::info "RHDH upgrade is complete." return 0 else log::error "RHDH upgrade encountered an issue or timed out." return 1 fi } # Check upgrade and run tests if successful # Args: # $1 - deployment_name: The name of the deployment # $2 - release_name: The Helm release name # $3 - namespace: The namespace # $4 - playwright_project: The Playwright project to run # $5 - url: The URL to test against # $6 - timeout: (optional) Timeout in seconds (default: 600) testing::check_upgrade_and_test() { local deployment_name="$1" local release_name="$2" local namespace="$3" local playwright_project="$4" local url=$5 local timeout=${6:-600} if [[ -z "$deployment_name" || -z "$release_name" || -z "$namespace" || -z "$playwright_project" || -z "$url" ]]; then log::error "${_TESTING_ERR_MISSING_PARAMS}" log::info "Usage: testing::check_upgrade_and_test <deployment_name> <release_name> <namespace> <playwright_project> <url> [timeout]" return 1 fi if testing::check_helm_upgrade "${deployment_name}" "${namespace}" "${timeout}"; then testing::check_and_test "${release_name}" "${namespace}" "${playwright_project}" "${url}" else log::error "Helm upgrade encountered an issue or timed out. Exiting..." deployment::mark_deploy_failed "$namespace" fi return 0 Reference reasoning: Existing e2e runner scripts treat deployment/container failures as hard failures by exiting with the captured non-zero exit code. This indicates the expected behavior is to propagate failure via process exit status, not just via reporting state files.

📄 References redhat-developer/rhdh/e2e-tests/container-init.sh [177-182] redhat-developer/rhdh/e2e-tests/container-init.sh [136-175] redhat-developer/rhdh/e2e-tests/container-init.sh [86-117] redhat-developer/rhdh/e2e-tests/container-init.sh [47-84] redhat-developer/rhdh/e2e-tests/local-run.sh [442-470] redhat-developer/rhdh/e2e-tests/local-run.sh [403-426] redhat-developer/rhdh/e2e-tests/local-run.sh [338-360] redhat-developer/rhdh/e2e-tests/local-run.sh [1-12]

[gustavolira]

/retest

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4366
• quay.io/rhdh-community/rhdh:pr-4366-ce7a4060

[zdrapela]

/test e2e-ocp-helm-nightly

[zdrapela]

Please make sure that each file that uses functions from other files sources all the files that it depends on - to ensure dependencies.

We should definitely check that for job triggered by /test e2e-ocp-helm-nightly, all the files in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/redhat-developer_rhdh/4366/pull-ci-redhat-developer-rhdh-main-e2e-ocp-helm-nightly/2029492297959215104/artifacts/e2e-ocp-helm-nightly/redhat-developer-rhdh-ocp-helm-nightly/artifacts/reporting/ have the correct number of entries.

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4366
• quay.io/rhdh-community/rhdh:pr-4366-88355bec

[github-actions]

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

[zdrapela]

/review
-i

[rhdh-qodo-merge]

Persistent review updated to latest commit fdca364

[zdrapela]

/agentic_review

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Missing failure-count entry 🐞 Bug ✧ Quality

Description

deployment::mark_test_result only writes STATUS_NUMBER_OF_TEST_FAILED when a non-empty value is
provided; testing::run_tests can pass an empty value if JUnit parsing returns no match, and
deployment-failure paths don’t set a placeholder count. This can lead to missing lines in
STATUS_NUMBER_OF_TEST_FAILED.txt, reducing report completeness and risking misalignment with other
per-deployment status files.

Code

.ci/pipelines/lib/deployment.sh[R42-52]

+deployment::mark_test_result() {
+  local passed="$1"
+  local num_failures="${2:-}"
+  if [[ "$passed" == "true" ]]; then
+    save_status_test_failed "${_DEPLOYMENT_COUNTER}" false
+  else
+    save_status_test_failed "${_DEPLOYMENT_COUNTER}" true
+  fi
+  if [[ -n "$num_failures" ]]; then
+    save_status_number_of_test_failed "${_DEPLOYMENT_COUNTER}" "$num_failures"
+  fi

Evidence

The deployment module conditionally persists failure counts only when num_failures is non-empty,
so any caller passing an empty string results in no line being appended to
STATUS_NUMBER_OF_TEST_FAILED.txt. testing::run_tests populates failed_tests via grep when
the JUnit file exists but does not default it if grep returns an empty string, and then passes it
through to deployment::mark_test_result. The documentation explicitly describes
STATUS_NUMBER_OF_TEST_FAILED.txt as having failure counts “one per line,” implying each deployment
should contribute an entry.

.ci/pipelines/lib/deployment.sh[42-53]
.ci/pipelines/lib/testing.sh[112-124]
.ci/pipelines/reporting.sh[47-53]
docs/e2e-tests/enhanced-ci-reporting.md[72-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`STATUS_NUMBER_OF_TEST_FAILED.txt` can miss an entry for a deployment because `deployment::mark_test_result` only persists the count when the value is non-empty, and callers may pass an empty string (e.g., if JUnit parsing yields no match). This reduces reporting completeness and can desynchronize per-deployment status files.

### Issue Context
- `deployment::mark_test_result` gates writing the failure count on `[[ -n "$num_failures" ]]`.
- `testing::run_tests` computes `failed_tests` via `grep` when the JUnit file exists, but does not default it if `grep` yields an empty result.
- Docs describe `STATUS_NUMBER_OF_TEST_FAILED.txt` as "one per line", implying each deployment should contribute an entry.

### Fix Focus Areas
- .ci/pipelines/lib/deployment.sh[34-53]
- .ci/pipelines/lib/testing.sh[112-124]
- (optional for consistency) .ci/pipelines/lib/deployment.sh[34-40]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit fdca364

Results up to commit N/A

Looking for bugs?

Come back again in a few minutes. An AI review agent is analysing this pull request

[zdrapela]

/agentic_review

[rhdh-qodo-merge]

Persistent review updated to latest commit fdca364

[zdrapela]

/test e2e-ocp-helm-nightly

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4366
• quay.io/rhdh-community/rhdh:pr-4366-d522a3d6

[github-actions]

The container image build workflow finished with status: cancelled.

[github-actions]

The container image build workflow finished with status: cancelled.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4366
• quay.io/rhdh-community/rhdh:pr-4366-798a2a02

[zdrapela]

/test e2e-ocp-helm-nightly

[zdrapela]

/test e2e-ocp-helm

[zdrapela]

Thank you! It's now looking great. I'll just wait for the nightly job to finish, and I'll approve.