test: remove environment-specific e2e defaults - #262
Merged
Merged
Conversation
WalkthroughThis PR centralizes e2e configuration in e2e/lib.sh (env defaults, skip-code, kubectl wrapper, mirror and YAML-templating helpers) and adapts GPU and NPU test cases to require their target namespaces and use those helpers to parameterize images, pull secrets, storage classes, node selectors, runtimeClassName, and resource limits. ChangesE2E Test Infrastructure Parametrization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying alauda-ai with
|
| Latest commit: |
6c3a8cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cc0dcee8.alauda-ai.pages.dev |
| Branch Preview URL: | https://codex-e2e-env-cleanup.alauda-ai.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/run_all.sh (1)
29-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale inline comment about skip return code.
The comment still says
rc=77, but skip rc is now configurable viaE2E_SKIP_RC. Please align the comment with current behavior.🤖 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 `@e2e/run_all.sh` around lines 29 - 30, Update the outdated inline comment that references a hardcoded skip return code "rc=77": change the comment to mention that the skip return code is configurable via the E2E_SKIP_RC environment variable (used by the script) and that tests may skip with the value of E2E_SKIP_RC rather than a fixed 77; locate the comment near the C12/Kueue note in run_all.sh and update its wording to reference E2E_SKIP_RC and current 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.
Inline comments:
In `@e2e/lib.sh`:
- Line 11: Validate and normalize E2E_SKIP_RC immediately after it’s set: check
that the variable E2E_SKIP_RC is a numeric string (digits only) and that its
integer value falls within 0..255; if either check fails, reset E2E_SKIP_RC to
the default 77 and export it so subsequent uses (exit and -eq comparisons) are
safe. Implement this by first using a digit-only pattern match on E2E_SKIP_RC
(e.g., case or regex) to reject non-numeric values, then perform numeric range
checks (using [ "$E2E_SKIP_RC" -lt 0 ] and [ "$E2E_SKIP_RC" -gt 255 ] guarded
behind the numeric check); if invalid, assign and export E2E_SKIP_RC=77.
---
Outside diff comments:
In `@e2e/run_all.sh`:
- Around line 29-30: Update the outdated inline comment that references a
hardcoded skip return code "rc=77": change the comment to mention that the skip
return code is configurable via the E2E_SKIP_RC environment variable (used by
the script) and that tests may skip with the value of E2E_SKIP_RC rather than a
fixed 77; locate the comment near the C12/Kueue note in run_all.sh and update
its wording to reference E2E_SKIP_RC and current behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35eb3411-22a7-49ba-9573-99056968fbff
📒 Files selected for processing (13)
e2e/cases/c11_qwen3_06b_mindspore.she2e/cases/c12_kueue_preemption.she2e/cases/c1_smoke_gpu.she2e/cases/c2_kubeflow_trainer_mnist.she2e/cases/c3_traininghub_sft.she2e/cases/c4_traininghub_osft.she2e/cases/c5_trainer_v2_llamafactory.she2e/cases/c6_volcanojob_llamafactory.she2e/cases/c7_smoke_npu.she2e/cases/c8_trainer_v2_mindspeed_npu.she2e/cases/c9_qwen3_finetune_verify.she2e/lib.she2e/run_all.sh
E2E_SKIP_RC is overridable via env in this harness, but was used both as a process exit status (truncated mod 256) and as the RHS of a numeric `-eq` compare in run_all.sh. A non-numeric or out-of-range override makes the two diverge, so a skipped case is misreported as FAIL (e.g. 300 -> exit 44, then `[ 44 -eq 300 ]` is false). Coerce any invalid override back to the 77 default right after assignment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/lib.sh (1)
140-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize
gotbefore the polling loop to avoid nounset crashes.If
timeoutis0(or negative), the loop never executes and${got}at Line 152 is unbound underset -u, causing a hard script error instead of a clean timeout return.Proposed fix
wait_for_status() { local kfn="$1" kind="$2" name="$3" ns="$4" path="$5" shift 5 local timeout="${!#}" local n=$(( $# - 1 )) local expected=( "${@:1:$n}" ) + local got="" local deadline=$(( SECONDS + timeout )) while [ "$SECONDS" -lt "$deadline" ]; do - local got got="$($kfn get "${kind}" "${name}" -n "${ns}" -o jsonpath="${path}" 2>/dev/null || true)" for e in "${expected[@]}"; do if [ "${got}" = "${e}" ]; then echo "${got}" return 0As per coding guidelines, runtime-order pitfalls and unbound-variable crash paths should be fixed before release.
🤖 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 `@e2e/lib.sh` around lines 140 - 153, The variable 'got' may be unbound if the while-loop never runs (e.g., timeout ≤ 0), causing set -u failures; initialize it before the loop by declaring local got="" (or local got='' ) right before the while that uses 'got' so the echo on timeout (the message referencing ${got}) is safe; update the existing local got declaration in the polling block to assign an empty string to avoid nounset crashes while keeping the current polling logic (references: variable 'got', 'timeout', and the while loop that checks SECONDS vs deadline).Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@e2e/lib.sh`:
- Around line 140-153: The variable 'got' may be unbound if the while-loop never
runs (e.g., timeout ≤ 0), causing set -u failures; initialize it before the loop
by declaring local got="" (or local got='' ) right before the while that uses
'got' so the echo on timeout (the message referencing ${got}) is safe; update
the existing local got declaration in the polling block to assign an empty
string to avoid nounset crashes while keeping the current polling logic
(references: variable 'got', 'timeout', and the while loop that checks SECONDS
vs deadline).
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
1 participant
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
Validation
bash -n e2e/lib.sh e2e/run_all.sh e2e/cases/*.shgit diff --checkrg -n "docker-mirrors|docker\.1ms|mlops-demo|g1-c1|npu-env|build-harbor|harbor-mlops|cephfs|152-231|my_dev_env|dev cluster|dev GPU|dev NPU|192\.168|queue: default|kubeflow-admin-cpaas-io|alauda\.cn" e2e(no matches)bash e2e/run_all.sh C1(skips cleanly withoutGPU_NAMESPACE)env NPU_NAMESPACE=test bash e2e/run_all.sh C8(skips cleanly withoutNPU_RESOURCE_NAME)yarn lintSummary by CodeRabbit
Chores
Tests