chore: Migrate smoke tests to pytest - #35
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>
…is a better test Signed-off-by: David Gardner <dagardner@nvidia.com>
…lready covers this Signed-off-by: David Gardner <dagardner@nvidia.com>
…s/e2e/test_hermes_e2e.py Signed-off-by: David Gardner <dagardner@nvidia.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThis PR converts smoke scripts into pytest tests, updates CI and documentation to use pytest invocations, adds shared fixtures for Hermes shim workspaces, adjusts several test root-path calculations, and replaces monkeypatch-based environment setup with direct ChangesSmoke-to-pytest migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
|
📖 Fern docs preview: https://nvidia-preview-pull-request-35.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.agents/skills/python-tests/SKILL.md:
- Around line 29-37: Add blank lines around the fenced Python examples in the
python-tests skill guidance so the prose does not touch the opening or closing
fences; update the SKILL.md example block near the “Avoid defensive programming
in tests” section to keep markdownlint MD031 satisfied. Ensure each fenced code
sample is separated from surrounding text by a blank line.
In `@tests/e2e/test_cli.py`:
- Around line 19-171: The test_cli function is too large and triggers PLR0915,
and a single failure masks later CLI checks. Split the current end-to-end flow
into a few smaller tests grouped by subcommand (for example
validate/inspect/plan/schema, doctor/run, and chat), and keep the shared
copytree/setup logic in a fixture or helper so test_cli is no longer one
101-statement test.
- Around line 173-215: The subprocess helper logic in the e2e tests is
duplicated across this file and other CLI test modules, so extract the shared
command-running behavior into a common utility in _utils/utils.py and reuse it
from call_json, run, run_with_stdin, and run_raw. Keep the existing
assertion/error handling behavior intact, but centralize the COMMAND/ROOT
execution wrapper so test helpers like test_cli and the matching helpers in the
other e2e files stay in sync and avoid drift.
- Line 30: The CLI test assertions are using defensive .get(..., []) on result
dicts, which hides missing-key failures and violates the test style guideline.
Update the assertions in the CLI response checks around inspected (and the other
similar result dict assertion in this test) to use direct indexing on the
expected keys instead of defaults so a malformed response fails loudly and
clearly.
- Around line 183-215: The `run()` and `run_raw()` helpers in `test_cli.py` call
`subprocess.run` without a timeout, so a hung `fabric-cli` invocation can stall
the test suite indefinitely. Update both helpers to pass a reasonable `timeout`
value through the existing `subprocess.run` calls, and keep the current
assertion/return behavior in `run()`, `run_with_stdin()`, and `run_raw()` so
failures still surface with stdout/stderr context.
In `@tests/e2e/test_hermes_cli.py`:
- Around line 54-61: The subprocess helper call_json in test_hermes_cli
currently runs fabric-cli without a bounded timeout, which can hang the e2e
suite indefinitely. Update the subprocess.run invocation in call_json to pass a
timeout value, and make it consistent with the other e2e helper in test_cli so
both helpers fail fast on stuck commands.
In `@tests/e2e/test_hermes_config_mapping.py`:
- Around line 13-24: The hermes config mapping test only covers the
relay_enabled=True path, leaving the False branch of write_hermes_config
untested. Update test_hermes_config_mapping to use pytest.mark.parametrize over
relay_enabled and assert the same saved config behavior for both values, keeping
the checks around hermes_common.write_hermes_config, config_path, and
yaml.safe_load.
- Line 23: The config read in test_hermes_config_mapping.py should explicitly
use UTF-8 to match write_hermes_config. Update the saved =
yaml.safe_load(config_path.read_text()) call to read the file with
encoding="utf-8" so the test does not depend on the platform default encoding.
In `@tests/e2e/test_local_env_e2e.py`:
- Around line 57-69: The call_json helper currently invokes subprocess.run
without any timeout, so a hung cargo run invocation can block the e2e test
forever. Update call_json to pass an explicit timeout to subprocess.run and
handle the resulting timeout case so the test fails cleanly with useful
diagnostics, keeping the existing command execution and error reporting behavior
intact.
In `@tests/e2e/test_swebench_style.py`:
- Around line 45-55: Specify UTF-8 explicitly when reading artifact contents in
the Swebench-style test so decoding is consistent across platforms. Update the
Path.read_text calls in the test that inspects patch_artifacts and
workspace_status to pass encoding="utf-8", matching the pattern used by
read_artifact in test_local_env_e2e.py and avoiding locale-dependent behavior.
- Around line 63-89: Add a timeout to both subprocess wrappers in run and
run_command so a hung cargo run or git process cannot block tests indefinitely.
Update the subprocess.run calls in these helpers to pass a reasonable timeout
value, and keep the existing failure handling in the AssertionError paths so
timeouts still surface with stdout and stderr context.
🪄 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: 5fbf6a8b-4949-4122-9481-5726ac42708a
📒 Files selected for processing (39)
.agents/skills/python-tests/SKILL.md.github/workflows/ci_python.ymlREADME.mdintegrations/harbor/README.mdpython/tests/smoke_sdk.pytests/adapters/test_adapaters_common_hermes.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_codex_cli.pytests/adapters/test_hermes_cli.pytests/adapters/test_hermes_cli_preflight.pytests/adapters/test_hermes_sdk_adapter.pytests/docs/test_python_api_docs.pytests/e2e/__init__.pytests/e2e/test_cli.pytests/e2e/test_codex_cli.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_hermes_cli.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/e2e/test_hermes_session.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/integrations/test_harbor_runner.pytests/python/test_consumer_neutral.pytests/python/test_environment_handle.pytests/python/test_harbor_integration.pytests/python/test_native_sdk.pytests/python/test_readme_examples.pytests/python/test_sdk_concurrency.pytests/python/test_sdk_contract.pytests/python/test_sdk_sessions.pytests/python/test_session.pytests/python/test_typed_config.pytests/smoke_cli.pytests/smoke_hermes_cli.pytests/smoke_hermes_sdk.pytests/smoke_local_env_e2e.pytests/smoke_relay_integration.pytests/smoke_swebench_style.py
💤 Files with no reviewable changes (7)
- tests/smoke_swebench_style.py
- tests/smoke_cli.py
- python/tests/smoke_sdk.py
- tests/smoke_hermes_cli.py
- tests/smoke_local_env_e2e.py
- tests/smoke_relay_integration.py
- tests/smoke_hermes_sdk.py
📜 Review details
⚠️ CI failures not shown inline (8)
GitHub Actions: Fern Docs / Build and publish docs: Remove tests/e2e/test_relay_integration.py as this is covered by test…
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Fern Docs / Build and publish docs: Remove tests/e2e/test_relay_integration.py as this is covered by test…
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(npx --prefix docs --no-install fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
GitHub Actions: Python / Test: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / 0_Test.txt: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Fern Docs / 0_Build and publish docs.txt: Remove tests/e2e/test_relay_integration.py as this is covered by test…
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.88s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (4)
.agents/skills/python-tests/**
⚙️ CodeRabbit configuration file
.agents/skills/python-tests/**: ---
name: python-tests
description: Python tests for Fabric; use this when writing tests
author: NVIDIA Corporation and Affiliates
license: Apache-2.0Python Test Style
- Pytest is used to run tests.
- Do not add
@pytest.mark.asyncioto any test. Async tests are automatically detected and run by the async runner; the decorator is unnecessary clutter.- Do not add a
-> Nonereturn type annotation to test functions. This is not a common convention in pytest and adds unnecessary verbosity.- When mocking a class, do not define a new class. Use
unittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.- The name of the mocked class should be prefixed with
mock, notfake.- Prefer pytest fixtures over helper methods.
- Do not repeat fixtures, if a fixture is needed in multiple test files, place it in a
conftest.pyfile.- When creating a fixture follow this pattern:
Only specify the scope argument when the value is something other than "function".`@pytest.fixture`(name="<fixture_name>"[, scope="<scope>"]) def <fixture_name>_fixture() -> <return_type>: ...- Prefer
pytest.mark.parametrizeover creating individual tests for
different input types.- If a fixture is needed for a test, but either does not return a value or the value is not used in the test, use the
@pytest.mark.usefixturesdecorator.tests/conftest.pycontains arestore_environ_fixturefixture that restores the environment variables to their original state after each test, it is defined withautouse=Trueso it is automatically applied to all tests. If you need to modify the environment variables in a test, do so usingos.environand the fixture will restore them after the test completes. There is no need to usemonkeypatch.setenvto modify environment variables in tests.Common Commands
# Focused test loop uv run pytest -k "<pattern>" # Run all te...
Files:
.agents/skills/python-tests/SKILL.md
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Fabric tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value or its value is unused, use@pytest.mark.usefixtures.
If a test modifies environment variables, useos.environ; the autouserestore_environ_fixturewill restore them after the test, so do not usemonkeypatch.setenvfor this purpose.
Avoid defensive programming in tests; let missing data raise clear failures (for example, preferresults["data"]overresults.get("data")).
Files:
tests/docs/test_python_api_docs.pytests/integrations/test_harbor_runner.pytests/adapters/test_codex_cli.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/python/test_consumer_neutral.pytests/python/test_environment_handle.pytests/e2e/test_hermes_cli.pytests/python/test_sdk_concurrency.pytests/adapters/test_hermes_sdk_adapter.pytests/python/test_readme_examples.pytests/e2e/test_cli.pytests/e2e/test_hermes_session.pytests/python/test_harbor_integration.pytests/python/test_typed_config.pytests/python/test_sdk_sessions.pytests/python/test_native_sdk.pytests/adapters/test_adapaters_common_utils.pytests/e2e/test_codex_cli.pytests/adapters/test_adapaters_common_hermes.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_harbor_swebench_task.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/docs/test_python_api_docs.pytests/integrations/test_harbor_runner.pytests/adapters/test_codex_cli.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/python/test_consumer_neutral.pytests/python/test_environment_handle.pytests/e2e/test_hermes_cli.pytests/python/test_sdk_concurrency.pytests/adapters/test_hermes_sdk_adapter.pytests/python/test_readme_examples.pytests/e2e/test_cli.pytests/e2e/test_hermes_session.pytests/python/test_harbor_integration.pytests/python/test_typed_config.pytests/python/test_sdk_sessions.pytests/python/test_native_sdk.pytests/adapters/test_adapaters_common_utils.pytests/e2e/test_codex_cli.pytests/adapters/test_adapaters_common_hermes.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_harbor_swebench_task.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.md
🧬 Code graph analysis (2)
tests/e2e/test_cli.py (3)
crates/fabric-core/src/schema.rs (1)
schema_dir(196-198)crates/fabric-core/src/runtime.rs (3)
text(48-55)adapter_id(1125-1130)adapter_kind(1150-1155)crates/fabric-core/src/doctor.rs (1)
check(456-463)
tests/e2e/test_hermes_config_mapping.py (1)
adapters/common/src/nemo_fabric_adapters/common/hermes.py (1)
write_hermes_config(106-116)
🪛 ast-grep (0.44.1)
tests/e2e/test_local_env_e2e.py
[info] 32-38: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"request_id": "local-env-e2e",
"input": "review local workspace",
"context": {"source": "local-e2e"},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 57-63: Command coming from incoming request
Context: subprocess.run(
[*COMMAND, *(str(arg) for arg in args)],
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
tests/e2e/test_swebench_style.py
[error] 78-84: Use of unsanitized data to create processes
Context: subprocess.run(
command,
cwd=cwd,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 63-69: Command coming from incoming request
Context: subprocess.run(
[*COMMAND, *(str(arg) for arg in args)],
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 78-84: Command coming from incoming request
Context: subprocess.run(
command,
cwd=cwd,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
tests/e2e/test_hermes_cli.py
[error] 54-60: Command coming from incoming request
Context: subprocess.run(
[*COMMAND, *(str(arg) for arg in args)],
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
tests/e2e/test_cli.py
[error] 183-189: Command coming from incoming request
Context: subprocess.run(
[*COMMAND, *(str(arg) for arg in args)],
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 207-214: Command coming from incoming request
Context: subprocess.run(
[*COMMAND, *(str(arg) for arg in args)],
cwd=ROOT,
input=stdin,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[info] 117-123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"request_id": "cli-structured-request",
"input": "hello structured hermes",
"context": {"task": {"source": "smoke"}},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/e2e/test_harbor_swebench_task.py
[info] 51-51: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_request(task_dir), indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 markdownlint-cli2 (0.22.1)
.agents/skills/python-tests/SKILL.md
[warning] 30-30: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 32-32: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 34-34: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Ruff (0.15.20)
tests/e2e/test_local_env_e2e.py
[error] 58-58: subprocess call: check for execution of untrusted input
(S603)
[warning] 66-68: Avoid specifying long messages outside the exception class
(TRY003)
tests/e2e/test_swebench_style.py
[error] 64-64: subprocess call: check for execution of untrusted input
(S603)
[warning] 72-74: Avoid specifying long messages outside the exception class
(TRY003)
[error] 79-79: subprocess call: check for execution of untrusted input
(S603)
[warning] 87-89: Avoid specifying long messages outside the exception class
(TRY003)
tests/e2e/test_hermes_cli.py
[error] 55-55: subprocess call: check for execution of untrusted input
(S603)
[warning] 63-65: Avoid specifying long messages outside the exception class
(TRY003)
tests/e2e/test_cli.py
[warning] 19-19: Too many statements (101 > 50)
(PLR0915)
[error] 184-184: subprocess call: check for execution of untrusted input
(S603)
[warning] 192-194: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 201-203: Avoid specifying long messages outside the exception class
(TRY003)
[error] 208-208: subprocess call: check for execution of untrusted input
(S603)
tests/python/test_sdk_sessions.py
[warning] 87-87: Unused method argument: plan_json
(ARG002)
tests/e2e/test_harbor_swebench_task.py
[warning] 38-38: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 70-70: Assertion should be broken down into multiple parts
(PT018)
🔇 Additional comments (25)
tests/python/test_consumer_neutral.py (1)
115-117: LGTM!tests/python/test_environment_handle.py (1)
8-16: LGTM!Also applies to: 28-31
tests/python/test_native_sdk.py (1)
18-19: LGTM!Also applies to: 36-39, 167-167
tests/python/test_readme_examples.py (1)
89-91: LGTM!tests/python/test_sdk_concurrency.py (1)
9-40: Clean pytest conversion usingtmp_pathfixture.Good use of the built-in
tmp_pathfixture instead of manual tempdir handling, consistent with the fixture-preference guideline.tests/python/test_sdk_sessions.py (1)
86-88: LGTM!Also applies to: 167-170, 227-231
tests/python/test_typed_config.py (1)
13-18: LGTM!Also applies to: 165-175, 184-188
tests/adapters/test_codex_cli.py (1)
21-21: ROOT depth correction looks consistent with the reorg.
parents[2]matches the file's new nesting undertests/adapters/one level deeper than before.tests/adapters/test_hermes_sdk_adapter.py (1)
8-16: LGTM! ROOT depth update andos.environ["TEST_API_KEY"]direct assignment match the reorg and the env-var handling convention. As per path instructions, "If a test modifies environment variables, useos.environ... do not usemonkeypatch.setenvfor this purpose."Also applies to: 129-129
tests/docs/test_python_api_docs.py (1)
16-16: LGTM!tests/integrations/test_harbor_runner.py (1)
13-13: LGTM!tests/adapters/test_adapaters_common_hermes.py (1)
122-125: LGTM! Directos.environset/pop replacingmonkeypatch.setenv/delenvand removal of now-unusedmonkeypatchparams matches the required convention, relying on the autouserestore_environ_fixturefor cleanup. As per path instructions, "If a test modifies environment variables, useos.environ... the autouserestore_environ_fixturewill restore them after the test."Also applies to: 195-217, 394-432, 463-469
tests/adapters/test_adapaters_common_utils.py (1)
6-6: LGTM!osimport and the switch frommonkeypatch.setenv/delenvto directos.environmutation/pop follow the repo convention; retainedmonkeypatch.setattr("sys.stdin", ...)at line 79 is unaffected since it's not an env-var call.Also applies to: 60-69, 75-80, 133-137, 154-154, 250-256
.github/workflows/ci_python.yml (1)
54-55: LGTM!Also applies to: 61-64
README.md (1)
316-317: LGTM!Also applies to: 375-375, 399-399
integrations/harbor/README.md (1)
94-94: LGTM!Also applies to: 105-113
tests/e2e/test_hermes_cli.py (1)
1-67: LGTM!tests/e2e/test_hermes_session.py (2)
31-62: Environment mutation and skip/fail gating look correct.Direct
os.environ[...]usage (PATH/PYTHONPATH) instead ofmonkeypatch.setenvis consistent with the autouserestore_environ_fixtureconvention, and the async test avoids@pytest.mark.asyncioper convention.
80-83: LGTM!Also applies to: 109-111
tests/e2e/test_harbor_swebench_task.py (2)
32-78: LGTM!
105-107: LGTM!Also applies to: 171-173
tests/python/test_harbor_integration.py (1)
128-143: Clean fixture swap.
tmp_pathcorrectly replaces the oldtempfile.TemporaryDirectoryforlogs_dir, and the async test avoids the disallowed@pytest.mark.asynciodecorator.tests/e2e/test_local_env_e2e.py (1)
17-54: LGTM!tests/e2e/test_codex_cli.py (1)
6-29: LGTM!Also applies to: 67-67
tests/e2e/test_hermes_config_mapping.py (1)
44-75: LGTM!
dagardner-nv
changed the title
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/conftest.py (1)
45-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate copytree+assert logic between
hermes_agent_dirandhermes_shim_agent_dirfixtures.Both fixtures repeat the same
shutil.copytree(..., ignore=shutil.ignore_patterns("artifacts"))+ existence-assert pattern. Consider extracting a small shared helper (e.g.,_copy_fixture_agent(src, dest)) to avoid drift as more shim-agent fixtures are added.♻️ Proposed helper extraction
+def _copy_fixture_agent(src: Path, dest: Path) -> Path: + shutil.copytree(src, dest, ignore=shutil.ignore_patterns("artifacts")) + assert dest.exists(), f"Missing fixture agent directory: {dest}" + return dest.resolve() + + def hermes_agent_dir_fixture(hermes_cli_agent_dir_src: Path, tmp_path: Path) -> Path: agent_dir = tmp_path / "hermes-cli-agent" - shutil.copytree(hermes_cli_agent_dir_src, agent_dir, ignore=shutil.ignore_patterns("artifacts")) - assert agent_dir.exists(), f"Missing fake Hermes CLI agent directory: {agent_dir}" - return agent_dir.resolve() + return _copy_fixture_agent(hermes_cli_agent_dir_src, agent_dir)🤖 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/conftest.py` around lines 45 - 71, The `hermes_agent_dir_fixture` and `hermes_shim_agent_dir_fixture` functions duplicate the same copy-and-assert workflow, so extract that shared behavior into a small helper (for example, a private helper used by both fixtures) that performs the `shutil.copytree(..., ignore=shutil.ignore_patterns("artifacts"))` and existence check. Update both fixtures to call the helper with their respective source and destination paths, keeping the fixture names and return behavior unchanged.tests/python/test_native_sdk.py (1)
23-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLarge
smokehelper flagged by Ruff (PLR0915) and guideline on preferring fixtures.The retained
smokefunction still carries the entire multi-stage workflow (~51+ statements) as a plain helper rather than being decomposed via fixtures/smaller test functions. As per coding guidelines, "Prefer pytest fixtures over helper methods." Given this is a single coherent end-to-end workflow (shared client/config state across resolve→plan→run→session), splitting it may add overhead without much test-isolation benefit, so treat as optional.🤖 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/python/test_native_sdk.py` around lines 23 - 161, The smoke helper is still a large all-in-one workflow and triggers Ruff PLR0915 while also violating the preference for fixtures over helper methods. Refactor smoke by moving shared setup into pytest fixtures and/or splitting the resolve, plan, and run/session assertions into smaller test functions so each test is shorter and easier to maintain. Keep the existing coverage for client.resolve, client.plan, client.run, and start_session, but distribute the assertions across smaller tests that reuse the same client and fixture_agent setup.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Outside diff comments:
In `@tests/conftest.py`:
- Around line 45-71: The `hermes_agent_dir_fixture` and
`hermes_shim_agent_dir_fixture` functions duplicate the same copy-and-assert
workflow, so extract that shared behavior into a small helper (for example, a
private helper used by both fixtures) that performs the `shutil.copytree(...,
ignore=shutil.ignore_patterns("artifacts"))` and existence check. Update both
fixtures to call the helper with their respective source and destination paths,
keeping the fixture names and return behavior unchanged.
In `@tests/python/test_native_sdk.py`:
- Around line 23-161: The smoke helper is still a large all-in-one workflow and
triggers Ruff PLR0915 while also violating the preference for fixtures over
helper methods. Refactor smoke by moving shared setup into pytest fixtures
and/or splitting the resolve, plan, and run/session assertions into smaller test
functions so each test is shorter and easier to maintain. Keep the existing
coverage for client.resolve, client.plan, client.run, and start_session, but
distribute the assertions across smaller tests that reuse the same client and
fixture_agent setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9fa27db5-2c56-4c71-ae4c-0f97643ce646
📒 Files selected for processing (7)
tests/conftest.pytests/e2e/test_cli.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/python/test_native_sdk.pytests/python/test_sdk_concurrency.py
📜 Review details
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.90s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
🧰 Additional context used
📓 Path-based instructions (2)
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 test functions; async tests are auto-detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with themockprefix instead offake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value or its return value is unused, use@pytest.mark.usefixtures.
If modifying environment variables in a test, useos.environ;restore_environ_fixturewill reset the environment after each test, so do not usemonkeypatch.setenvfor this purpose.
Avoid defensive programming in tests; prefer direct indexing likeresults["data"]overresults.get("data")so failures are loud and clear.
Files:
tests/python/test_sdk_concurrency.pytests/e2e/test_swebench_style.pytests/conftest.pytests/e2e/test_cli.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_local_env_e2e.pytests/python/test_native_sdk.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_sdk_concurrency.pytests/e2e/test_swebench_style.pytests/conftest.pytests/e2e/test_cli.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_local_env_e2e.pytests/python/test_native_sdk.py
🧬 Code graph analysis (1)
tests/python/test_native_sdk.py (1)
crates/fabric-core/src/lib.rs (1)
version(39-41)
🪛 ast-grep (0.44.1)
tests/e2e/test_harbor_swebench_task.py
[info] 47-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_request(task_dir), indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
tests/e2e/test_cli.py
[warning] 18-18: Too many statements (97 > 50)
(PLR0915)
tests/python/test_native_sdk.py
[warning] 23-23: Too many statements (51 > 50)
(PLR0915)
🔇 Additional comments (15)
tests/e2e/test_cli.py (4)
29-29: Defensive.get()still present on CLI result dicts.Both lines still use
.get(..., default)instead of direct indexing on keys that should always be present in a well-formed CLI response.Also applies to: 87-87
Source: Coding guidelines
18-169: Single oversized test still covers the entire CLI surface (97 statements, RuffPLR0915).One failure still hides later subcommand checks (
doctor,run,chat).Source: Linters/SAST tools
172-214: Subprocess helpers still duplicated across e2e files, and still lacktimeout=.
call_text/call_json/run/run_with_stdin/run_rawmirror the same pattern intest_hermes_cli.py/test_harbor_swebench_task.py, and none passtimeouttosubprocess.run, so a hungfabric-cliinvocation can stall CI indefinitely.
18-24: LGTM!tests/e2e/test_local_env_e2e.py (2)
52-65:call_jsonstill has nosubprocess.runtimeout.A hung
cargo runinvocation will block the test indefinitely instead of failing cleanly.
16-49: LGTM!tests/e2e/test_swebench_style.py (3)
40-40: Missing explicit UTF-8 encoding when reading artifacts.
read_text()here omitsencoding="utf-8", unlikeread_artifactin the other e2e tests; decoding depends on the platform locale.Also applies to: 50-50
53-84:run/run_commandstill lack asubprocess.runtimeout.A hung
cargo runorgitinvocation blocks the test/CI job indefinitely.
16-39: LGTM!tests/e2e/test_harbor_swebench_task.py (2)
41-49: LGTM!
73-75: 🩺 Stability & AvailabilityCheck
verify_with_harbor_taskcreates the log directory
hermes_shim_agent_dirno longer copiesartifacts, sohermes_shim_agent_dir / "artifacts" / "verifier"must be created before any log writes; otherwiseVERIFY_ENV=1will fail when that path is missing.tests/python/test_native_sdk.py (2)
16-24: LGTM!Also applies to: 33-36, 161-167
16-24: 🩺 Stability & AvailabilityConfirm
hermes_shim_agent_dirisolation.client.runandclient.start_sessionwrite artifacts, so this needs a fresh per-test copy or function-scoped fixture; a shared path can leak state across runs.tests/python/test_sdk_concurrency.py (2)
30-36: LGTM!
23-28: 🩺 Stability & AvailabilityEnsure
run_copyuses distinct destinations per agent. Both concurrent calls sharetmp_path, so the copy step must not resolve to the same path.
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e/test_swebench_style.py (1)
57-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
run()now resolves prior timeout comment — but siblingrun_command()may still be exposed.Delegating
run()torun_fabric_clifixes half of the earlier "Add timeouts torunandrun_command" feedback.run_command()(used forgit/dockerinvocations in this same test) wrapssubprocess.rundirectly and isn't covered by this refactor, so it may still lack a bounded timeout and can hang the test/CI job on a stuckgit/dockercall.#!/bin/bash rg -n -A10 'def run_command' tests/e2e/test_swebench_style.py🤖 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/e2e/test_swebench_style.py` around lines 57 - 63, `run()` now delegates through `run_fabric_cli`, but the sibling `run_command()` helper still calls `subprocess.run` directly and can hang on stuck git/docker operations. Update `run_command()` to use the same bounded timeout behavior as `run()` by routing it through the shared Fabric CLI wrapper or otherwise adding an explicit timeout in that helper, keeping the fix centered on `run_command` and its direct subprocess invocation.
🤖 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.
Duplicate comments:
In `@tests/e2e/test_swebench_style.py`:
- Around line 57-63: `run()` now delegates through `run_fabric_cli`, but the
sibling `run_command()` helper still calls `subprocess.run` directly and can
hang on stuck git/docker operations. Update `run_command()` to use the same
bounded timeout behavior as `run()` by routing it through the shared Fabric CLI
wrapper or otherwise adding an explicit timeout in that helper, keeping the fix
centered on `run_command` and its direct subprocess invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2c637271-ee4a-4d94-8a8c-73bf9a1ac20c
📒 Files selected for processing (7)
tests/_utils/utils.pytests/conftest.pytests/e2e/test_cli.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_hermes_cli.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build and publish docs
- GitHub Check: Test
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 6.08s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run and write tests in the Python test suite.
Do not add@pytest.mark.asyncioto test functions; async tests are automatically handled by the async runner.
Do not annotate test functions with-> None; omit return type annotations on pytest test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock(withspecwhen needed) instead of defining a new class.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in tests, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixture, somonkeypatch.setenvis unnecessary.
Avoid defensive programming in tests; access expected data directly (for example,results["data"]instead ofresults.get("data")) so failures are loud and clear.
Files:
tests/e2e/test_hermes_cli.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/_utils/utils.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_cli.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/e2e/test_hermes_cli.pytests/e2e/test_local_env_e2e.pytests/e2e/test_swebench_style.pytests/_utils/utils.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/e2e/test_cli.py
🪛 ast-grep (0.44.1)
tests/_utils/utils.py
[error] 18-26: Command coming from incoming request
Context: subprocess.run(
[*FABRIC_COMMAND, *(str(arg) for arg in args)],
cwd=REPO_ROOT,
input=stdin,
text=True,
capture_output=True,
check=False,
timeout=timeout,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
tests/_utils/utils.py
[error] 19-19: subprocess call: check for execution of untrusted input
(S603)
tests/e2e/test_harbor_swebench_task.py
[warning] 178-180: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
tests/_utils/utils.py (2)
30-35: LGTM!
9-27: 🩺 Stability & Availability
run_fabric_clitimeout may be too short for the first coldcargo run
This shared helper puts a fixed 60s cap on every CLI invocation, including the first one in a fresh test process. There’s no session-scoped prebuild here, so a slow CI runner can still hitTimeoutExpiredbefore the cache is warm. Consider prebuildingfabric-clionce or making the default timeout configurable.tests/conftest.py (2)
61-85: LGTM! Extracting_copy_agent_dirremoves the duplicatedcopytree/ignore-pattern logic betweenhermes_shim_agent_dir_fixtureandcode_review_agent_dir_fixture.
73-76: 🎯 Functional CorrectnessNo action needed:
hermes_shim_agent_dir_srcalready asserts the shim source directory exists beforehermes_shim_agent_dir_fixturecopies it.> Likely an incorrect or invalid review comment.tests/e2e/test_cli.py (1)
179-185: LGTM! Delegating torun_fabric_cliresolves the earlier "missing timeout" and "duplicated subprocess helper" feedback for this file.Also applies to: 197-198
tests/e2e/test_hermes_cli.py (1)
50-56: LGTM! Resolves the earlier missing-timeout comment oncall_json.tests/e2e/test_local_env_e2e.py (1)
50-56: LGTM! Resolves the earlier missing-timeout comment oncall_json.tests/e2e/test_harbor_swebench_task.py (2)
20-20: LGTM!
175-181: 🗄️ Data Integrity & IntegrationShared CLI wrapper is fine here.
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
left a comment, looks good otherwise. I think AGENTS.md also has some stale references to some of the remove smoke tests, might be worth it to update that too?
Signed-off-by: David Gardner <dagardner@nvidia.com>
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 `@README.md`:
- Around line 391-413: The fenced command blocks in the README section are
missing the required surrounding blank lines, triggering MD031. Update the
Markdown around the command examples so each fenced block is separated from
adjacent prose by a blank line, keeping the content under the relevant command
examples intact. Locate the affected section by the visible command examples
like the build-all, test-all, test-rust, and test-python snippets.
🪄 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: ba759192-7269-4537-888a-9c4347a77a33
📒 Files selected for processing (1)
README.md
📜 Review details
⚠️ CI failures not shown inline (8)
GitHub Actions: Fern Docs / Build and publish docs: Don't advertize smoke tests as workflows, document developer setup
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Fern Docs / 0_Build and publish docs.txt: Don't advertize smoke tests as workflows, document developer setup
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Fern Docs / Build and publish docs: Don't advertize smoke tests as workflows, document developer setup
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(npx --prefix docs --no-install fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
GitHub Actions: Python / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / 0_Test.txt: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: chore: Migrate smoke tests to pytest
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.86s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (1)
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.md
🪛 LanguageTool
README.md
[grammar] ~391-~391: Ensure spelling is correct
Context: ...uv` flag to avoid building reinstalling depdnendencies in the virtual environment. ```bash jus...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 392-392: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 397-397: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 402-402: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 407-407: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 412-412: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 414-414: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (1)
README.md (1)
385-389: 🎯 Functional Correctness | ⚡ Quick winFix the bootstrap command typo.
The trailing
'after3.12makes this shell example invalid as written, so copy/paste will fail. Please verify the example against the currentuvCLI. As per path instructions,README.mddocs must be technically accurate against the current API, command correctness, and consistency with generated schemas.Suggested fix
-uv venv --seed .venv --python 3.12' +uv venv --seed .venv --python 3.12Source: Path instructions
Signed-off-by: David Gardner <dagardner@nvidia.com>
2 participants
Overview
Migrate smoke tests to pytest, and categorize existing tests
Details
smoke_*.pytests have been renamed totest_*.pyand refactored intopytesttestspython/testshave been moved totests/python, this was needed in order to make use of fixtures defined intests/conftest.pyWhere should the reviewer start?
tests/Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests