Merged
Conversation
Reviewer's GuideReplace autogenerated unittest stubs with concrete pytest-style tests for multiple Ibutsu API client modules, using shared mock_api_client and mock_rest_response fixtures to validate response deserialization, HTTP methods, URLs, query parameters, and request bodies including file uploads. 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 (68.68%) 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 #47 +/- ##
===========================================
+ Coverage 57.22% 68.68% +11.45%
===========================================
Files 58 58
Lines 4783 4783
Branches 496 496
===========================================
+ Hits 2737 3285 +548
+ Misses 1996 1338 -658
- Partials 50 160 +110
Flags with carried forward coverage won't be shown. Click here to find out more. 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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/test_dashboard_api.py:87-96` </location>
<code_context>
+ def test_get_dashboard_list(self, mock_api_client, mock_rest_response):
</code_context>
<issue_to_address>
**nitpick (testing):** Use URL parsing for query parameter assertions to make the test less brittle
In `test_get_dashboard_list` (and `test_admin_get_project_list`), you’re asserting query params by checking substrings like `"page=1"` and `"pageSize=25"` in the URL, which is brittle if ordering or encoding changes. Since other tests (`RunApi`/`ProjectApi`/`ResultApi`) already use `urlparse` and `parse_qs`, please follow the same pattern here and assert on `query_params["page"]` and `query_params["pageSize"]` instead.
</issue_to_address>
### Comment 2
<location> `test/test_artifact_api.py:159-163` </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>
### Comment 3
<location> `test/test_artifact_api.py:160-163` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, 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>
### Comment 4
<location> `test/test_artifact_api.py:156-163` </location>
<code_context>
def test_upload_artifact(self, mock_api_client, mock_rest_response):
"""Test case for upload_artifact"""
api = ArtifactApi(api_client=mock_api_client)
run_id = uuid4()
filename = "test.txt"
file_content = b"content"
artifact_data = {
"id": str(uuid4()),
"filename": filename,
"result_id": None,
"run_id": str(run_id),
}
# Mock the API response
mock_response = mock_rest_response(data=artifact_data, status=201)
mock_api_client.call_api.return_value = mock_response
# Call the API
response = api.upload_artifact(filename=filename, file=file_content, run_id=run_id)
# Verify result
assert isinstance(response, Artifact)
assert response.filename == filename
assert str(response.run_id) == str(run_id)
# Verify call
mock_api_client.call_api.assert_called_once()
args, _kwargs = mock_api_client.call_api.call_args
assert args[0] == "POST"
assert args[1].endswith("/artifact")
# Verify form params include file and metadata
# args[4] is post_params
post_params = args[4]
# Check if file is in post_params
# post_params is a list of tuples
file_found = False
for param in post_params:
if param[0] == "file" and param[1][1] == file_content:
# param[1] should be (filename, filedata, mimetype)
file_found = True
break
assert file_found
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use any() instead of for loop ([`use-any`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-any/))
```suggestion
file_found = any(
param[0] == "file" and param[1][1] == file_content
for param in post_params
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for API modules in the ibutsu_client, replacing auto-generated test stubs with functional unit tests. The tests use pytest fixtures for mocking and validate both API request construction and response deserialization.
- Replaces empty unittest-based stubs with pytest-based tests using fixtures
- Adds full test coverage for CRUD operations across 6 API modules
- Implements consistent patterns for mocking API calls and validating responses
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_run_api.py | Comprehensive tests for RunApi including add, get, list, update, and bulk update operations with proper URL parsing for query parameter validation |
| test/test_result_api.py | Complete test suite for ResultApi CRUD operations with robust query parameter validation using URL parsing |
| test/test_project_api.py | Full coverage for ProjectApi including standard CRUD operations and filter params endpoint testing |
| test/test_dashboard_api.py | Tests for DashboardApi covering all CRUD operations including delete functionality; uses string matching for URL verification |
| test/test_artifact_api.py | Complete test coverage for ArtifactApi including file upload/download, view, and delete operations; uses string matching for URL verification |
| test/test_admin_project_management_api.py | Full test suite for AdminProjectManagementApi admin endpoints; uses string matching for URL verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
115
to
117
| assert "/admin/project" in args[1] | ||
| assert "page=1" in args[1] | ||
| assert "pageSize=25" in args[1] |
There was a problem hiding this comment.
The query parameter verification using string matching is less robust than the URL parsing approach used in other test files (e.g., test_run_api.py, test_result_api.py). Consider using urlparse and parse_qs for consistency and to avoid false positives if the parameter appears elsewhere in the URL.
Example:
from urllib.parse import parse_qs, urlparse
parsed_url = urlparse(args[1])
assert parsed_url.path.endswith("/admin/project")
query_params = parse_qs(parsed_url.query)
assert query_params["page"] == ["1"]
assert query_params["pageSize"] == ["25"]
test/test_dashboard_api.py
Outdated
Comment on lines
115
to
117
| assert "/dashboard" in args[1] | ||
| assert "page=1" in args[1] | ||
| assert "pageSize=25" in args[1] |
There was a problem hiding this comment.
The query parameter verification using string matching is less robust than the URL parsing approach used in other test files (e.g., test_run_api.py, test_result_api.py). Consider using urlparse and parse_qs for consistency and to avoid false positives if the parameter appears elsewhere in the URL.
Example:
from urllib.parse import parse_qs, urlparse
parsed_url = urlparse(args[1])
assert parsed_url.path.endswith("/dashboard")
query_params = parse_qs(parsed_url.query)
assert query_params["page"] == ["1"]
assert query_params["pageSize"] == ["25"]
Comment on lines
+42
to
+44
| assert "/artifact" in args[1] | ||
| assert "page=1" in args[1] | ||
| assert "pageSize=25" in args[1] |
There was a problem hiding this comment.
The query parameter verification using string matching is less robust than the URL parsing approach used in other test files (e.g., test_run_api.py, test_result_api.py). Consider using urlparse and parse_qs for consistency and to avoid false positives if the parameter appears elsewhere in the URL.
Example:
from urllib.parse import parse_qs, urlparse
parsed_url = urlparse(args[1])
assert parsed_url.path.endswith("/artifact")
query_params = parse_qs(parsed_url.query)
assert query_params["page"] == ["1"]
assert query_params["pageSize"] == ["25"]
mshriver
force-pushed
the
apis-coverage
branch
from
2020bcc to
714637f
Compare
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
Add comprehensive unit-style tests for ibutsu_client API modules using pytest-style tests and mocked API clients.
Tests: