USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730
Draft
agullon wants to merge 3 commits into
Draft
USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730agullon wants to merge 3 commits into
agullon wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6746 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
Contributor
|
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:
WalkthroughAdds router E2E test assets/manifests, a Robot Framework shared resource library, two Robot suites (routes and router-config), and scenario/CI scripts to run them. ChangesRouter End-to-End Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@test/assets/router/web-server-signed-deploy.yaml`:
- Around line 50-58: The nginx container currently lacks an explicit
securityContext; update the container spec for the container named "nginx" to
add a securityContext that enforces non-root execution and reduces privileges
(e.g., set runAsNonRoot: true, runAsUser to a non-root UID such as 1000, set
allowPrivilegeEscalation: false, set readOnlyRootFilesystem: true, and drop
capabilities like ["ALL"]); place this securityContext block under the nginx
container entry so the pod explicitly hardens runtime privileges.
In `@test/resources/router.resource`:
- Around line 131-137: The JsonPath currently used in the "Router Pod Env Should
Have Value" keyword queries all router pods via
.items[*].spec.containers[*].env[...] which can return multiple values during
rollouts; change the Oc Get JsonPath call to scope to a single pod (e.g., use
.items[0].spec.containers[*].env[?(@.name=="${env_name}")].value) so only one
value is returned, keeping the rest of the keyword (Should Be Equal As Strings
and variables like ${ROUTER_NS} and ${env_name}) unchanged.
In `@test/suites/router/router-config.robot`:
- Around line 722-723: The assertion is checking the wrong truncated hostname
and the comment is wrong; change the test that uses Wait For Router Logs To
Contain from waiting for "route-unsec82003.ap" to the correctly truncated
16-character hostname "route-unsec82003" and update the preceding comment to
state that the full hostname should be truncated to "route-unsec82003" (16
chars) when maxLength: 16; locate the assertion by the call to Wait For Router
Logs To Contain and the nearby comment in the router-config.robot test and
replace both the expected string and the comment to reflect the correct
16-character truncation.
In `@test/suites/router/router-routes.robot`:
- Around line 171-179: Add admission assertions after each route creation: after
the Create OC Route calls for route-http, route-edge, and route-passth call the
Route Should Be Admitted keyword (same as for route-reen) so every created route
is verified before curl checks; also mirror this change in the other identical
route-creation block that creates route-http, route-edge and route-passth so all
created routes are gated on admission.
- Around line 117-120: The test currently assigns ${env} from Oc Get JsonPath
and then asserts it equals a single "true", which fails when multiple router
pods produce values like "true true"; update the assertion so it verifies every
returned env value for ROuTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK is "true" (e.g.,
split ${env} into items or iterate over the list and assert each item == "true",
or use a collection-level assertion) instead of comparing the aggregated string
with a single "true"; adjust the steps surrounding Oc Get JsonPath / ${env} and
replace the single-string check (Should Be Equal As Strings ${env} true) with a
per-item check so multi-pod results pass.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ebedf450-e7d9-43c3-85aa-30293f17a3da
📒 Files selected for processing (22)
robocop.tomltest/assets/router/error-page-404.httptest/assets/router/error-page-503.httptest/assets/router/microshift-ingress-destca.yamltest/assets/router/microshift-ingress-http.yamltest/assets/router/rsyslogd-pod.yamltest/assets/router/test-client-pod.yamltest/assets/router/web-server-deploy.yamltest/assets/router/web-server-signed-deploy.yamltest/resources/router.resourcetest/scenarios-bootc/el10/presubmits/el102-src@router-extended.shtest/scenarios-bootc/el10/presubmits/el102-src@router.shtest/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.shtest/scenarios-bootc/el9/presubmits/el98-src@router-extended.shtest/scenarios-bootc/el9/presubmits/el98-src@router.shtest/scenarios-bootc/el9/releases/el98-lrel@osconfig-router.shtest/scenarios/presubmits/el98-src@router-extended.shtest/scenarios/presubmits/el98-src@router.shtest/scenarios/releases/el98-lrel@router-extended.shtest/scenarios/releases/el98-lrel@router.shtest/suites/router/router-config.robottest/suites/router/router-routes.robot
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/scenarios-bootc/el9/releases/el98-lrel`@router-extended.sh:
- Around line 15-16: The test script incorrectly calls exit_if_image_not_found
in teardown/test phases which can silently skip cleanup or tests; remove the
exit_if_image_not_found invocations from scenario_remove_vms and
scenario_run_tests and leave the guard only in scenario_create_vms so image
availability only blocks VM creation; locate and delete the
exit_if_image_not_found "${start_image}" calls in the scenario_remove_vms and
scenario_run_tests blocks (also check the duplicate occurrences noted around the
other invocation) and ensure no other teardown/test functions call
exit_if_image_not_found.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ee327aa5-6071-49cd-bfcc-37af5609b3c0
📒 Files selected for processing (2)
test/scenarios-bootc/el10/releases/el102-lrel@router-extended.shtest/scenarios-bootc/el9/releases/el98-lrel@router-extended.sh
✅ Files skipped from review due to trivial changes (1)
- test/scenarios-bootc/el10/releases/el102-lrel@router-extended.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@test/resources/router.resource`:
- Around line 102-105: The helper currently forces a HEAD request by passing -I
to curl in the oc exec call (the line using "oc exec ... -- curl ${url} -sI
--resolve ${resolve} --connect-timeout 10 ${flags}"), which strips the body;
remove the hardcoded -I so the command becomes "-s --resolve ${resolve}
--connect-timeout 10 ${flags}" (or otherwise omit -I) and rely on the existing
${flags} argument to opt into HEAD when callers need it; update the Robot
keyword that wraps Run With Kubeconfig to use the modified curl invocation so
downstream checks (e.g. custom 503 error-page assertions) can see response
bodies.
- Around line 338-348: The wrapper keyword "Generate Client Cert File In Dir" is
incorrectly prepending "/CN=" before the ${cn} when calling "Generate CSR And
Key", causing double "/CN=/CN=..." for callers that already provide a full
subject; modify the call to Generate CSR And Key to pass ${cn} unchanged (remove
the hardcoded "/CN=" prefix) so the subject provided by callers is forwarded
verbatim.
In `@test/suites/router/router-config.robot`:
- Around line 702-710: The test "Verify Custom LB Ports And IP" currently uses
"Get LB IPs" + "Should Contain", which allows extra IPs; change it to assert
exclusivity by checking the list length and membership: after calling ${lb_ips}=
Get LB IPs add "Length Should Be ${lb_ips} 1" and then keep or add "Should
Contain ${lb_ips} ${host_ip}" (or replace both with creating @{expected}
with ${host_ip} and use "Lists Should Be Equal ${lb_ips} ${expected}").
This ensures the router-default LB exposes exactly the single custom ${host_ip}.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 924210dd-c568-4210-9330-ea0caa00cd47
📒 Files selected for processing (3)
test/resources/router.resourcetest/suites/router/router-config.robottest/suites/router/router-routes.robot
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Contributor
Author
|
Addressed CodeRabbit inline suggestions:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/retest |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold until dynamic scheduling is implemented because this PR adds 5 new scenarios targetting latest release version |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/retest |
Contributor
Author
|
/cancel |
Contributor
Author
|
/cancel |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/test e2e-aws-tests-release |
Contributor
Author
|
/cancel |
Contributor
Author
|
/cancel |
Contributor
Author
|
/cancel |
Port 23 router end-to-end test cases from openshift-tests-private (Go/Ginkgo OTP suite) to Robot Framework. The tests are organized into 6 suites covering basic routes, route configuration, TLS, mTLS, access logging, and infrastructure settings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Each router .robot suite now runs as its own release scenario, allowing them to execute in parallel on separate VMs instead of sequentially on a single VM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
…or resources Apply the create-run-shutdown pattern (already used by the C2CC jobs) to release jobs as well. Splitting router-extended into 5 scenarios grew the total VM count — powering off VMs of passed scenarios as they finish keeps the hypervisor from running out of resources. Cap concurrent release scenarios at 25. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
/cancel |
Contributor
|
@agullon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrate 23 router test cases from the openshift-tests-private Go-based OTP suite to Robot Framework:
test/resources/router.resource): keywords for route creation, curl assertions, TLS cert generation, haproxy config inspection, syslog verification, and workload cleanup.test/suites/router/: router-config-infra, router-config-tls, router-config-policies, router-config-logging, router-routes.test/assets/router/(YAML manifests, HTTP error page fixtures).2h → 5×30min), each running one.robotsuite file directly. Presubmits narrowed torouter-basic.robotonly.create-run-shutdownto release jobs so passed scenarios' VMs are powered off as they finish, freeing hypervisor resources for the remaining scenarios. Cap concurrent release scenarios at 25.Test plan
🤖 Generated with Claude Code