Skip to content

fix: remove race conditions in progression tracking E2E test - #200

Merged
robert-bell merged 1 commit into
opendatahub-io:mainfrom
robert-bell:fix-test-race-condition
Aug 3, 2026
Merged

robert-bell merged 1 commit into
opendatahub-io:mainfrom
robert-bell:fix-test-race-condition

Conversation

@robert-bell

@robert-bell robert-bell commented Aug 3, 2026 •

Copy link
Copy Markdown
Collaborator

The "metrics endpoint unreachable" test had two race conditions:

  1. The Consistently block checking running status could race with job completion and annotation synthesis between status check and assertion
  2. The final assertion expected no annotation, but raced with the reconciler's fallback that synthesizes 100% on completion

Test was passing for 8 months probably because the race condition never got hit - the check that no annotation was added was done quickly, probably before the final reconcile happened.

Fix: Remove flaky assertions about annotation state. Focus on what matters - the controller handles metrics errors gracefully and the TrainJob completes successfully

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

  • Docs included if any changes are user facing

Summary by CodeRabbit

  • Tests
    • Updated end-to-end coverage to verify that TrainJobs complete successfully even when metrics connections encounter errors.

The "metrics endpoint unreachable" test had two race conditions:

1. The Consistently block checking running status could race with job
   completion and annotation synthesis between status check and assertion
2. The final assertion expected no annotation, but raced with the
   reconciler's fallback that synthesizes 100% on completion

Test was passing for 8 months probably because the race condition never got
hit - the check that no annotation was added was done quickly, probably before
the final reconcile happened.

Fix: Remove flaky assertions about annotation state. Focus on what
matters - the controller handles metrics errors gracefully and the
TrainJob completes successfully

Signed-off-by: Rob Bell <robell@redhat.com>
@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 96d17ead-b261-4ebf-a338-31b1020166af

📥 Commits

Reviewing files that changed from the base of the PR and between 60f558b and f5ffd6d.

📒 Files selected for processing (1)
  • test/e2e/rhai/progression_e2e_test.go

📝 Walkthrough

Walkthrough

The unreachable-metrics E2E test removes intermediate reconciliation checks and assertions that trainerStatus is absent. The test now focuses on verifying that the TrainJob completes despite metrics connection errors.

Estimated code review effort: 1 (Trivial) | ~3 minutes

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: removing race conditions from the progression tracking E2E test.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Contribution Quality And Spam Detection ✅ Passed The patch only removes race-prone assertions from one specific E2E test; it adds no security fix or validation logic, and the description gives code-specific race details.
No Hardcoded Secrets ✅ Passed The patch changes only one test description line; added lines contain no credentials, secret assignments, private keys, credential URLs, or long base64 strings (CWE-798).
No Weak Cryptography ✅ Passed The PR changes one E2E test message only; the changed file has no crypto imports, banned primitives, secret comparisons, custom crypto, or dependency/configuration changes.
No Injection Vectors ✅ Passed HEAD^..HEAD changes only test/e2e/rhai/progression_e2e_test.go. The added lines contain no SQL, shell, eval/exec, unsafe YAML, pickle, or HTML injection patterns under CWE-78, CWE-89, CWE-94, CWE-5...
No Privileged Containers ✅ Passed The PR changes only a test file and adds no privileged-container settings; no CWE-250/CWE-269 indicators appear in the added code.
No Sensitive Data In Logs ✅ Passed The PR only adds a static ginkgo.By message; it adds no log/print call, credentials, PII, or raw request/response data.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@abhijeet-dhumal abhijeet-dhumal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
Thanks @robert-bell 🙌

@robert-bell
robert-bell merged commit 0b26b60 into opendatahub-io:main Aug 3, 2026
14 checks passed
@rhods-ci-bot

Copy link
Copy Markdown

@robert-bell: The following test has Succeeded:

OCI Artifact Browser URL

View in Artifact Browser

Inspecting Test Artifacts Manually

To inspect your test artifacts manually, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:odh-pr-test-trainer-x49hf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants