Skip to content

Fix/simplify event loop test#1405

Merged
mnajafian-nv merged 1 commit intorelease/1.4from
fix/simplify-event-loop-test
Jan 14, 2026
Merged

Fix/simplify event loop test#1405
mnajafian-nv merged 1 commit intorelease/1.4from
fix/simplify-event-loop-test

Conversation

@mnajafian-nv
Copy link
Contributor

@mnajafian-nv mnajafian-nv commented Jan 14, 2026

Description

fix: simplify event loop test to assert_called

The test was checking exact call counts of get_running_loop() which
varies by architecture, Python version, and pydantic-core version.
This caused repeated CI failures (#1383, #1396).

Simplified to assert_called() since:

  • Exact call count is an implementation detail
  • Other tests already cover the event loop code path
  • The previous approach was not sustainable

Fixes: #1396

By Submitting this PR I confirm:

  • I am familiar with the Contributing Guidelines.
  • We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license.
    • Any contribution which contains commits that are not Signed-Off will not be accepted.
  • When the PR is ready for review, new or existing tests cover these changes.
  • When the PR is ready for review, the documentation is up to date with these changes.

Summary by CodeRabbit

  • Tests
    • Simplified event-loop verification by replacing architecture-dependent assertions with a basic check that the running event loop is accessed.
    • Renamed and clarified the related test to reflect that only access is validated, improving cross-environment reliability and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@mnajafian-nv mnajafian-nv requested a review from a team as a code owner January 14, 2026 14:39
@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

Walkthrough

Test in the tool wrapper suite was renamed and simplified: it now checks that the event loop is accessed by asserting get_running_loop was called, removing platform-specific call-count checks and the platform import.

Changes

Cohort / File(s) Summary
Test Simplification
packages/nvidia_nat_agno/tests/test_tool_wrapper.py
Renamed test_get_event_loop_calledtest_event_loop_is_accessed. Removed platform import and per-architecture expected call-count logic. Assertion simplified to verify get_running_loop was called (no platform-specific counts).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix/simplify event loop test' is concise, descriptive, and uses imperative mood, clearly summarizing the main change to simplify the event loop test.
Linked Issues check ✅ Passed The PR changes align with the linked issue #1396, which is a forward-merge PR to keep develop up-to-date with release/1.4 by resolving CI failures related to event loop test flakiness.
Out of Scope Changes check ✅ Passed All changes are scoped to simplifying the event loop test in test_tool_wrapper.py by reducing architecture-specific assertions, directly addressing the CI failure issue mentioned in the PR objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
packages/nvidia_nat_agno/tests/test_tool_wrapper.py (1)

178-192: Consider using assert_called() for cleaner mock assertion.

The test logic is correct and the simplification aligns with the PR objective. However, using the mock's built-in assert_called() method is more idiomatic and provides better error messages on failure.

✨ Suggested improvement
-        assert mock_get_running_loop.called, ("get_running_loop should be called to access the event loop")
+        mock_get_running_loop.assert_called()

This removes the need for a custom error message since assert_called() provides a clear failure message automatically.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d61d01a and 8cf478b.

📒 Files selected for processing (1)
  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf with PEP 8 base and 'column_limit = 120' for code formatting
Use 'ruff check --fix' for linting with configuration from 'pyproject.toml', fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer 'collections.abc' / 'typing' abstractions (e.g., 'Sequence' over 'list') for type hints
Use 'typing.Annotated' for units or extra metadata when useful
Treat 'pyright' warnings (configured in 'pyproject.toml') as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare 'raise' statements when re-raising, and use 'logger.error()' for logging (not 'logger.exception()') to avoid duplicate stack trace output
When catching and logging exceptions without re-raising, always use 'logger.exception()' (equivalent to 'logger.error(exc_info=True)') to capture full stack trace information
Pydantic models using 'SecretStr', 'SerializableSecretStr', or 'OptionalSecretStr' should use 'default=None' for optional fields and 'default_factory=lambda: SerializableSecretStr("")' for non-optional fields to avoid initialization bugs
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period
Surround code entities in docstrings with backticks to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer 'httpx' with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use 'async'/'await' for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with 'functools.lru_cache' or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
**/*.{py,yaml,yml,json,toml}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Indent with 4 spaces (never tabs) and ensure every file ends with a single newline

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
**/test_*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/test_*.py: Use pytest with pytest-asyncio for asynchronous code testing
Test functions should be named with 'test_' prefix using snake_case
Extract frequently repeated code in tests into pytest fixtures with the 'fixture_' prefix on the function name and a 'name' argument in the decorator
Mock external services with 'pytest_httpserver' or 'unittest.mock' instead of hitting live endpoints in tests
Mark slow tests with '@pytest.mark.slow' so they can be skipped in the default test suite
Mark integration tests requiring external services with '@pytest.mark.integration' so they can be skipped in the default test suite

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
**/*.{py,js,ts,tsx,jsx,sh,yaml,yml,json,toml,md,mdx,rst}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.{py,js,ts,tsx,jsx,sh,yaml,yml,json,toml,md,mdx,rst}: Every file must start with the standard SPDX Apache-2.0 header
Confirm that copyright years are up-to-date whenever a file is changed
All source files must include the SPDX Apache-2.0 header template

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
**/*.{py,md,mdx,rst}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Version numbers are derived automatically by 'setuptools-scm'; never hard-code them in code or docs

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
**/*

⚙️ CodeRabbit configuration file

**/*: # Code Review Instructions

  • Ensure the code follows best practices and coding standards. - For Python code, follow
    PEP 20 and
    PEP 8 for style guidelines.
  • Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of None,
    in that situation no return type hint is needed).
    Example:
    def my_function(param1: int, param2: str) -> bool:
        pass
  • For Python exception handling, ensure proper stack trace preservation:
    • When re-raising exceptions: use bare raise statements to maintain the original stack trace,
      and use logger.error() (not logger.exception()) to avoid duplicate stack trace output.
    • When catching and logging exceptions without re-raising: always use logger.exception()
      to capture the full stack trace information.

Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any

