Skip to content

fix(egress): replace reruns with sleep in credential injection tests#1019

Open
averevki wants to merge 1 commit into
Kuadrant:mainfrom
averevki:remove-reruns-from-cred-injections
Open

fix(egress): replace reruns with sleep in credential injection tests#1019
averevki wants to merge 1 commit into
Kuadrant:mainfrom
averevki:remove-reruns-from-cred-injections

Conversation

@averevki

@averevki averevki commented Jul 15, 2026

Copy link
Copy Markdown
Member

Description

Reruns on this test break nightly runs for some reason. Replacing reruns with the small sleep

Summary by CodeRabbit

  • Bug Fixes

    • Improved the reliability of credential-injection scenarios by allowing policy changes sufficient time to take effect before validation.
    • Reduced dependence on automatic test reruns by improving test timing and stability.
  • Tests

    • Updated egress credential-injection coverage to better reflect policy-enforcement timing.
    • Preserved existing authentication, routing, and credential validation behaviour.

Signed-off-by: averevki <sandyverevkin@gmail.com>
@averevki averevki self-assigned this Jul 15, 2026
@averevki
averevki requested a review from silvi-t July 15, 2026 10:59
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The credential-injection test modules now wait for policy enforcement and no longer use flaky retry markers. One module adds an autouse delay fixture, while the destination-specific module adds the delay to its existing fixture.

Changes

Credential injection test synchronisation

Layer / File(s) Summary
Policy enforcement waits and marker updates
testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py, testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py
Both modules remove the flaky marker configuration and add a 10-second wait before credential-injection tests run against committed policies.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: Test case

Suggested reviewers: crstrn13, silvi-t

Poem

I’m a rabbit with tests in my burrow,
Waiting ten seconds—no need to hurry.
Flaky retries hop away,
Policies settle into play,
And credentials pass safely—furry!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description states the goal, but it omits the required Changes and Verification sections. Add the missing Changes and Verification sections, and briefly explain how the change was tested.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes replacing reruns with sleep in the credential injection egress tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py (1)

171-176: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Use condition-based enforcement synchronisation in both modules.

A fixed ten-second delay can still race on slow clusters and wastes time on fast clusters. Replace it with bounded polling of a concrete enforcement signal where possible.

  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py#L171-L176: replace the autouse sleep(10) fixture with condition-based polling.
  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py#L185-L185: apply the same bounded enforcement wait after both policies report ready.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py`
around lines 171 - 176, Replace the fixed sleep in the autouse commit fixture of
test_credential_injection.py with bounded polling for a concrete
policy-enforcement signal. Apply the same condition-based enforcement wait in
test_credential_injection_by_destination.py after both policies report ready;
update both specified sites and preserve bounded timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py`:
- Around line 171-176: Replace the fixed sleep in the autouse commit fixture of
test_credential_injection.py with bounded polling for a concrete
policy-enforcement signal. Apply the same condition-based enforcement wait in
test_credential_injection_by_destination.py after both policies report ready;
update both specified sites and preserve bounded timeout behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 64d818b3-0234-4e2a-900b-459400800dbd

📥 Commits

Reviewing files that changed from the base of the PR and between 3b4fca8 and c6f01b7.

📒 Files selected for processing (2)
  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py
  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py

@silvi-t

silvi-t commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@averevki I would suggest closing this PR, as the actual cause of the nightly failure was likely that the security quotas in OpenStack had been exhausted. I checked the nightly build today, and the egress tests passed and the rerun helped them.

@averevki

Copy link
Copy Markdown
Member Author

That's good, let's follow nightly results for a couple more days to make sure the reruns is indeed enough, then I'll close this PR as not needed 👍

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.

2 participants