Gemini3 iteration on extending coverage for models - #46
Merged
Merged
Conversation
Reviewer's GuideRefactors several model tests from unittest stubs to pytest-based, behavior-focused tests while updating multiple Pydantic model to_json implementations to use model_dump_json with aliases and exclude_none semantics. Class diagram for updated Pydantic model to_json implementationsclassDiagram
class PydanticBaseModel
class Artifact {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class ArtifactList {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class Dashboard {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class Group {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class Pagination {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class Token {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class User {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class WidgetConfig {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
class WidgetType {
+to_json() str
+to_str() str
+to_dict() dict
+from_json(json_str str) Self
}
Artifact --|> PydanticBaseModel
ArtifactList --|> PydanticBaseModel
Dashboard --|> PydanticBaseModel
Group --|> PydanticBaseModel
Pagination --|> PydanticBaseModel
Token --|> PydanticBaseModel
User --|> PydanticBaseModel
WidgetConfig --|> PydanticBaseModel
WidgetType --|> PydanticBaseModel
Flow diagram for new to_json behavior using model_dump_jsonflowchart LR
A["Call to_json on model instance"] --> B["Invoke model_dump_json with by_alias true and exclude_none true"]
B --> C["Pydantic builds dict using field aliases"]
C --> D["Pydantic removes fields with None values"]
D --> E["Pydantic serializes to JSON string"]
E --> F["Return JSON string from to_json"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (57.22%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
===========================================
+ Coverage 46.43% 57.22% +10.78%
===========================================
Files 58 58
Lines 4783 4783
Branches 496 496
===========================================
+ Hits 2221 2737 +516
+ Misses 2521 1996 -525
- Partials 41 50 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several model test classes repeat very similar patterns (e.g., creation/to_dict/to_json/from_dict/from_json/roundtrip); consider using pytest parametrization or helper functions/fixtures to reduce duplication and keep the tests easier to maintain.
- You switched several models’ to_json implementations to use model_dump_json(by_alias=True, exclude_none=True); if any of those models’ to_dict methods perform custom transformations beyond alias/None handling, consider reusing that logic (or centralizing it) so dict and JSON representations remain consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several model test classes repeat very similar patterns (e.g., creation/to_dict/to_json/from_dict/from_json/roundtrip); consider using pytest parametrization or helper functions/fixtures to reduce duplication and keep the tests easier to maintain.
- You switched several models’ to_json implementations to use model_dump_json(by_alias=True, exclude_none=True); if any of those models’ to_dict methods perform custom transformations beyond alias/None handling, consider reusing that logic (or centralizing it) so dict and JSON representations remain consistent.
## Individual Comments
### Comment 1
<location> `test/test_result.py:80` </location>
<code_context>
+ r = Result(result=res)
+ assert r.result == res
+
+ def test_result_validate_enum_invalid(self):
+ """Test invalid enum values for result field raises ValueError"""
+ with pytest.raises(ValueError, match="must be one of enum values"):
+ Result(result="invalid_status")
+
</code_context>
<issue_to_address>
**suggestion (testing):** The enum invalid-value test is brittle because it asserts on the exact error message text.
Matching the full error string makes this test fragile against minor wording or validation changes in pydantic. Consider either asserting only that a ValueError is raised (no match argument) or using a broader regex (e.g. just "enum") so the test still verifies invalid values are rejected without depending on the exact message text.
```suggestion
# Use a broad match to avoid brittleness on exact error message wording
with pytest.raises(ValueError, match="enum"):
```
</issue_to_address>
### Comment 2
<location> `test/test_pagination.py:17` </location>
<code_context>
-
-if __name__ == "__main__":
- unittest.main()
+class TestPagination:
+ """Pagination model tests"""
+
</code_context>
<issue_to_address>
**issue (testing):** There are no tests covering the updated `to_json` behavior for several other models (Token, User, WidgetConfig, WidgetType), even though their implementations were changed similarly.
Since these models now share the same updated `to_json` implementation, please add tests for Token, User, WidgetConfig, and WidgetType similar to the existing pagination tests (e.g., `test_pagination_to_json` and the `*_none_values_excluded` cases). Those tests should confirm that the JSON is valid, uses field aliases where applicable, and excludes `None` values while still allowing explicitly required `None` fields when dictated by the model’s logic.
</issue_to_address>
### Comment 3
<location> `test/test_artifact_list.py:55-66` </location>
<code_context>
+ assert "pagination" in artifact_list_dict
+ assert artifact_list_dict["pagination"]["page"] == 1
+
+ def test_artifact_list_to_json(self):
+ """Test ArtifactList to_json conversion"""
+ artifact = Artifact(filename="json.txt")
+ artifact_list = ArtifactList(artifacts=[artifact])
+ artifact_list_json = artifact_list.to_json()
+
+ assert isinstance(artifact_list_json, str)
+ parsed = json.loads(artifact_list_json)
+ assert len(parsed["artifacts"]) == 1
+ assert parsed["artifacts"][0]["filename"] == "json.txt"
+
+ def test_artifact_list_from_dict(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a JSON serialization test for a completely empty ArtifactList to cover the new `exclude_none` behavior.
Since `to_json` now calls `model_dump_json(..., exclude_none=True)`, please add a test for an empty `ArtifactList()` (both `artifacts` and `pagination` as `None`). This should verify that the resulting JSON omits these keys entirely, ensuring behavior is correct for consumers that depend on absent empty fields.
```suggestion
def test_artifact_list_to_json(self):
"""Test ArtifactList to_json conversion"""
artifact = Artifact(filename="json.txt")
artifact_list = ArtifactList(artifacts=[artifact])
artifact_list_json = artifact_list.to_json()
assert isinstance(artifact_list_json, str)
parsed = json.loads(artifact_list_json)
assert len(parsed["artifacts"]) == 1
assert parsed["artifacts"][0]["filename"] == "json.txt"
def test_artifact_list_to_json_empty(self):
"""Test ArtifactList to_json conversion for an empty list with exclude_none behavior"""
artifact_list = ArtifactList() # artifacts=None, pagination=None by default
artifact_list_json = artifact_list.to_json()
assert isinstance(artifact_list_json, str)
parsed = json.loads(artifact_list_json)
# With exclude_none=True, optional None fields should be omitted entirely
assert "artifacts" not in parsed
assert "pagination" not in parsed
def test_artifact_list_from_dict(self):
```
</issue_to_address>
### Comment 4
<location> `test/test_result.py:74-76` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mshriver
force-pushed
the
models-coverage
branch
from
November 24, 2025 13:46
7870d5f to
fe8e4c8
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes and extends test coverage for model classes in the ibutsu_client package. The changes migrate from unittest to pytest-based tests and update model implementations to use pydantic v2's model_dump_json method.
- Converted 7 test files from unittest to pytest with comprehensive test coverage
- Updated 9 model files to use pydantic v2's
model_dump_jsonmethod - Added extensive testing for serialization, deserialization, and edge cases
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_run.py | Converted to pytest with comprehensive tests for Run model serialization/deserialization |
| test/test_result.py | Converted to pytest with enum validation and comprehensive coverage for Result model |
| test/test_project.py | Converted to pytest with tests for Project model operations |
| test/test_pagination.py | Converted to pytest with alias-aware tests for Pagination model |
| test/test_group.py | Converted to pytest with basic Group model tests |
| test/test_dashboard.py | Converted to pytest with Dashboard model tests |
| test/test_artifact_list.py | Converted to pytest with nested model tests for ArtifactList |
| test/test_artifact.py | Converted to pytest with Artifact model tests |
| ibutsu_client/models/widget_type.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/widget_config.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/user.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/token.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/pagination.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/group.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/dashboard.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/artifact_list.py | Updated to_json to use model_dump_json with exclude_none |
| ibutsu_client/models/artifact.py | Updated to_json to use model_dump_json with exclude_none |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address a bunch of TODOs since pydantic v2 is integrated now
mshriver
force-pushed
the
models-coverage
branch
from
November 24, 2025 14:36
fe8e4c8 to
1a12e6e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve model serialization behavior and significantly expand test coverage for core client models.
Bug Fixes:
Enhancements:
Tests: