bug: fix test_installed_adapter_discovery.py to run within a uv environment - #127
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>
WalkthroughCI selects the ChangesPython environment compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/python/test_installed_adapter_discovery.py`:
- Around line 97-101: Update the _create_venv helper signature to explicitly
annotate that it returns None, leaving its virtual-environment creation behavior
unchanged.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: c567e61a-54ad-4316-9c18-d13fc81ff44b
📒 Files selected for processing (2)
.github/workflows/ci_python.ymltests/python/test_installed_adapter_discovery.py
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (17)
**/.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-ci/SKILL.md)
**/.github/workflows/*.{yml,yaml}: Definepermissions:per job that needs token access, using the minimum required scopes; avoid workflow-level permissions unless centralized inheritance is intentionally documented.
Pin every third-party GitHub Action to a full commit SHA and retain a readable version comment after the SHA.
Use action-native or ecosystem-native caching instead of genericactions/cachewhen available.
Tie cache invalidation to lockfiles, dependency manifests, or explicit tool versions; avoid caching generated outputs unless deliberate repository behavior requires it.
Useastral-sh/setup-uvcache support withcache-dependency-globanchored touv.lock.
UseSwatinem/rust-cachewith explicitshared-keyandworkspacesinstead of ad hoc target-directory caching.
Keep deploy and publish permissions isolated to only the jobs that require them; restrictpages: writeandid-token: writeto Pages deployment jobs and their reusable-workflow callers.
Usecontents: readas the minimum permission for checkout-based build, test, documentation, and packaging jobs.
Grantpull-requests: readonly to jobs that perform pull-request metadata lookups.
When usingworkflow_call, inspect both caller and callee, and ensure the caller grants every permission required by the called jobs because the callee cannot elevate permissions.
Keep local workflow commands aligned with equivalentjustfilerecipes, and keep Python, Rust, and documentation jobs aligned with their lockfiles.
Preserve intended concurrency settings, branch filters, and documentation publish guards when modifying workflows.
Before editing workflows, inspect workflow relationships and settings with searches coveringuses:,permissions:,secrets:,concurrency:,cache, andjust.
Files:
.github/workflows/ci_python.yml
{docs/package.json,docs/package-lock.json,.github/workflows/*.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Keep documentation tooling, CI workflows, install commands, build/test/clean recipes, and examples consistent with current package names and commands.
Files:
.github/workflows/ci_python.yml
{pyproject.toml,python/pyproject.toml,Cargo.toml,docs/package.json,.github/workflows/*.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Reflect public packaging changes in release-facing documentation and ensure CI references the same package names and commands used by local workflows.
Files:
.github/workflows/ci_python.yml
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (.agents/skills/README.md)
GitHub Actions workflows must use pinned actions and be validated locally when maintained.
Files:
.github/workflows/ci_python.yml
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/python/test_installed_adapter_discovery.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
**/*.{rs,py}: When changing the Rust core or public schemas, run both the Rust and Python test suites.
When adding functionality, include tests in the corresponding Rust crate or the relevant area undertests/.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.
Files:
tests/python/test_installed_adapter_discovery.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected and run by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; place fixtures needed by multiple test files inconftest.py.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a function named<fixture_name>_fixture; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its return value is unused or it does not return a value.
Use the autouserestore_environ_fixturefromtests/conftest.pyto restore environment variables; modify variables withos.environand do not usemonkeypatch.setenv.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{rs,py,html,md,mdx,toml,yml,yaml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Rust, Python, HTML, Markdown, MDX, TOML, YAML, and shell source files must include the project SPDX copyright and Apache-2.0 license headers using the comment syntax appropriate to each file type.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use type annotations for public Python APIs.
Files:
tests/python/test_installed_adapter_discovery.py
**/*.{py,rs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,rs}: Keep native Python binding declarations synchronized with their Rust implementations.
Usesnake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
tests/python/test_installed_adapter_discovery.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_installed_adapter_discovery.py
🪛 Ruff (0.15.21)
tests/python/test_installed_adapter_discovery.py
[warning] 97-97: Missing return type annotation for private function _create_venv
Add return type annotation: None
(ANN202)
🪛 zizmor (1.26.1)
.github/workflows/ci_python.yml
[warning] 91-91: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 91-91: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (4)
.github/workflows/ci_python.yml (1)
91-91: LGTM!tests/python/test_installed_adapter_discovery.py (3)
104-111: LGTM!
174-174: LGTM!
247-247: LGTM!
|
/merge |
Overview
hermes-agentextra is installed for CI testsWhere should the reviewer start?
tests/python/test_installed_adapter_discovery.pyRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-140
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
Tests
Chores