test: create a helper function to send request directly to mocker instances

[zhixiongli2011]

Overview:

Create a helper function in tests/router/test_router_e2e_with_mockers.py to send request directly to mocker instances. It will be used by the coming test functions to send dummy request to check the readiness of the mockers and to send good test request for testing.

Details:

1. Moved the existing nested function wait_for_mockers_ready() out of test_kv_push_router_bindings() function and renamed to send_request_to_specified_mocker_instance()
2. Enhanced send_request_to_specified_mocker_instance() from doing simple dummy test to be a general, shared helper function accepting router parameters for various test purposes.
3. Changed test_kv_push_router_bindings() to use the new send_request_to_specified_mocker_instance() replacing the old nested wait_for_mockers_ready().
4. Tested the changed test_kv_push_router_bindings successfully.

Where should the reviewer start?

File: tests/router/test_router_e2e_with_mockers.py
Functions: send_request_to_specified_mocker_instance() and test_kv_push_router_bindings()

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

• closes GitHub issue: [FEATURE]: more comprehensive e2e testings in per-merge CIs with mockers #2089

Summary by CodeRabbit

• Tests
  • Added a reusable asynchronous helper to target specific mocker instances with retry/backoff.
  • Updated end-to-end router tests to use the new helper for request handling.
  • Removed legacy readiness-wait logic previously embedded in tests.
  • Improves test reliability, reduces flakiness, and simplifies maintenance.

[copy-pr-bot]

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.

[github-actions]

👋 Hi zhixiongli2011! 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.

🚀

[coderabbitai]

Walkthrough

Introduces an async helper to send a request to a specified mocker via KvPushRouter with exponential backoff and optional overrides. Updates two tests to use this helper for readiness/verification. Removes the prior inline readiness-wait loop.

Changes

Cohort / File(s)	Summary
Router tests helper and usage tests/router/test_router_e2e_with_mockers.py	Added send_request_to_specified_mocker_instance with configurable params and retry/backoff; replaced inline readiness-wait logic in two tests to call the helper; removed old dummy request readiness loop.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Test
  participant Helper as Helper Function
  participant Router as KvPushRouter
  participant Worker as Mocker Instance

  Test->>Helper: send_request_to_specified_mocker_instance(opts, worker_id)
  Helper->>Helper: Build KvRouterConfig + validate worker_id
  loop up to max_retries (8)
    Helper->>Router: generate(request, config override)
    Router->>Worker: route to specified worker
    Worker-->>Router: stream chunks
    Router-->>Helper: stream chunks
    alt stream completes successfully
      Helper->>Helper: consume stream, verify success
      Helper-->>Test: return True
    else error/timeout
      Helper->>Helper: exponential backoff wait
    end
  end
  Helper-->>Test: raise RuntimeError on exhaustion

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paw: “Retry, retry!”
Backoff blooms beneath the sky.
Route the bytes to workers near—
Streams complete, the path is clear.
Tests now hop with tidy grace,
One helper sets the winning pace.
Carrots cached, all checks in place.

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly describes the primary change by indicating the addition of a helper function for sending requests directly to mocker instances and follows a clear conventional commit style, making the intent immediately obvious to readers.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	The pull request description closely follows the repository’s template by including the Overview, Details, Where should the reviewer start, and Related Issues sections with clear and specific content. It describes the purpose of the changes, outlines the precise modifications made, indicates where to inspect the updated helper and test functions, and correctly references and closes issue #2089. Each section is present and filled with actionable information, making the description comprehensive and targeted to reviewers.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

tests/router/test_router_e2e_with_mockers.py (2)

348-349: Rename unused stream item

The iterator variable isn’t used; renaming it to _ keeps the intent clear and satisfies Ruff’s B007 warning.

-            async for response in stream:
+            async for _ in stream:
                 pass

---

360-362: Preserve original exception context

Chaining the original exception retains the traceback, which makes failures easier to diagnose and addresses Ruff’s B904 warning.

