Skip to content

Conversation

ronantakizawa
Copy link

@ronantakizawa ronantakizawa commented Oct 7, 2025

Overview:

Remove parser library tests that use mock data instead of actual engine responses from
vLLM, sglang, or trtllm engines.

Details:

This PR removes two test files from the parser library:

  1. lib/llm/tests/test_jail.rs (2,553 lines) - Contains ~40 unit tests that use manually created
    mock data via helper functions like create_mock_response_chunk().

  2. lib/llm/tests/preprocessor.rs (495 lines) - Contains tests that download HuggingFace models
    to test prompt formatting, but do not test against realistic engine streaming data.

Where should the reviewer start?

  • Verify that lib/llm/tests/test_parsers_e2e.rs still provides adequate test coverage with real
    engine data
  • Confirm no other tests depend on the deleted test files
  • Review the CI checks to ensure all tests pass after removal

Related Issues:

Summary by CodeRabbit

  • Tests
    • Removed an extensive integration test suite related to prompt formatting and model downloads to streamline the test pipeline and reduce external dependencies.
  • Chores
    • Simplified test infrastructure and cleanup of obsolete test utilities. No impact on app behavior, performance, or APIs for end-users.

@ronantakizawa ronantakizawa requested a review from a team as a code owner October 7, 2025 02:36
Copy link

copy-pr-bot bot commented Oct 7, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copy link

github-actions bot commented Oct 7, 2025

👋 Hi ronantakizawa! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions bot added fix external-contribution Pull request is from an external contributor labels Oct 7, 2025
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Walkthrough

Deletes the entire preprocessor test module under lib/llm/tests, removing Hugging Face token handling, model download helpers, ModelDeploymentCard builders, and multiple async prompt formatting tests with snapshots and tool scenarios.

Changes

Cohort / File(s) Summary of changes
Test module removal
lib/llm/tests/preprocessor.rs
Removed the full test file, including HF token utilities, model download/cache helpers, ModelDeploymentCard constructors, chat/tool test data, async tokio tests, and insta snapshot assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled the tests, hop-hop—now they’re gone,
No tokens to fetch, no snapshots at dawn.
The burrow is cleaner, the grass lying still,
Less chatter to chase down the side of the hill.
Thump-thump—onward we race, to green code we thrill!

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely summarizes the primary change of cleaning up parser library tests by removing outdated test files and uses a conventional commit prefix without extraneous detail, making the intent clear to reviewers.
Description Check ✅ Passed The pull request description fully follows the repository template by providing distinct Overview, Details, Where should the reviewer start, and Related Issues sections, each populated with clear information about the test removal and reviewer guidance, and it correctly links the resolved issue.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73b0cdb and 1c0a257.

📒 Files selected for processing (1)
  • lib/llm/tests/preprocessor.rs (0 hunks)
💤 Files with no reviewable changes (1)
  • lib/llm/tests/preprocessor.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build and Test - dynamo
  • GitHub Check: tests (.)
  • GitHub Check: clippy (.)
  • GitHub Check: clippy (lib/bindings/python)
  • GitHub Check: clippy (launch/dynamo-run)
  • GitHub Check: clippy (lib/runtime/examples)
  • GitHub Check: tests (lib/bindings/python)
  • GitHub Check: tests (launch/dynamo-run)
  • GitHub Check: tests (lib/runtime/examples)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ronantakizawa ronantakizawa force-pushed the ronantakizawa/cleanupparserlibrarytests branch from 2cb593f to 7ae3d95 Compare October 7, 2025 13:09
@ronantakizawa ronantakizawa force-pushed the ronantakizawa/cleanupparserlibrarytests branch from 7ae3d95 to a15611c Compare October 7, 2025 13:10
@rmccorm4
Copy link
Contributor

rmccorm4 commented Oct 7, 2025

Hi @ronantakizawa,

Can you elaborate on the motivation and goal of removing these files? Do you have some analysis on test/feature coverage with/without?

@ronantakizawa
Copy link
Author

ronantakizawa commented Oct 7, 2025

@rmccorm4 this was an attempt to fix issue #3399 that used unrealistic/mock data instead of actual engine responses from vLLM, sglang, or trtllm.

I removed test_jail.rs and preprocessor.rs, and fixed test_gpt_oss_e2e_with_no_tool_calls_vllm that was failing in the CI due to gpt_oss_parser creating a blocking reqwest::Client inside an async context.

Signed-off-by: ronantakizawa <[email protected]>
@ronantakizawa ronantakizawa force-pushed the ronantakizawa/cleanupparserlibrarytests branch from 6dcd48b to e6ac0cb Compare October 7, 2025 18:52
@ayushag-nv
Copy link
Contributor

@ronantakizawa Thanks for your contribution. We really don't want to delete any test files. The cleanup required is mostly on removing redundant tests but there are many unit tests that test the core functionality of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contribution Pull request is from an external contributor fix size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants