Add E2E tests for hermes_cli adapter - #18
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Python test skill doc, new FastAPI/Uvicorn test dependencies, a mock chat-completions server utility, shared pytest fixtures, and a Hermes CLI relay end-to-end test suite that rewrites relay config and validates generated artifacts. ChangesHermes CLI relay test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
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
🧹 Nitpick comments (3)
tests/test_hermes_cli.py (2)
189-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded record/step counts are brittle.
len(atof_records) == 6andlen(steps) == 5will break on any benign change to the relay's emitted telemetry volume, with a failure that points here rather than at the cause. Consider asserting>=a minimum, or anchoring on the named boundary records (which you already check) instead of exact totals.Also applies to: 218-218
🤖 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/test_hermes_cli.py` at line 189, The assertions in the Hermes CLI tests are too brittle because they hardcode exact telemetry record and step counts. Update the checks around the test cases using atof_records and steps to avoid fixed totals, either by asserting a minimum threshold or by relying on the already-validated boundary records in the relevant test functions. Keep the named boundary assertions intact and adjust the total-count expectations so they remain stable as telemetry volume changes.
74-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffAutouse fixture re-runs the relay once per test method.
pytest instantiates the class per test method, so this function-scoped autouse fixture executes the full
FabricClient().run(...)relay invocation for each oftest_artifacts,test_atof_artifacts, andtest_atif_artifacts— three end-to-end runs to assert on one set of outputs. If the run is deterministic, computing it once and sharing the result would cut E2E time significantly.One option: move the run into a class- or module-scoped fixture that returns the result dict, and have each test read from it instead of
self.🤖 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/test_hermes_cli.py` around lines 74 - 98, The autouse fixture run_hermes_cli_relay is doing the full FabricClient.run relay invocation separately for every test method, which repeats the same expensive end-to-end setup. Refactor this so the execution happens once in a wider-scoped fixture (class- or module-scoped) that returns the result/artifact data, then have test_artifacts, test_atof_artifacts, and test_atif_artifacts consume that shared fixture instead of relying on self state.pyproject.toml (1)
26-31: 📐 Maintainability & Code Quality | 🔵 TrivialConsider tightening the FastAPI/Uvicorn pins.
~=0.138and~=0.49still admit later0.xminors (~=0.138allows0.139.*,~=0.49allows0.50.*). Use~=0.138.0and~=0.49.0if patch-line reproducibility matters.Suggested pin
- "fastapi~=0.138", + "fastapi~=0.138.0", "pytest>=8", "pytest-asyncio>=0.26", "pytest-cov~=7.0", "pyyaml>=6.0", - "uvicorn~=0.49", + "uvicorn~=0.49.0",🤖 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 `@pyproject.toml` around lines 26 - 31, The dependency pins in pyproject.toml are too loose for FastAPI and Uvicorn because `~=0.138` and `~=0.49` still allow later minor releases. Tighten the version specifiers in the dependency list to the patch-level form for `fastapi` and `uvicorn` so only patch updates within the same minor line are accepted.
🤖 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/conftest.py`:
- Around line 122-126: The mock_nvidia_api_key fixture is mutating os.environ
directly, which leaves NVIDIA_API_KEY set after the fixture finishes. Update
mock_nvidia_api_key_fixture to use monkeypatch.setenv so the temporary value is
scoped to the test and automatically restored at teardown; keep the fixture name
and return value the same so existing tests continue to use it.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 26-31: The dependency pins in pyproject.toml are too loose for
FastAPI and Uvicorn because `~=0.138` and `~=0.49` still allow later minor
releases. Tighten the version specifiers in the dependency list to the
patch-level form for `fastapi` and `uvicorn` so only patch updates within the
same minor line are accepted.
In `@tests/test_hermes_cli.py`:
- Line 189: The assertions in the Hermes CLI tests are too brittle because they
hardcode exact telemetry record and step counts. Update the checks around the
test cases using atof_records and steps to avoid fixed totals, either by
asserting a minimum threshold or by relying on the already-validated boundary
records in the relevant test functions. Keep the named boundary assertions
intact and adjust the total-count expectations so they remain stable as
telemetry volume changes.
- Around line 74-98: The autouse fixture run_hermes_cli_relay is doing the full
FabricClient.run relay invocation separately for every test method, which
repeats the same expensive end-to-end setup. Refactor this so the execution
happens once in a wider-scoped fixture (class- or module-scoped) that returns
the result/artifact data, then have test_artifacts, test_atof_artifacts, and
test_atif_artifacts consume that shared fixture instead of relying on self
state.
🪄 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: 13e76d1d-648f-4cc3-9924-34552354bff2
📒 Files selected for processing (6)
.agents/skills/python-tests/SKILL.mdpyproject.tomltests/_utils/mock_api_server.pytests/_utils/utils.pytests/conftest.pytests/test_hermes_cli.py
Signed-off-by: David Gardner <dagardner@nvidia.com>
2 participants
tests/_utils/mock_api_server.pyapi_serverfixture intests/conftest.pyfastapianduvicornto thetestdependency groupTestHermesE2Etest class intests/test_hermes_cli.pycontaining the following related tests:Summary by CodeRabbit
New Features
Tests
Documentation
Chores