[None][test] Split verl tests into 19 fine-grained per-case wrappers#14037
[None][test] Split verl tests into 19 fine-grained per-case wrappers#14037Superjomn wants to merge 1 commit into
Conversation
Replace the 3 file-level verl wrappers (test_async_server, test_adapter, test_rollout_utils) with 19 wrappers that map 1-to-1 to upstream verl pytest cases. Each wrapper shells out to `pytest <file> -k <case>` so individual cases can be waived without skipping a whole file. Also: - Bump verl pin: 4cda6af -> d324b01 - Expand l0_verl test list to the 19 case-level entries - Remove the 3 blanket waives under nvbugs/5981833 Local smoke (2xH800 NVL dev host): - pytest --collect-only: 19 / 19 collected - 4 pure HTTP-mock adapter cases run directly in a verl clone: 4/4 PASS - All 19 case names verified unique across the 5 verl test files (no -k substring collision) The verl_setup fixture (gdrcopy/nvshmem/DeepEP build) needs root and will only run cleanly in CI containers; full validation depends on /bot run on 4xB200. Signed-off-by: Superjomn <328693+Superjomn@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR refactors the verl integration test infrastructure to enable fine-grained test waiving. Test wrapper functions now map 1-to-1 to individual pytest cases via a new ChangesVerl Test Infrastructure Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![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
🤖 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 `@tests/integration/defs/verl/test_verl_cases.py`:
- Around line 139-142: The wrapper _run_single currently uses "-k" which matches
substrings; change it to invoke pytest with the exact node id instead: build a
node id by joining the test file and case name (e.g. "{verl_file}::{case_name}"
or "{verl_file}::ClassName::test_name" when needed) and pass that as a
positional arg via _run_verl_test's extra_args instead of ["-k", case_name], so
pytest runs exactly that single test; update _run_single to construct and pass
the full node id to _run_verl_test.
🪄 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: Enterprise
Run ID: 910098c5-8e32-40a3-b5e2-611f7480c33b
📒 Files selected for processing (4)
tests/integration/defs/verl/test_verl_cases.pytests/integration/defs/verl/verl_config.ymltests/integration/test_lists/test-db/l0_verl.ymltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| def _run_single(verl_file, case_name, timeout=600): | ||
| """Run exactly one verl pytest case by name.""" | ||
| _run_verl_test(verl_file, extra_args=["-k", case_name], timeout=timeout) | ||
|
|
There was a problem hiding this comment.
Use exact pytest node IDs instead of -k for single-case wrappers.
Line 141 uses -k, which can select multiple tests via substring/expression matching. That breaks the “exactly one case” guarantee and can silently reduce waive precision as upstream adds similarly named tests.
Suggested patch (exact-case execution)
def _run_verl_test(test_path, extra_args=None, timeout=600):
"""Run a test from the verl repo via subprocess."""
- full_path = os.path.join(VERL_ROOT, test_path)
- assert os.path.exists(full_path), f"Verl test not found: {full_path}"
- cmd = [sys.executable, "-m", "pytest", full_path, "-v", "--tb=short"]
+ test_file = test_path.split("::", 1)[0]
+ full_test_file = os.path.join(VERL_ROOT, test_file)
+ assert os.path.exists(full_test_file), f"Verl test not found: {full_test_file}"
+ node_id = os.path.join(VERL_ROOT, test_path)
+ cmd = [sys.executable, "-m", "pytest", node_id, "-v", "--tb=short"]
if extra_args:
cmd.extend(extra_args)
@@
def _run_single(verl_file, case_name, timeout=600):
"""Run exactly one verl pytest case by name."""
- _run_verl_test(verl_file, extra_args=["-k", case_name], timeout=timeout)
+ _run_verl_test(f"{verl_file}::{case_name}", timeout=timeout)🤖 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 `@tests/integration/defs/verl/test_verl_cases.py` around lines 139 - 142, The
wrapper _run_single currently uses "-k" which matches substrings; change it to
invoke pytest with the exact node id instead: build a node id by joining the
test file and case name (e.g. "{verl_file}::{case_name}" or
"{verl_file}::ClassName::test_name" when needed) and pass that as a positional
arg via _run_verl_test's extra_args instead of ["-k", case_name], so pytest runs
exactly that single test; update _run_single to construct and pass the full node
id to _run_verl_test.
|
/bot --help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1" |
|
PR_Github #47921 [ run ] triggered by Bot. Commit: |
|
PR_Github #47921 [ run ] completed with state
|
2 participants
Summary
tests/integration/defs/verl/test_verl_cases.py(one wrapper per upstream verl pytest case)4cda6af->d324b01l0_verltest list to enumerate the 19 case-level entriesnvbugs/5981833waives that were skipping the old file-level wrappersWhy
File-level wrappers force us to skip an entire upstream test file when a single case is flaky. Per-case wrappers let us waive precisely.
Test plan
pytest --collect-only tests/integration/defs/verl/test_verl_cases.py-> 19 collectedd324b01(test_make_async_request_{get,post,http_error,max_attempts_exceeded}_method)-ksubstring collisions/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1"to exercise the 4xB200l0_verlpost-merge stageNotes
The
verl_setupfixture builds gdrcopy + nvshmem + DeepEP and needs root for/usr/local/lib. It runs cleanly in CI containers but cannot run on a non-root dev shell, so full validation depends on CI.Summary by CodeRabbit
Tests
Chores