Conversation
📝 WalkthroughWalkthroughThe PR introduces a "Lfast" test level and "fast mode" support for CI/CD pipelines. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/functional/L1_Functional_Tests_GPU.sh (1)
73-80: Quote command substitution to prevent word splitting.Same static analysis warning as in L0_Unit_Tests_Other.sh. Consider quoting for defensive coding.
🔧 Proposed fix
if [[ "${FAST:-0}" != "1" ]]; then for test_script in research/*/tests/functional/*.sh; do - project_dir=$(echo $test_script | cut -d/ -f1-2) + project_dir=$(echo "$test_script" | cut -d/ -f1-2) pushd $project_dir - time uv run --no-sync bash $(echo $test_script | cut -d/ -f3-) + time uv run --no-sync bash "$(echo "$test_script" | cut -d/ -f3-)" popd done fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/L1_Functional_Tests_GPU.sh` around lines 73 - 80, The shell loop is using unquoted command substitutions which can cause word-splitting; update usages of test_script and project_dir and the echo substitution so they are quoted: quote "${test_script}" when computing project_dir and when passing into pushd/popd and quote the result of the path extraction used with bash/uv run (e.g., quote the substitution that computes the relative test path). Specifically update references to FAST, test_script, project_dir, pushd, popd and the uv run invocation so all command substitutions and variables are wrapped in double quotes to prevent word splitting.tests/unit/L0_Unit_Tests_Other.sh (1)
68-76: Quote command substitution to prevent word splitting.Static analysis flagged unquoted command substitution on line 71. While the project prefers consistent formatting, this particular case could cause issues with paths containing spaces.
🔧 Proposed fix
if [[ "${FAST:-0}" != "1" ]]; then for i in research/*/tests/unit; do - project_dir=$(dirname $(dirname $i)) + project_dir=$(dirname "$(dirname "$i")") pushd $project_dir uv run --no-sync pytest tests/unit popd done fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/L0_Unit_Tests_Other.sh` around lines 68 - 76, The unquoted command substitutions in the loop can cause word-splitting for paths with spaces; change project_dir=$(dirname $(dirname $i)) to project_dir="$(dirname "$(dirname "$i")")" and also quote usages of the variable when changing directories (e.g., pushd "$project_dir" and popd as appropriate) so all command substitutions and variable expansions are safely quoted; update references in the loop handling (variable i, project_dir, and pushd) to use these quoted forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cicd-main.yml:
- Around line 328-351: The pipeline fails because the test-template action
declares azure-client-id, azure-tenant-id, and azure-subscription-id as required
but most callers (e.g., cicd-fast-functional-tests, cicd-doc-tests,
cicd-unit-tests, cicd-functional-tests) do not pass them; update
.github/actions/test-template/action.yml to mark these inputs as required: false
and give them default: '' (empty string) and ensure the action uses the
has-azure-credentials input to gate Azure usage, or alternatively make the
callers set has-azure-credentials: "false" and provide empty placeholders for
azure-client-id/azure-tenant-id/azure-subscription-id in jobs like
cicd-fast-functional-tests; adjust the action logic to only read Azure inputs
when has-azure-credentials is true.
---
Nitpick comments:
In `@tests/functional/L1_Functional_Tests_GPU.sh`:
- Around line 73-80: The shell loop is using unquoted command substitutions
which can cause word-splitting; update usages of test_script and project_dir and
the echo substitution so they are quoted: quote "${test_script}" when computing
project_dir and when passing into pushd/popd and quote the result of the path
extraction used with bash/uv run (e.g., quote the substitution that computes the
relative test path). Specifically update references to FAST, test_script,
project_dir, pushd, popd and the uv run invocation so all command substitutions
and variables are wrapped in double quotes to prevent word splitting.
In `@tests/unit/L0_Unit_Tests_Other.sh`:
- Around line 68-76: The unquoted command substitutions in the loop can cause
word-splitting for paths with spaces; change project_dir=$(dirname $(dirname
$i)) to project_dir="$(dirname "$(dirname "$i")")" and also quote usages of the
variable when changing directories (e.g., pushd "$project_dir" and popd as
appropriate) so all command substitutions and variable expansions are safely
quoted; update references in the loop handling (variable i, project_dir, and
pushd) to use these quoted forms.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/actions/test-template/action.yml.github/workflows/cicd-main.ymltests/functional/L1_Functional_Tests_GPU.shtests/unit/L0_Unit_Tests_Generation.shtests/unit/L0_Unit_Tests_Other.shtests/unit/L0_Unit_Tests_Policy.sh
fec41cb to
82ab72d
Compare
Signed-off-by: Terry Kong <terryk@nvidia.com>
The _build_container.yml reusable workflow already tags images with the branch name (:main), so the separate tag-nightly-container job was redundant. Remove it and update Lfast to reference :main. Signed-off-by: Terry Kong <terryk@nvidia.com>
Replace NRL_FORCE_REBUILD_VENVS=true with a one-time prefetch_venvs.py call and fingerprint regeneration when reusing a pre-built container. This avoids rebuilding venvs on every Ray remote call, significantly speeding up test execution. Signed-off-by: Terry Kong <terryk@nvidia.com>
The file was untracked, causing "No such file or directory" errors in L0_Unit_Tests_Other.sh when running in FAST mode. Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com> Signed-off-by: Yuki Huang <yukih@nvidia.com> Co-authored-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com> Signed-off-by: Yuki Huang <yukih@nvidia.com> Co-authored-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com> Signed-off-by: Yuki Huang <yukih@nvidia.com> Co-authored-by: Yuki Huang <yukih@nvidia.com>
What does this PR do ?
Intro a new test suite called when the
CI:Lfastlabel is applied. The high level changes are:Will still run new unit tests since it follows an exclusion pattern, but if you want a test to be run in L1 fast mode you have to label it appropriately.
We will still run nightly tests on the whole suite, but this will give us a way to get thru prs quicker.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit
Release Notes