words listed in the ci/vale/styles/config/vocabularies/nat/reject.txt file, words that might appear to be
spelling mistakes but are listed in the ci/vale/styles/config/vocabularies/nat/accept.txt file are OK.

  • Documentation in Markdown files should not contain usage of a possessive 's with inanimate objects
    (ex: "the system's performance" should be "the performance of the system").
  • Documentation in Markdown files should not use NAT as an acronym, always spell out NeMo Agent Toolkit.
    The exception to this rule is when referring to package names or code identifiers that contain "nat", th...

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
packages/**/*

⚙️ CodeRabbit configuration file

packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain a pyproject.toml file. - The pyproject.toml file should declare a dependency on nvidia-nat or another package with a name starting
with nvidia-nat-. This dependency should be declared using ~=<version>, and the version should be a two
digit version (ex: ~=1.0).

  • Not all packages contain Python code, if they do they should also contain their own set of tests, in a
    tests/ directory at the same level as the pyproject.toml file.
  • When adding a new package, that new package name (as defined in the pyproject.toml file) should
    be added as a dependency to the nvidia-nat-all package in packages/nvidia_nat_all/pyproject.toml

Files:

  • packages/nvidia_nat_agno/tests/test_tool_wrapper.py
🧬 Code graph analysis (1)
packages/nvidia_nat_agno/tests/test_tool_wrapper.py (1)
packages/nvidia_nat_agno/src/nat/plugins/agno/tool_wrapper.py (1)
  • agno_tool_wrapper (293-361)

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mnajafian-nv mnajafian-nv changed the base branch from develop to release/1.4 January 14, 2026 14:39
@mnajafian-nv mnajafian-nv added improvement Improvement to existing functionality non-breaking Non-breaking change labels Jan 14, 2026
@mnajafian-nv mnajafian-nv force-pushed the fix/simplify-event-loop-test branch from 525ccff to d61d01a Compare January 14, 2026 15:05
Replace exact call count assertion with assert_called().
Call count varies by arch/Python/pydantic-core version and
has caused repeated CI failures (#1383, #1396).

Fixes: #1396
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
@mnajafian-nv mnajafian-nv force-pushed the fix/simplify-event-loop-test branch from d61d01a to 8cf478b Compare January 14, 2026 15:20
@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.95%. Comparing base (f0bbabe) to head (8cf478b).
⚠️ Report is 2 commits behind head on release/1.4.

Additional details and impacted files
@@             Coverage Diff              @@
##           release/1.4    #1405   +/-   ##
============================================
  Coverage        74.95%   74.95%           
============================================
  Files              553      553           
  Lines            38835    38835           
============================================
  Hits             29109    29109           
  Misses            9726     9726           
Flag Coverage Δ
unittests 74.95% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mnajafian-nv mnajafian-nv merged commit 8929c6f into release/1.4 Jan 14, 2026
17 checks passed
Jerryguan777 pushed a commit to Jerryguan777/NeMo-Agent-Toolkit that referenced this pull request Jan 28, 2026
## Description
fix: simplify event loop test to assert_called

The test was checking exact call counts of get_running_loop() which
varies by architecture, Python version, and pydantic-core version.
This caused repeated CI failures (NVIDIA#1383, NVIDIA#1396).

Simplified to assert_called() since:
- Exact call count is an implementation detail
- Other tests already cover the event loop code path
- The previous approach was not sustainable

Fixes: NVIDIA#1396

## By Submitting this PR I confirm:
- I am familiar with the [Contributing
Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md).
- We require that all contributors "sign-off" on their commits. This
certifies that the contribution is your original work, or you have
rights to submit it under the same license, or a compatible license.
- Any contribution which contains commits that are not Signed-Off will
not be accepted.
- When the PR is ready for review, new or existing tests cover these
changes.
- When the PR is ready for review, the documentation is up to date with
these changes.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Tests**
* Simplified event-loop verification by replacing architecture-dependent
assertions with a basic check that the running event loop is accessed.
* Renamed and clarified the related test to reflect that only access is
validated, improving cross-environment reliability and maintainability.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement to existing functionality non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants