docs: Add README.md guide with examples for doc test CI test scripts. #458
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@docs/add-testing-guideRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@docs/add-testing-guide |
ganeshku1
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ganeshku1
force-pushed
the
docs/add-testing-guide
branch
from
6379ed5 to
e34860f
Compare
WalkthroughA new README was added at tests/ci/test_docs_end_to_end/README.md documenting how documentation example end-to-end tests are discovered, the required tagged blocks per server (setup, health-check, aiperf-run), execution flow (build → setup → health check → test → cleanup), validation rules, running modes, and examples. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/ci/test_docs_end_to_end/README.md (3)
8-15: Potential Unicode character issue on line 14 — clarify the escape removal instruction.Line 14 appears to contain a zero-width space or similar invisible character that may confuse readers trying to copy the example. Replace the inline code example with a clearer textual explanation.
Instead of:
you should writebash ` (without the backslash)` Use: `remove the backslash so it becomes: three backticks (`) followed bybash`This makes the instruction explicit and avoids confusion from hidden Unicode characters.
Apply this diff to clarify the instruction:
-For example, this file shows examples like `\```bash` but you should write ````bash` (without the backslash). +For example, this file shows examples like `\```bash` but you should write ` ```bash ` (without the backslash).
195-201: Clarify the default server selection behavior.Line 197 states the framework "tests the first discovered server by default," but the phrasing is ambiguous — it's unclear whether this is documented design behavior or a known limitation to be improved.
If this is a limitation, clarify that the
--all-serversflag is required to test multiple servers. If it's intended design, the phrasing could be: "By design, the framework tests the first discovered server. Use--all-serversto test all discovered servers."Verify with the framework maintainers whether testing only the first server by default is:
- Intended behavior (document as design choice)
- A known limitation (suggest future improvement)
Apply this diff to clarify the behavior:
-Currently, the framework tests the first discovered server by default. Use `--all-servers` to test all: +By default, the framework tests the first discovered server. Use `--all-servers` to test all discovered servers:
1-314: Comprehensive documentation with excellent structure and coverage.This README effectively documents the end-to-end testing framework with clear examples, actionable troubleshooting, and reference information. The progressive structure (overview → discovery → step-by-step → examples → running → validation → execution flow → patterns → troubleshooting → best practices → reference) is ideal for both newcomers and maintainers.
Strengths:
- Prominent warning about escaped code blocks (lines 8-15) prevents common mistakes
- Examples are self-contained and demonstrable (lines 49-58, 70-76, 88-101)
- Validation rules are explicit, helping catch configuration errors early (lines 203-210)
- Troubleshooting section addresses realistic failure modes (lines 255-283)
- Best practices balance speed with correctness (lines 285-293)
Minor issues flagged separately above (Unicode character on line 14, ambiguous phrasing on line 197).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/ci/test_docs_end_to_end/README.md(1 hunks)
⏰ 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: integration-tests (ubuntu-latest, 3.11)
- GitHub Check: integration-tests (macos-latest, 3.13)
- GitHub Check: integration-tests (ubuntu-latest, 3.12)
- GitHub Check: integration-tests (ubuntu-latest, 3.13)
- GitHub Check: integration-tests (macos-latest, 3.11)
- GitHub Check: integration-tests (macos-latest, 3.10)
- GitHub Check: integration-tests (ubuntu-latest, 3.10)
- GitHub Check: integration-tests (macos-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.13)
| **The bash code examples in this documentation use backslashes (`\`) before the triple backticks** to prevent them from being parsed as actual test commands by the test framework. | ||
|
|
||
| **When copying examples from this file, you MUST remove the backslashes (`\`) before using them.** | ||
|
|
||
| For example, this file shows examples like `\```bash` but you should write ````bash` (without the backslash). |
There was a problem hiding this comment.
Can you just have the test script ignore this file? I think that would be a much cleaner way of doing it.
There was a problem hiding this comment.
I thought about that option, one drawback is user would not be able to copy paste the sample from the preview they, but they can do that from the raw file.
I will make that change and update this patch set.
There was a problem hiding this comment.
which ever one makes the most sense for the end user experience. is fine.
| - Do NOT include `--ui-type` flag - the test framework adds `--ui-type simple` automatically | ||
| - Each command is executed inside the AIPerf Docker container | ||
| - Commands should complete in a reasonable time (default timeout: 300 seconds) | ||
| - Use small values for `--num-prompts` and `--max-tokens` to keep tests fast |
There was a problem hiding this comment.
Can you add a note that the server is not taken down in between aiperf profile commands?
There was a problem hiding this comment.
Yes that is a good note, i missed that, will add that to the readme.
|
LGTM |
Include warnings about escaped code blocks to prevent confusion for user copy pasting the sample code blocks. Signed-off-by: Ganesh Kudleppanavar <[email protected]>
- Rename README.md to DOC_CI_TEST_README.md to clarify it documents the CI test framework - Update parser.py to skip DOC_CI_TEST_README.md when parsing markdown files - Remove warning about escaped backticks (no longer needed since file is skipped) - Add notes that server is NOT restarted between multiple aiperf commands Addresses PR feedback to have test script ignore this documentation file instead of using escaped backticks in examples. Signed-off-by: Ganesh Kudleppanavar <[email protected]>
ganeshku1
force-pushed
the
docs/add-testing-guide
branch
from
b5a0725 to
21752a0
Compare
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@21752a0c80100f1ef414dfa296f45e7aad214ebcRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@21752a0c80100f1ef414dfa296f45e7aad214ebcLast updated for commit: |
The changes in the PR include README for the doc test CI.
The README includes:
\```bash) so users don't get confused when copying examplessetup-,health-check-, andaiperf-run-followed by bash code blocks)Summary by CodeRabbit