-                raise RuntimeError(
-                    f"Failed to connect to mockers after {max_retries + 1} attempts: {e}"
-                )
+                raise RuntimeError(
+                    f"Failed to connect to mockers after {max_retries + 1} attempts"
+                ) from e

📜 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 9d73be1 and e90f558.

📒 Files selected for processing (1)

• tests/router/test_router_e2e_with_mockers.py (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/router/test_router_e2e_with_mockers.py (2)

lib/bindings/python/src/dynamo/_core.pyi (9)

• namespace (37-41)
• component (63-67)
• endpoint (82-86)
• KvRouterConfig (739-741)
• KvPushRouter (1126-1225)
• block_size (497-501)
• block_size (520-524)
• block_size (1117-1124)
• generate (1147-1179)

lib/bindings/python/rust/llm/kv.rs (4)

• block_size (437-439)
• block_size (498-500)
• block_size (962-964)
• generate (1077-1144)

🪛 Ruff (0.13.1)

tests/router/test_router_e2e_with_mockers.py

325-327: Avoid specifying long messages outside the exception class

(TRY003)

---

348-348: Loop control variable response not used within loop body

Rename unused response to _response

(B007)

---

352-352: Consider moving this statement to an else block

(TRY300)

---

354-354: Do not catch blind exception: Exception

(BLE001)

---

360-362: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

360-362: Avoid specifying long messages outside the exception class

(TRY003)

🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/3269/merge) by zhixiongli2011.

tests/router/test_router_e2e_with_mockers.py

[error] 1-1: Black formatting check failed. reformatted tests/router/test_router_e2e_with_mockers.py.

⏰ 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). (1)

• GitHub Check: Build and Test - dynamo

[zhixiongli2011]

@PeaBrane The pre-merg test failed at the below step in test_kv_push_router_bindings when trying to call the new helper however test_kv_push_router_bindings runs successfully on my VM. Any ideas please.

asyncio.run(send_request_to_specified_mocker_instance(mockers=mockers))

[PeaBrane]

@zhixiongli2011 thanks, can you check the CIs? I think it may just be linting. You may need to run the black linter

[zhixiongli2011]

@PeaBrane Thank you very much for the review and suggestions. I will check them out and let you know. I will use this PR to accommodate changes related to the new helper like remove overwrite, add pass in parameters (max_retries, router instance), verify token count, etc. and create another 1-2 PR to simply other test functions by using the new helper, and, of course, new PR for new tests, trying not to make big PR. Does this sound a good plan?

[PeaBrane]

@zhixiongli2011 sure that sounds good. On my end, I don't really mind the PR being too big. If you can have the current tests using your helper function it would actually include some code deletions which is always good. But it's up to you.

And yea the new tests can be in a separate PR either way. Thanks!

[zhixiongli2011]

@PeaBrane Good afternoon, all recommended changes have been applied with test_kv_push_router_bindings being fully simplified and tested successfully. Attached is the test output log file. Once this PR is reviewed and approved by you, I will open another PR to simplify test_indexers_sync

test_kv_push_router_bindings.test.log.txt

One issue encountered during the test is KvPushRouter.generate() doesn't take the 'worker_id" argument.
when using kwarg, it failed with "KvPushRouter.generate() got an unexpected keyword argument 'worker_id'"
I also tried the positional arg. It still failed with "KvPushRouter.generate() takes from 2 to 6 positional arguments but 7 were given". It seems like a python-rust binding issue. Since we don't need to pass the worker_id until the new tests, I commented it out for this PR and will try to figure it out as part of the new tests with your help.

the Rust code does have the arg.

[PeaBrane]

@zhixiongli2011 hmm that is very strange, what version/release of dynamo are you running? I think you may need to rebuild dynamo from main (with maturin) probably, instructions here. Is this an actual runtime error, or just some mypy error?

But either way, I'm fine with this for now, and will try to merge it. Thanks. (Edit: added a few more comments)

[PeaBrane]

/ok to test f703fd3

[PeaBrane]

/ok to test abfda5a