add the ability to run one test - #63
Conversation
gingko --focus is a nice way to develop one test at a time or just execute one test at a time. Using an env var "ECO_TEST_FOCUS" Parameterize the timeout from a hardcoded 24h to default 24h and 30m for one test Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
📝 WalkthroughWalkthroughChangesTest runner configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
PR Summary by QodoAdd single-test focus and configurable timeout to ginkgo test runner
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
13 rules 1. Env var command injection
|
![qodo-2-for-medik8s[bot]](https://avatars.githubusercontent.com/in/4026794?s=60&v=4){kind=small}
|
|
||
| # Build ginkgo command | ||
| cmd="${GINKGO} -timeout=24h --keep-going --require-suite --randomize-all -r" | ||
| cmd="${GINKGO} -timeout=${ECO_TEST_TIMEOUT} --keep-going --require-suite --randomize-all -r" |
There was a problem hiding this comment.
1. Env var command injection 🐞 Bug ⛨ Security
scripts/test-runner.sh now interpolates user-controlled ECO_TEST_TIMEOUT (and ECO_TEST_FOCUS) into a command string that is executed with eval, allowing shell metacharacters in those env vars to alter execution (up to arbitrary command execution). This also makes focus strings containing quotes break the generated command line instead of being passed as a literal focus value.
Agent Prompt
## Issue description
`scripts/test-runner.sh` constructs a single string `cmd+=...` that includes user-controlled env vars (`ECO_TEST_TIMEOUT`, `ECO_TEST_FOCUS`) and then executes it via `eval`. This allows shell parsing of untrusted content and breaks if focus contains quotes.
## Issue Context
The PR changed timeout from hardcoded `24h` to `${ECO_TEST_TIMEOUT}` and added `--focus="${ECO_TEST_FOCUS}"`, expanding the attack surface of the existing `eval` pattern.
## Fix Focus Areas
- scripts/test-runner.sh[7-71]
## Implementation guidance
- Replace the string-based command construction with a bash array, e.g.:
- `cmd=("$GINKGO" "-timeout=$ECO_TEST_TIMEOUT" "--keep-going" "--require-suite" "--randomize-all" "-r")`
- If `ECO_TEST_LABELS` set: `cmd+=("--label-filter=$ECO_TEST_LABELS")`
- If `ECO_TEST_FOCUS` set: `cmd+=("--focus=$ECO_TEST_FOCUS")`
- Append user args safely: `cmd+=("$@")`
- Append feature dirs safely (ideally as an array too).
- Execute without `eval`: `printf '%q ' "${cmd[@]}"; echo` then `"${cmd[@]}"`.
- (Optional) Validate `ECO_TEST_TIMEOUT` format and fail fast with a clear error if invalid.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ginkgo -timeout=24h --keep-going --require-suite -r --focus="should remediate a worker node after kubelet stop" ./tests/snr-operator | ||
| ``` |
There was a problem hiding this comment.
2. Readme command output mismatch 🐞 Bug ⚙ Maintainability
The new README examples for running with ECO_TEST_FOCUS/ECO_TEST_TIMEOUT show ginkgo commands without --randomize-all, but the runner script always includes --randomize-all; users copying the displayed output won’t reproduce actual runner behavior.
Agent Prompt
## Issue description
README examples for focused runs show the emitted ginkgo command but omit `--randomize-all`, which the runner script always includes.
## Issue Context
This is introduced by the newly added example blocks under the `ECO_TEST_FOCUS`/`ECO_TEST_TIMEOUT` documentation.
## Fix Focus Areas
- README.md[119-141]
## Implementation guidance
- Update the example command lines to include `--randomize-all`, or explicitly label the command snippets as illustrative and not exact runner output.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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)
scripts/test-runner.sh (1)
48-48: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAvoid passing environment values through
eval.
ECO_TEST_TIMEOUTandECO_TEST_FOCUSare inserted intocmd, then Line 70 reparses the string. Quotes added at Line 64 do not escape shell syntax inside the value. A value containing shell metacharacters can execute additional commands. Buildcmdas a Bash array and invoke"${cmd[@]}". Convert the existing label, positional-argument, and feature-directory appends as part of the same change.Verify whether CI can supply these variables from untrusted input.
Also applies to: 61-70
🤖 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 `@scripts/test-runner.sh` at line 48, Replace the string-based command construction and eval-based execution in the test runner with a Bash array, including Ginkgo options, ECO_TEST_TIMEOUT, ECO_TEST_FOCUS, labels, positional arguments, and feature directories; invoke it with `"${cmd[@]}"` so variable contents cannot be reparsed as shell syntax. Check CI configuration for untrusted values supplied to these environment variables and preserve the existing argument 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 `@scripts/test-runner.sh`:
- Line 7: Update scripts/test-runner.sh at lines 7-7 to default ECO_TEST_TIMEOUT
to 30m when ECO_TEST_FOCUS is set and 24h otherwise, while preserving any
explicit ECO_TEST_TIMEOUT value. Update README.md lines 100-104 to document both
defaults, and lines 119-141 to show focused runs using the automatic 30m timeout
without a manual override.
---
Nitpick comments:
In `@scripts/test-runner.sh`:
- Line 48: Replace the string-based command construction and eval-based
execution in the test runner with a Bash array, including Ginkgo options,
ECO_TEST_TIMEOUT, ECO_TEST_FOCUS, labels, positional arguments, and feature
directories; invoke it with `"${cmd[@]}"` so variable contents cannot be
reparsed as shell syntax. Check CI configuration for untrusted values supplied
to these environment variables and preserve the existing argument 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d2a0c401-7325-42bf-8979-27a03311d16b
📒 Files selected for processing (2)
README.mdscripts/test-runner.sh
| GOPATH="${GOPATH:-${HOME}/go}" | ||
| PATH=$PATH:$GOPATH/bin | ||
| TEST_DIR="./tests" | ||
| ECO_TEST_TIMEOUT="${ECO_TEST_TIMEOUT:-24h}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement and document the focused-test timeout default.
The runner always selects 24h, so the documentation compensates with a manual 30m override. Select 30m automatically when ECO_TEST_FOCUS is set, then align the documentation with that contract.
scripts/test-runner.sh#L7-L7: derive the default fromECO_TEST_FOCUS, while preserving explicitECO_TEST_TIMEOUT.README.md#L100-L104: document24hfor normal runs and30mfor focused runs.README.md#L119-L141: show the focused command using the automatic30mtimeout.
📍 Affects 2 files
scripts/test-runner.sh#L7-L7(this comment)README.md#L100-L104README.md#L119-L141
🤖 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 `@scripts/test-runner.sh` at line 7, Update scripts/test-runner.sh at lines 7-7
to default ECO_TEST_TIMEOUT to 30m when ECO_TEST_FOCUS is set and 24h otherwise,
while preserving any explicit ECO_TEST_TIMEOUT value. Update README.md lines
100-104 to document both defaults, and lines 119-141 to show focused runs using
the automatic 30m timeout without a manual override.
|
I wonder if we can throw more ai at this review.. lolz |
|
@ugreener please review |
|
/pj-rehearse ack |
|
@JonahSussman: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abrugaro, eemcmullan, jmontleon, JonahSussman, slintes, ugreener, weshayutin 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 |
7 participants
gingko --focus is a nice way to develop one test
at a time or just execute one test at a time.
Using an env var "ECO_TEST_FOCUS"
Parameterize the timeout from a hardcoded 24h
to default 24h and 30m for one test
Summary by CodeRabbit
New Features
ECO_TEST_TIMEOUT.ECO_TEST_FOCUSsetting to run a specific test.Documentation