-
Notifications
You must be signed in to change notification settings - Fork 498
Add comprehensive test coverage and tooling improvements #1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,56 @@ | ||||||||||||||||||||||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||
| # you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||
| # You may obtain a copy of the License at | ||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||
| # Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||
| # See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||
| # limitations under the License. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Codecov configuration for NeMo Agent Toolkit | ||||||||||||||||||||||||||||
| # See https://docs.codecov.com/docs/codecov-yaml for configuration options | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| coverage: | ||||||||||||||||||||||||||||
| precision: 2 | ||||||||||||||||||||||||||||
| round: down | ||||||||||||||||||||||||||||
| range: "70...100" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| status: | ||||||||||||||||||||||||||||
| project: | ||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||
| target: auto | ||||||||||||||||||||||||||||
| threshold: 1% | ||||||||||||||||||||||||||||
| informational: true | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you have informational set to true? |
||||||||||||||||||||||||||||
| patch: | ||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||
| target: auto | ||||||||||||||||||||||||||||
| threshold: 1% | ||||||||||||||||||||||||||||
| informational: true | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| parsers: | ||||||||||||||||||||||||||||
| gcov: | ||||||||||||||||||||||||||||
| branch_detection: | ||||||||||||||||||||||||||||
| conditional: yes | ||||||||||||||||||||||||||||
| loop: yes | ||||||||||||||||||||||||||||
| method: no | ||||||||||||||||||||||||||||
| macro: no | ||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+42
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is the default, why specify it at all? |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| comment: | ||||||||||||||||||||||||||||
| layout: "reach,diff,flags,files" | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does reach meaningfully add? |
||||||||||||||||||||||||||||
| behavior: default | ||||||||||||||||||||||||||||
| require_changes: true | ||||||||||||||||||||||||||||
| require_base: false | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should |
||||||||||||||||||||||||||||
| require_head: true | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ignore: | ||||||||||||||||||||||||||||
| - "tests/**/*" | ||||||||||||||||||||||||||||
| - "examples/**/*" | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Examples have their own code which could/should probably be part of coverage |
||||||||||||||||||||||||||||
| - "docs/**/*" | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A cursory search reveals no python code under docs |
||||||||||||||||||||||||||||
| - "**/__pycache__/**" | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is ignored by default |
||||||||||||||||||||||||||||
| - "**/conftest.py" | ||||||||||||||||||||||||||||
|
Comment on lines
+51
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing trailing newline. Per coding guidelines, every YAML file must end with a single newline. Add a newline after the last line. Proposed fix ignore:
- "tests/**/*"
- "examples/**/*"
- "docs/**/*"
- "**/__pycache__/**"
- "**/conftest.py"
+📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this added? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,11 @@ | |
|
|
||
| import dataclasses | ||
|
|
||
| import pytest | ||
| from pydantic import BaseModel | ||
|
|
||
| from nat.utils.string_utils import convert_to_str | ||
| from nat.utils.string_utils import truncate_string | ||
|
|
||
|
|
||
| class _M(BaseModel): | ||
|
|
@@ -42,3 +44,91 @@ def __str__(self): | |
| return f"C({self.x})" | ||
|
|
||
| assert convert_to_str(C(3)) == "C(3)" | ||
|
|
||
|
|
||
| def test_convert_to_str_pydantic_model(): | ||
| """Test convert_to_str with Pydantic BaseModel.""" | ||
| model = _M(a=42, b="test") | ||
| result = convert_to_str(model) | ||
| assert '"a":42' in result | ||
| assert '"b":"test"' in result | ||
|
|
||
|
|
||
| def test_convert_to_str_pydantic_model_excludes_none(): | ||
| """Test that Pydantic model serialization excludes None values.""" | ||
| model = _M(a=42) | ||
| result = convert_to_str(model) | ||
| assert '"a":42' in result | ||
| assert '"b"' not in result | ||
|
|
||
|
|
||
| def test_convert_to_str_empty_list(): | ||
| """Test convert_to_str with empty list.""" | ||
| assert convert_to_str([]) == "" | ||
|
|
||
|
|
||
| def test_convert_to_str_empty_dict(): | ||
| """Test convert_to_str with empty dictionary.""" | ||
| assert convert_to_str({}) == "" | ||
|
|
||
|
|
||
| def test_convert_to_str_nested_list(): | ||
| """Test convert_to_str with nested structures in list.""" | ||
| result = convert_to_str([[1, 2], [3, 4]]) | ||
| assert "[1, 2]" in result | ||
| assert "[3, 4]" in result | ||
|
|
||
|
|
||
| def test_convert_to_str_numeric_types(): | ||
| """Test convert_to_str with various numeric types.""" | ||
| assert convert_to_str(42) == "42" | ||
| assert convert_to_str(3.14) == "3.14" | ||
| assert convert_to_str(True) == "True" | ||
|
|
||
|
|
||
| class TestTruncateString: | ||
| """Tests for truncate_string function.""" | ||
|
|
||
| def test_truncate_none_input(self): | ||
| """Test that None input returns None.""" | ||
| assert truncate_string(None) is None | ||
|
|
||
| def test_truncate_empty_string(self): | ||
| """Test that empty string returns empty string.""" | ||
| assert truncate_string("") == "" | ||
|
Comment on lines
+96
to
+98
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Superfluous given test below |
||
|
|
||
| def test_truncate_short_string(self): | ||
| """Test that strings shorter than max_length are not truncated.""" | ||
| text = "Hello, World!" | ||
| assert truncate_string(text, max_length=100) == text | ||
|
|
||
| def test_truncate_exact_length(self): | ||
| """Test string with exact max_length is not truncated.""" | ||
| text = "x" * 100 | ||
| assert truncate_string(text, max_length=100) == text | ||
|
Comment on lines
+103
to
+108
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not reduce the length to something more representative (e.g. 20) |
||
|
|
||
| def test_truncate_long_string(self): | ||
| """Test that long strings are properly truncated with ellipsis.""" | ||
| text = "x" * 150 | ||
| result = truncate_string(text, max_length=100) | ||
| assert len(result) == 100 | ||
| assert result.endswith("...") | ||
| assert result == "x" * 97 + "..." | ||
|
|
||
| def test_truncate_custom_max_length(self): | ||
| """Test truncation with custom max_length.""" | ||
| text = "This is a test string" | ||
| result = truncate_string(text, max_length=10) | ||
| assert len(result) == 10 | ||
| assert result == "This is..." | ||
|
Comment on lines
+110
to
+123
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are superfluous |
||
|
|
||
| def test_truncate_very_short_max_length(self): | ||
| """Test truncation with very short max_length.""" | ||
| text = "Hello" | ||
| result = truncate_string(text, max_length=4) | ||
| assert result == "H..." | ||
|
|
||
| def test_truncate_preserves_type(self): | ||
| """Test that truncate_string preserves string type.""" | ||
| result = truncate_string("test", max_length=100) | ||
| assert isinstance(result, str) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A code search reveals that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the default, why specify it at all?