test: allow zero coverage in resource sampling checks by mnajafian-nv · Pull Request #483 · NVIDIA/NeMo-Fabric

mnajafian-nv · 2026-10-24T00:01:25Z

Overview

Resource sampling can collect valid memory values without covering any elapsed time. Coverage counts only intervals between adjacent valid readings that are no longer than twice the requested sampling interval.

The SDK and Rust integration tests require positive coverage. The SDK assertion has failed on macOS with a valid covered_fraction of 0.0. Accept the full [0, 1] range in both checks so a valid partial measurement does not fail these tests.

Details

Add a short comment at each assertion explaining why zero is valid. The Rust test still requires the sampled memory peak to exceed the final reading by more than 128 MiB. Both tests retain their sample-count, artifact, identity and final-timestamp checks. The SDK test also retains its typed-result and serialization checks.

The deterministic gauge tests continue to require full positive coverage when readings qualify and exclude missing readings or long gaps.

Validation

The TypeScript adapter checks remain blocked by the shared dependency audit tracked in #484.

The SDK test module and the focused Rust runtime and sampling tests pass. Formatting passes.

python -m pytest -p no:cacheprovider tests/python/test_system_stats.py -q
cargo test -p nemo-fabric-core --locked sampled_stats_capture_transient_worker_memory_and_preserve_correlation
cargo test -p nemo-fabric-core --locked system_stats::sampling::tests
cargo fmt --all -- --check

The previous SDK assertion failed in the Python 3.11 macOS job and the Python 3.14 macOS job. Full-suite result pending.

Where should the reviewer start?

Start with the coverage assertions in tests/python/test_system_stats.py and sampled_stats_capture_transient_worker_memory_and_preserve_correlation in crates/fabric-core/src/runtime.rs. Compare them with Gauge::push, Gauge::finish and the deterministic tests in crates/fabric-core/src/system_stats/sampling.rs.

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

Relates to: none
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
- Updated sampled-statistics checks to accept coverage values from 0 through 1, including samples with no nearby readings contributing to coverage.

Sparse valid readings can produce zero covered time. Accept the full coverage range while preserving spike, artifact and result checks. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>

coderabbitai · 2026-10-24T00:01:56Z

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto
Repository: NVIDIA/NeMo-Fabric
Branch: main
File: .coderabbit.yaml
Commit: fd3963d
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a4708711-cd1c-40eb-b2d2-12c531e64ce2

📥 Commits

Reviewing files that changed from the base of the PR and between fd3963d and 56e59c7.

📒 Files selected for processing: 2

crates/fabric-core/src/runtime.rs
tests/python/test_system_stats.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout: 27

GitHub Check: Preview docs
GitHub Check: Test (Python 3.12, windows-amd64)
GitHub Check: Test (Python 3.13, macos-arm64)
GitHub Check: Test (Python 3.12, linux-arm64)
GitHub Check: Test adapters (Node 24)
GitHub Check: Test (Python 3.11, linux-arm64)
GitHub Check: Test (Python 3.12, linux-amd64)
GitHub Check: Test (Python 3.11, linux-amd64)
GitHub Check: Test (Python 3.13, linux-arm64)
GitHub Check: Test (Python 3.14, linux-amd64)
GitHub Check: Test (Python 3.12, macos-arm64)
GitHub Check: Test adapters (Node 22.19.0)
GitHub Check: Test (Python 3.11, macos-arm64)
GitHub Check: Qwen Code E2E
GitHub Check: Test (Python 3.14, windows-amd64)
GitHub Check: Test (arm64)
GitHub Check: Test (Python 3.11, windows-amd64)
GitHub Check: Test (Python 3.14, linux-arm64)
GitHub Check: Test (Python 3.14, macos-arm64)
GitHub Check: Test (Python 3.13, windows-amd64)
GitHub Check: Cline E2E
GitHub Check: OpenCode E2E
GitHub Check: Hermes adapter (upstream Relay 0.10, Python 3.14)
GitHub Check: Test (Python 3.13, linux-amd64)
GitHub Check: Pre-commit
GitHub Check: Relay 0.9 (Python 3.12, linux-amd64)
GitHub Check: Test (x86_64)

⚠️ CI failures not shown inline: 10

GitHub Actions: TypeScript / 0_Test (Node 20.18.3).txt: test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

GitHub Actions: TypeScript / Test (Node 20.18.3): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

GitHub Actions: TypeScript / 1_Test adapters (Node 24).txt: test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m
...
 ##[group]Run just test-typescript-adapters
...
     at async node:internal/modules/esm/loader:647:26
     at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5)
 Node.js v24.21.0
 error: Recipe `test-typescript-adapters` failed on line 579 with exit code 1
 ##[error]Process completed with exit code 1.

GitHub Actions: TypeScript / Test adapters (Node 24): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

GitHub Actions: TypeScript / Test adapters (Node 24): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

ential (3.245034ms)
 ✔ rejects append system instructions before loading the OpenCode SDK (0.373167ms)
 ✔ passes replace system instructions through OpenCode's build agent (2.779925ms)
 ✔ encodes system instructions literally before OpenCode parses configuration variables (1.797839ms)
 ✔ configures validated Fabric skill directories for OpenCode (109.348977ms)
 ✔ aborts a stalled skill discovery request at the discovery deadline (9.426767ms)
 ✔ rejects a missing Fabric skill directory before loading the OpenCode SDK (1.492209ms)
 ✔ rejects a configured skill that OpenCode did not load (5026.651482ms)
 ✔ rejects additional OpenCode-discoverable files in a configured skill directory (9.398374ms)
 ✔ isolates OpenCode discovery from skills added to the source after startup begins (57.68663ms)
 ✔ allows OpenCode built-in skills outside configured Fabric skill directories (5.519854ms)
 ✔ rejects configured skills with duplicate loaded names (6.703526ms)
 ✔ resolves configured skill paths literally before staging them for OpenCode (5.779799ms)
 ✔ configures Fabric stdio and streamable-HTTP MCP servers for OpenCode (1.755761ms)
 ✔ encodes every Fabric-supplied MCP value literally before OpenCode parses configuration variables (1.159196ms)
 ✔ resolves MCP header references from the parent environment without exposing them to OpenCode (1.028372ms)
 ✔ prefers explicitly configured values over parent environment values in MCP headers (1.004267ms)
 ✔ rejects a missing MCP header reference before loading the OpenCode SDK (0.236843ms)
 ✔ does not resolve inherited names in MCP header references (0.178623ms)
 ✔ rejects invalid expanded MCP HTTP headers before loading the SDK (0.501937ms)
 ✔ rejects command arguments on streamable-HTTP MCP servers before loading the SDK (0.206145ms)
 ✔ rejects environment variables on streamable-HTTP MCP servers before loading the SDK (0.183944ms)
 ✔ rejects non-loopback HTTP MCP servers before expanding headers or loading the SDK (0.225641ms)
 ✔...

GitHub Actions: TypeScript / 2_Test adapters (Node 22.19.0).txt: test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m
...
 ##[group]Run just test-typescript-adapters
...
     at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:651:26)
     at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)
 Node.js v22.19.0
 error: Recipe `test-typescript-adapters` failed on line 579 with exit code 1
 ##[error]Process completed with exit code 1.

GitHub Actions: TypeScript / Test adapters (Node 22.19.0): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

GitHub Actions: TypeScript / Test adapters (Node 22.19.0): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

red root
   ---
   duration_ms: 6.137671
   type: 'test'
   ...
 # Subtest: rejects non-text input and normalizes terminal OpenCode failures
 ok 87 - rejects non-text input and normalizes terminal OpenCode failures
   ---
   duration_ms: 1.084531
   type: 'test'
   ...
 # Subtest: invalidates a runtime after an OpenCode session transport failure
 ok 88 - invalidates a runtime after an OpenCode session transport failure
   ---
   duration_ms: 0.938167
   type: 'test'
   ...
 1..88
 # tests 88
 # suites 0
 # pass 88
 # fail 0
 # cancelled 0
 # skipped 0
 # todo 0
 # duration_ms 18980.060574
 > nemo-fabric-adapters-qwen@0.6.0 test
 > npm run clean && npm run build && node --test test/*.test.mjs
 > nemo-fabric-adapters-qwen@0.6.0 clean
 > rm -rf dist
 > nemo-fabric-adapters-qwen@0.6.0 build
 > tsc -p tsconfig.build.json
 TAP version 13
 # Subtest: descriptor claims only implemented normalized fields
 ok 1 - descriptor claims only implemented normalized fields
   ---
   duration_ms: 1.368126
   type: 'test'
   ...
 # Subtest: selects and validates an OpenAI-compatible model
 ok 2 - selects and validates an OpenAI-compatible model
   ---
   duration_ms: 7.089564
   type: 'test'
   ...
 # Subtest: maps both system instruction modes and rejects unclaimed config
 ok 3 - maps both system instruction modes and rejects unclaimed config
   ---
   duration_ms: 0.95282
   type: 'test'
   ...
 # Subtest: maps normalized stdio and streamable HTTP MCP servers
 ok 4 - maps normalized stdio and streamable HTTP MCP servers
   ---
   duration_ms: 1.214499
   type: 'test'
   ...
 # Subtest: explicit HTTP model hosts support Docker and Kubernetes gateways
 ok 5 - explicit HTTP model hosts support Docker and Kubernetes gateways
   ---
   duration_ms: 2.878206
   type: 'test'
   ...
 # Subtest: HTTP host settings compose with Qwen permissions without relaxing MCP trust
 ok 6 - HTTP host settings compose with Qwen permissions without relaxing MCP trust
   ---
   duration_ms: 0.352148
   ty...

GitHub Actions: TypeScript / 3_Test (Node 24).txt: test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

GitHub Actions: TypeScript / Test (Node 24): test: allow zero coverage in resource sampling checks

Conclusion: failure

View job details

##[group]Run bail() {
 �[36;1mbail() {�[0m
 �[36;1m  printf '::error::install-action: %s\n' "$*"�[0m

🧰 Additional context used

📚 Code guidelines: 1

.agents/skills/prepare-pr/SKILL.md — configured

📓 Path-based instructions: 3

Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.

CodeRabbit configuration file

Files:

crates/fabric-core/src/runtime.rs

Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.

CodeRabbit configuration file

Files:

tests/python/test_system_stats.py

Source excerpt: [ ] Any Rust change ran `just test-rust` Source excerpt: [ ] Any Rust change ran `cargo fmt --all -- --check` Source excerpt: [ ] `crates/fabric-core` changes ran both the Rust and Python suites

CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)

Files:

crates/fabric-core/src/runtime.rs

🔇 Additional comments: 2

tests/python/test_system_stats.py: 1

89-89: LGTM!

crates/fabric-core/src/runtime.rs: 1

5290-5290: 📐 Maintainability & Code Quality

No validation finding is established.

The repository requires these checks, but the absence of reported results does not show that they were skipped or failed. No explicit requirement to report the results was provided.

Walkthrough

The Rust and Python sampling tests now allow covered_fraction to equal zero. Both retain the upper bound of 1.0 and add a comment about samples without a nearby neighbor.

Changes

Sampling coverage tests

Layer / File(s)	Summary
Update coverage assertions `crates/fabric-core/src/runtime.rs`, `tests/python/test_system_stats.py`	Both tests accept `covered_fraction` values from 0.0 through 1.0. Comments note that a sample may lack a neighbor within twice the sampling interval.

Priority: Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Other

Merge Risk: Minimal · up to 56e59

The tests now accept the valid zero-coverage case while retaining the upper-bound and sampling checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title follows Conventional Commits format with the allowed `test` type, uses an imperative summary, stays under 72 characters, and accurately describes the test changes.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The pull request description is complete and follows the repository template. It explains the change, identifies reviewer starting points, documents validation and known full-suite limitations, and in…

Full details: Description check

Explanation

The pull request description is complete and follows the repository template. It explains the change, identifies reviewer starting points, documents validation and known full-suite limitations, and includes the required contribution and duplicate-work confirmations. The related-issues section uses the required action keyword and states that no issue applies.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

github-actions · 2026-10-24T00:05:43Z

Fern docs preview: https://nvidia-preview-pull-request-483.docs.buildwithfern.com/nemo/fabric

test: allow zero coverage in resource sampling checks

56e59c7

Sparse valid readings can produce zero covered time. Accept the full coverage range while preserving spike, artifact and result checks. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>

copy-pr-bot Bot deployed to fern October 24, 2026 00:01 Active

This was referenced Oct 25, 2026

feat: save and inspect opt-in invocation records #423

Open

feat: record requests that return no invocation result #424

Open

feat: save invocation resource observations in run records #426

Open

mnajafian-nv marked this pull request as ready for review October 25, 2026 17:55

mnajafian-nv requested a review from a team as a code owner October 25, 2026 17:55

This was referenced Oct 25, 2026

feat: compare task attempts with accounting coverage #480

Open

feat: preserve execution facts in saved run records #481

Open

Navigation Menu

Repository navigation

test: allow zero coverage in resource sampling checks - #483

test: allow zero coverage in resource sampling checks#483
mnajafian-nv wants to merge 1 commit into
NVIDIA:mainfrom
mnajafian-nv:test/allow-zero-sampling-coverage

mnajafian-nv commented Oct 24, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 24, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mnajafian-nv commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Details

Validation

Where should the reviewer start?

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

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mnajafian-nv commented Oct 24, 2026 •

edited

Loading

coderabbitai Bot commented Oct 24, 2026 •

edited

Loading