-
Notifications
You must be signed in to change notification settings - Fork 27
chore: Add SpannerMockServer #667
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
Conversation
bhatt4982
commented
Dec 5, 2025
- Code is combination of both python-spanner and python-spanner-sqlalchemy
- Added system tests for verification
- Added nox for project configuration and verification
- Added workflow to publish to PyPi
Summary of ChangesHello @bhatt4982, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Python library, Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
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.
Code Review
This pull request introduces a comprehensive mock server for Google Cloud Spanner, which is a valuable addition for facilitating testing. The implementation includes the core mock logic, gRPC service definitions, helper utilities for tests, and initial system tests. The code is generally well-structured. However, I've identified a critical bug in the mock Spanner logic that could lead to incorrect test behavior, along with some high-severity issues related to unreachable code from incorrect dictionary handling. I've also found several medium-severity issues in the noxfile.py configuration, such as using a non-existent Python version, inconsistent linting rules, and an undefined nox session. The suggested changes aim to fix these bugs and improve the robustness and maintainability of the new mock server.
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
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.
Code Review
This pull request introduces a new Python package spannermockserver, a lightweight in-memory mock server for Google Cloud Spanner. The package includes mock implementations for SpannerServicer and DatabaseAdminServicer, along with a noxfile.py for project automation and system tests for verification.
The overall structure is well-organized, and the use of a mock server for testing is a great practice. I've identified a few areas for improvement, primarily concerning potential bugs in the mock implementation and configuration inconsistencies. My main concerns are:
- A bug in
mock_spanner.pythat nullifies a transaction parameter. - Unsafe dictionary access in the mock servicers that could lead to
KeyErrorexceptions instead of graceful gRPC errors. - Inconsistencies in the
noxfile.pyconfiguration for linting and session names.
I've provided specific suggestions to address these points. Once these are addressed, this will be a solid foundation for the mock server.
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/pyproject.toml
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/spannermockserver/mock_spanner.py
Show resolved
Hide resolved
olavloite
left a comment
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.
LGTM, with some minor nits/requests. These can be picked up in follow-up PRs if that is easier.
...pers/spannerlib-python/spannermockserver/spannermockserver/testbase/mock_server_test_base.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/tests/integration/test_mock_spanner.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannermockserver/noxfile.py
Outdated
Show resolved
Hide resolved
…er for Cloud Spanner, including gRPC stubs, mock services, and test utilities.
…and partitioning, and improve test cleanup.
…and create a README for the generated files.
…and remove the local publish nox session.
…essions to run system tests
…streaming_sql` method.