USHIFT-6640: Fix gateway API tests#6411
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request references USHIFT-6640 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 bug to target the "4.22.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. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe gateway-api Robot test suite removed the suite-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/suites/optional/gateway-api.robot (1)
81-82: Pass port explicitly to avoid hidden coupling.Line 82 hardwires
${GATEWAY_PORT}insideCreate HTTP Route, which makes the keyword less reusable and harder to maintain when ports vary.Suggested refactor
Test Simple HTTP Route @@ - Create HTTP Route ${GATEWAY_HOSTNAME} ${NS_GATEWAY} + Create HTTP Route ${GATEWAY_HOSTNAME} ${NS_GATEWAY} ${GATEWAY_PORT} Create HTTP Route @@ - [Arguments] ${hostname} ${namespace} + [Arguments] ${hostname} ${namespace} ${gateway_port} @@ - ... Verify Gateway Port Listening ${GATEWAY_PORT} + ... Verify Gateway Port Listening ${gateway_port}As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/gateway-api.robot` around lines 81 - 82, The test currently relies on a hidden global `${GATEWAY_PORT}` inside the reusable keyword Create HTTP Route; update the Create HTTP Route keyword to accept an explicit port argument (e.g., port) and use that argument instead of referencing `${GATEWAY_PORT}` internally, then call Create HTTP Route from the test passing `${GATEWAY_PORT}` explicitly and keep Verify Gateway Port Listening invoked with the same `${GATEWAY_PORT}` so the port is not coupled implicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/optional/gateway-api.robot`:
- Around line 102-107: The test "Verify Gateway Port Listening" uses `Run And
Return Rc` with `nc -z ${USHIFT_HOST} ${port}` which can hang; replace this with
`Run Process` invoking `nc -z ${USHIFT_HOST} ${port}` (or `nc -z -w <seconds>`),
and supply an explicit Robot Framework `timeout` argument so the command fails
fast on unresponsive networks; update the assertion to check the process return
code from `Run Process` instead of `Run And Return Rc`.
---
Nitpick comments:
In `@test/suites/optional/gateway-api.robot`:
- Around line 81-82: The test currently relies on a hidden global
`${GATEWAY_PORT}` inside the reusable keyword Create HTTP Route; update the
Create HTTP Route keyword to accept an explicit port argument (e.g., port) and
use that argument instead of referencing `${GATEWAY_PORT}` internally, then call
Create HTTP Route from the test passing `${GATEWAY_PORT}` explicitly and keep
Verify Gateway Port Listening invoked with the same `${GATEWAY_PORT}` so the
port is not coupled implicitly.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dafe35e2-9bed-49f4-96b1-b60cf0eebd90
📒 Files selected for processing (1)
test/suites/optional/gateway-api.robot
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test all |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/optional/gateway-api.robot`:
- Around line 115-116: The test incorrectly only checks
.status.loadBalancer.ingress[0].ip so services that provide a hostname fail;
change the kubectl/jsonpath invocation used to set ${result} (the oc get svc
call) to return either the ip or the hostname (for example by concatenating
JSONPath expressions
'{.status.loadBalancer.ingress[0].ip}{.status.loadBalancer.ingress[0].hostname}'
or by querying both and using the first non-empty), then keep the existing
Should Not Be Empty assertion on ${result} (update the failure message if
desired) so the test accepts either an external IP or a hostname for the gateway
service.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1aeb1702-71c6-4c45-b310-7ea0fed8dd87
📒 Files selected for processing (1)
test/suites/optional/gateway-api.robot
| ... oc get svc test-gateway-openshift-gateway-api -n ${namespace} -o jsonpath='{.status.loadBalancer.ingress[0].ip}' | ||
| Should Not Be Empty ${result} Gateway service does not have an external IP assigned |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "gateway-api.robot" --max-one-resultRepository: openshift/microshift
Length of output: 297
🏁 Script executed:
cat -n test/suites/optional/gateway-api.robot | sed -n '110,120p'Repository: openshift/microshift
Length of output: 696
🏁 Script executed:
# Check if the entire test file exists and its size
wc -l test/suites/optional/gateway-api.robotRepository: openshift/microshift
Length of output: 106
🏁 Script executed:
grep -n "ingress\|loadBalancer" test/suites/optional/gateway-api.robotRepository: openshift/microshift
Length of output: 191
🏁 Script executed:
# Check the surrounding context to understand the test better
cat -n test/suites/optional/gateway-api.robot | sed -n '100,125p'Repository: openshift/microshift
Length of output: 1632
Handle LoadBalancer hostname as well as IP.
This check only reads .ingress[0].ip. Some cloud providers populate .ingress[0].hostname instead, causing test failures even when the gateway is properly configured and reachable.
Suggested fix
Verify Gateway Service Has External IP
[Documentation] Verify that the gateway's LoadBalancer service has an external IP assigned
[Arguments] ${namespace}
${result} Run With Kubeconfig
- ... oc get svc test-gateway-openshift-gateway-api -n ${namespace} -o jsonpath='{.status.loadBalancer.ingress[0].ip}'
- Should Not Be Empty ${result} Gateway service does not have an external IP assigned
+ ... oc get svc test-gateway-openshift-gateway-api -n ${namespace} -o jsonpath='{.status.loadBalancer.ingress[0].ip}{.status.loadBalancer.ingress[0].hostname}'
+ Should Not Be Empty ${result} Gateway service does not have an external address assigned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/optional/gateway-api.robot` around lines 115 - 116, The test
incorrectly only checks .status.loadBalancer.ingress[0].ip so services that
provide a hostname fail; change the kubectl/jsonpath invocation used to set
${result} (the oc get svc call) to return either the ip or the hostname (for
example by concatenating JSONPath expressions
'{.status.loadBalancer.ingress[0].ip}{.status.loadBalancer.ingress[0].hostname}'
or by querying both and using the first non-empty), then keep the existing
Should Not Be Empty assertion on ${result} (update the failure message if
desired) so the test accepts either an external IP or a hostname for the gateway
service.
|
@pacevedom: 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. |
2 participants
No description provided.