Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive mock implementation of the Google Slides API client for testing purposes, including an in-memory MockGoogleAPIClient class, a batch request processor handling multiple slide operations, snapshot utilities for test data persistence, and extensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Client as MockGoogleAPIClient
participant Batch as Batch Processor
participant Store as In-Memory Store
Test->>Client: create_presentation(config)
Client->>Store: store new presentation
Client-->>Test: return presentation_id
Test->>Client: delete_object(object_id, presentation_id)
Client->>Client: queue batch request
Test->>Client: flush_batch_update()
Client->>Batch: process_batch_requests(queued_requests)
Batch->>Store: retrieve presentation
Batch->>Store: mutate (delete/duplicate/create)
Batch-->>Client: return replies with results
Client->>Client: log batch operations
Client-->>Test: return batch results
sequenceDiagram
participant Test as Test Code
participant Snap as Snapshot Module
participant Client as GoogleAPIClient
participant Mock as MockGoogleAPIClient
participant Store as In-Memory Store
Test->>Snap: dump_presentation_snapshot(client, pres_id, path)
Snap->>Client: get_presentation_json(pres_id)
Client-->>Snap: return presentation JSON
Snap->>Snap: write JSON to file
Snap-->>Test: snapshot saved
Test->>Snap: load_presentation_snapshot(mock_client, path)
Snap->>Snap: read JSON from file
Snap->>Mock: seed_presentation(pres_id, json_data)
Mock->>Store: store presentation JSON
Mock-->>Test: return presentation_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/test_mock_client.py (3)
28-82: Consider using a pytest fixture for common mock client setup.Multiple test classes repeat
mock = MockGoogleAPIClient()setup. A fixture would reduce boilerplate and align with testing guidelines.♻️ Suggested fixture
Add at the top of the file after imports:
`@pytest.fixture` def mock_client(): """Fixture providing a fresh MockGoogleAPIClient for each test.""" return MockGoogleAPIClient()Then tests can use:
def test_is_initialized(self, mock_client): assert mock_client.is_initialized is TrueAs per coding guidelines: "Use
pytestfixtures for common setup patterns in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mock_client.py` around lines 28 - 82, Tests repeatedly instantiate MockGoogleAPIClient; replace this boilerplate with a pytest fixture by adding a fixture function that returns MockGoogleAPIClient (e.g., define a mock_client fixture) and update each test method signature to accept mock_client and use that instance instead of calling MockGoogleAPIClient() inside the body; update all tests referencing MockGoogleAPIClient (e.g., test_is_initialized, test_set_credentials_noop, test_initialize_credentials_noop, test_create_presentation, test_get_presentation_json, test_get_presentation_not_found, test_get_slide_json, test_get_slide_not_found, test_create_presentation_returns_unique_ids) to use the fixture.
429-429: Move import to the top of the file.Inline imports within test functions are less discoverable and can cause import errors to be hidden until the specific test runs.
🔧 Proposed fix
Add to the imports at the top of the file (line 16-22):
from gslides_api.request.request import ( CreateSlideRequest, DeleteObjectRequest, DuplicateObjectRequest, InsertTextRequest, UpdateSlidesPositionRequest, # Add this UpdateTextStyleRequest, )Then remove the inline import at line 429.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mock_client.py` at line 429, The test contains an inline import of UpdateSlidesPositionRequest which should be moved to the module-level imports; add UpdateSlidesPositionRequest to the existing import group that already imports CreateSlideRequest, DeleteObjectRequest, DuplicateObjectRequest, InsertTextRequest, UpdateTextStyleRequest (so the import list includes UpdateSlidesPositionRequest) and remove the inline "from gslides_api.request.request import UpdateSlidesPositionRequest" inside the test function to keep imports discoverable and consistent.
446-446: Move import to the top of the file.Same as above,
ThumbnailPropertiesshould be imported at the top with other domain imports.🔧 Proposed fix
Add to the imports at the top:
from gslides_api.domain.domain import ThumbnailPropertiesThen remove the inline import at line 446.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mock_client.py` at line 446, Move the inline import of ThumbnailProperties into the module's top-level import block with the other domain imports: add "from gslides_api.domain.domain import ThumbnailProperties" to the existing domain imports at the top and remove the inline "from gslides_api.domain.domain import ThumbnailProperties" that appears later in the test file; ensure there are no duplicate imports and run tests to confirm imports resolve correctly.gslides_api/mock/snapshots.py (2)
62-63: Specify explicit encoding for file I/O.Similarly, reading files should specify encoding for consistency across platforms.
🔧 Proposed fix
- with open(snapshot_path) as f: + with open(snapshot_path, encoding="utf-8") as f: data = json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/snapshots.py` around lines 62 - 63, The file open call that reads snapshot_path uses the system default encoding; update the open(...) call where snapshot_path is read (the with open(snapshot_path) as f: ... block that then calls json.load(f)) to specify an explicit encoding (e.g., encoding='utf-8') and an explicit mode ('r') to ensure consistent cross-platform behavior when loading JSON snapshots.
42-43: Specify explicit encoding for file I/O.Writing files without explicit encoding can cause issues on different platforms where the default encoding varies.
🔧 Proposed fix
- with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f: json.dump(data, f, indent=2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/snapshots.py` around lines 42 - 43, The file write uses open(output_path, "w") without an explicit encoding which can cause platform-specific issues; update the open call that writes snapshot JSON (the block using variables output_path and data and calling json.dump) to specify an explicit encoding (e.g., encoding="utf-8") when opening the file so the write is deterministic across platforms.gslides_api/mock/client.py (3)
212-222: Document thatpropertiesparameter is intentionally ignored.The
propertiesparameter is unused (flagged by static analysis). For a mock, returning a stub thumbnail regardless of properties is reasonable, but a brief comment would clarify this is intentional.📝 Proposed documentation
def slide_thumbnail( self, presentation_id: str, slide_id: str, properties: ThumbnailProperties ) -> ImageThumbnail: + # Note: properties parameter is intentionally ignored; mock returns a fixed stub. self.flush_batch_update() # Verify the presentation and slide exist self.get_slide_json(presentation_id, slide_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/client.py` around lines 212 - 222, The properties parameter in slide_thumbnail is intentionally unused; update the function (slide_thumbnail in client.py) to explicitly document this by adding a short comment like "properties intentionally ignored in mock — returning stub thumbnail" near the top of the method (or rename the parameter to _properties or assign it to _ to silence linters) while leaving the existing calls to flush_batch_update and get_slide_json and the ImageThumbnail return intact.
280-287: Type hint inconsistency withfolder_idparameter.Same issue as
duplicate_object- usesstr | Noneinstead ofOptional[str].🔧 Proposed fix
- def upload_image_to_drive(self, image_path, folder_id: str | None = None) -> str: + def upload_image_to_drive(self, image_path: str, folder_id: Optional[str] = None) -> str:Also adds missing type hint for
image_path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/client.py` around lines 280 - 287, Change the type hints in upload_image_to_drive to use typing.Optional and add a missing type for image_path: replace the parameter signature using `str | None` with `Optional[str]` for folder_id and annotate image_path as `str`; import Optional from typing if not already present. Also mirror the same fix used for duplicate_object to keep type hint style consistent across the module.
192-206: Inconsistent type hint syntax.Line 196 uses
Dict[str, str] | None(Python 3.10+ union syntax) while the rest of the file usesOptional[...]from typing. Consider using consistent style.🔧 Proposed fix for consistency
def duplicate_object( self, object_id: str, presentation_id: str, - id_map: Dict[str, str] | None = None, + id_map: Optional[Dict[str, str]] = None, ) -> str:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/client.py` around lines 192 - 206, The type hint for parameter id_map in duplicate_object uses Python 3.10 union syntax; change it to match the file's typing style by using Optional[Dict[str, str]] for id_map and ensure Optional is imported from typing if not already; update the function signature in duplicate_object and any related references to use Optional[...] instead of the `| None` union form so the file remains consistent.gslides_api/mock/batch_processor.py (1)
54-55: Usenext(iter(...))for single-key dict access.This is more idiomatic and slightly more efficient than creating a list.
🔧 Proposed fix
- request_type = list(request_dict.keys())[0] + request_type = next(iter(request_dict.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gslides_api/mock/batch_processor.py` around lines 54 - 55, Replace the idiomatic single-key dict access using list(request_dict.keys())[0] with next(iter(request_dict)) to avoid constructing a list; update the assignment of request_type in the code that sets request_type = list(request_dict.keys())[0] (in the batch processor logic) to request_type = next(iter(request_dict)) and keep request_body = request_dict[request_type]; if the dict could be empty, guard with an explicit check (e.g., if not request_dict: ...) before using next(iter(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gslides_api/mock/batch_processor.py`:
- Around line 196-198: The code currently returns a generated ID silently when
the source_id isn't found (id_mapping.get(source_id) or generate_id()), which
can hide test bugs; update the duplicate handling in the function containing
id_mapping and source_id so that when neither a slide nor page element is found
you log a warning (e.g., using the module logger or logging.warning) that
includes the missing source_id and that a synthetic ID was generated, then
continue to return {"duplicateObject": {"objectId": new_id}} to preserve
behavior; reference id_mapping, source_id, generate_id(), and the
{"duplicateObject": {"objectId": ...}} return in your change.
---
Nitpick comments:
In `@gslides_api/mock/batch_processor.py`:
- Around line 54-55: Replace the idiomatic single-key dict access using
list(request_dict.keys())[0] with next(iter(request_dict)) to avoid constructing
a list; update the assignment of request_type in the code that sets request_type
= list(request_dict.keys())[0] (in the batch processor logic) to request_type =
next(iter(request_dict)) and keep request_body = request_dict[request_type]; if
the dict could be empty, guard with an explicit check (e.g., if not
request_dict: ...) before using next(iter(...)).
In `@gslides_api/mock/client.py`:
- Around line 212-222: The properties parameter in slide_thumbnail is
intentionally unused; update the function (slide_thumbnail in client.py) to
explicitly document this by adding a short comment like "properties
intentionally ignored in mock — returning stub thumbnail" near the top of the
method (or rename the parameter to _properties or assign it to _ to silence
linters) while leaving the existing calls to flush_batch_update and
get_slide_json and the ImageThumbnail return intact.
- Around line 280-287: Change the type hints in upload_image_to_drive to use
typing.Optional and add a missing type for image_path: replace the parameter
signature using `str | None` with `Optional[str]` for folder_id and annotate
image_path as `str`; import Optional from typing if not already present. Also
mirror the same fix used for duplicate_object to keep type hint style consistent
across the module.
- Around line 192-206: The type hint for parameter id_map in duplicate_object
uses Python 3.10 union syntax; change it to match the file's typing style by
using Optional[Dict[str, str]] for id_map and ensure Optional is imported from
typing if not already; update the function signature in duplicate_object and any
related references to use Optional[...] instead of the `| None` union form so
the file remains consistent.
In `@gslides_api/mock/snapshots.py`:
- Around line 62-63: The file open call that reads snapshot_path uses the system
default encoding; update the open(...) call where snapshot_path is read (the
with open(snapshot_path) as f: ... block that then calls json.load(f)) to
specify an explicit encoding (e.g., encoding='utf-8') and an explicit mode ('r')
to ensure consistent cross-platform behavior when loading JSON snapshots.
- Around line 42-43: The file write uses open(output_path, "w") without an
explicit encoding which can cause platform-specific issues; update the open call
that writes snapshot JSON (the block using variables output_path and data and
calling json.dump) to specify an explicit encoding (e.g., encoding="utf-8") when
opening the file so the write is deterministic across platforms.
In `@tests/test_mock_client.py`:
- Around line 28-82: Tests repeatedly instantiate MockGoogleAPIClient; replace
this boilerplate with a pytest fixture by adding a fixture function that returns
MockGoogleAPIClient (e.g., define a mock_client fixture) and update each test
method signature to accept mock_client and use that instance instead of calling
MockGoogleAPIClient() inside the body; update all tests referencing
MockGoogleAPIClient (e.g., test_is_initialized, test_set_credentials_noop,
test_initialize_credentials_noop, test_create_presentation,
test_get_presentation_json, test_get_presentation_not_found,
test_get_slide_json, test_get_slide_not_found,
test_create_presentation_returns_unique_ids) to use the fixture.
- Line 429: The test contains an inline import of UpdateSlidesPositionRequest
which should be moved to the module-level imports; add
UpdateSlidesPositionRequest to the existing import group that already imports
CreateSlideRequest, DeleteObjectRequest, DuplicateObjectRequest,
InsertTextRequest, UpdateTextStyleRequest (so the import list includes
UpdateSlidesPositionRequest) and remove the inline "from
gslides_api.request.request import UpdateSlidesPositionRequest" inside the test
function to keep imports discoverable and consistent.
- Line 446: Move the inline import of ThumbnailProperties into the module's
top-level import block with the other domain imports: add "from
gslides_api.domain.domain import ThumbnailProperties" to the existing domain
imports at the top and remove the inline "from gslides_api.domain.domain import
ThumbnailProperties" that appears later in the test file; ensure there are no
duplicate imports and run tests to confirm imports resolve correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gslides_api/mock/__init__.pygslides_api/mock/batch_processor.pygslides_api/mock/client.pygslides_api/mock/snapshots.pytests/test_mock_client.py
Comment on lines
+196
to
+198
| # Object not found — still return a reply with a generated ID | ||
| new_id = id_mapping.get(source_id) or generate_id() | ||
| return {"duplicateObject": {"objectId": new_id}} |
Contributor
There was a problem hiding this comment.
Duplicating a nonexistent object silently succeeds.
When the source object isn't found (neither slide nor page element), the function returns a reply with a generated ID without any warning. This could mask bugs in test code where an invalid objectId is passed.
🛡️ Consider logging a warning
# Object not found — still return a reply with a generated ID
+ logger.warning(
+ f"duplicateObject: source object '{source_id}' not found in presentation"
+ )
new_id = id_mapping.get(source_id) or generate_id()
return {"duplicateObject": {"objectId": new_id}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gslides_api/mock/batch_processor.py` around lines 196 - 198, The code
currently returns a generated ID silently when the source_id isn't found
(id_mapping.get(source_id) or generate_id()), which can hide test bugs; update
the duplicate handling in the function containing id_mapping and source_id so
that when neither a slide nor page element is found you log a warning (e.g.,
using the module logger or logging.warning) that includes the missing source_id
and that a synthetic ID was generated, then continue to return
{"duplicateObject": {"objectId": new_id}} to preserve behavior; reference
id_mapping, source_id, generate_id(), and the {"duplicateObject": {"objectId":
...}} return in your change.
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
1 participant
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 CodeRabbit
New Features
Tests