Feature/sqlite repo #71
Open
Conversation
This file contains 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
…om blacksheep framework. most of these layers do not have to be tied to a framework's folder. any other framework can simply consume these layers as well
…sing functionality to make tests green
…heir type annotaitons. added e2e api testing support in start_server and conftest. added unit test configuration setup in pyproject. ignore any db files in project
…st-integration marker WIP. adds support for test markers
…app level infra setup using DI using rodi's Container that sets up database and session
…B. refactored code to use container to setup database using start_test_database and asyncSetUp. WIP: test_fetch_facilities
Unstable, active and WIP: This is where things start taking shape for setting test coverage `GeoLocationServiceSqlRepoDBTest` to implement issue: #26 ; - major rewrite of test services for integration test for Sqlite Repo DB. refactored code to use container to setup database using start_test_database and asyncSetUp. WIP: test_fetch_facilities - added sqlite models to support sqlite database. database.py enhances app level infra setup using DI using rodi's Container that sets up database and session
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request implements test coverage improvements and introduces a new File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @codecakes - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using more specific exception types in setUpTestDatabase class instead of RuntimeError. This would make error handling more precise and informative.
- In test_fetch_facilities, consider adding more detailed assertions to thoroughly test the functionality of the fetch_facilities method.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
xcov19/tests/test_services.py
Outdated
Comment on lines
210
to
213
| self._test_db = setUpTestDatabase() | ||
| await self._test_db.setup_test_database() | ||
| self._session = await self._test_db.start_async_session() | ||
| assert isinstance(self._session, AsyncSessionWrapper) |
There was a problem hiding this comment.
suggestion (testing): Improved test database setup
The new setup is more concise and uses a dedicated class for database setup. Consider adding error handling for the case where the session is not an AsyncSessionWrapper.
self._test_db = setUpTestDatabase()
await self._test_db.setup_test_database()
try:
self._session = await self._test_db.start_async_session()
if not isinstance(self._session, AsyncSessionWrapper):
raise TypeError("Session is not an AsyncSessionWrapper")
except Exception as e:
await self._test_db.teardown_test_database()
raise RuntimeError(f"Failed to set up test database: {str(e)}")
There was a problem hiding this comment.
@sourcery-ai It is implemented in this file:
xcov19/tests/test_services.py
Outdated
| await self._session.commit() | ||
| await self._stack.aclose() | ||
| print("async test server closing.") | ||
| await self._test_db.aclose() | ||
| await super().asyncTearDown() | ||
|
|
||
| def _patient_query_lookup_svc_using_repo( |
There was a problem hiding this comment.
question (testing): Changed return type of _patient_query_lookup_svc_using_repo
The return type has changed from a Callable to a List[FacilitiesResult]. Ensure that this change is intentional and that it doesn't break any existing tests or functionality.
xcov19/tests/test_services.py
Outdated
Comment on lines
227
to
234
| providers = await GeolocationQueryService.fetch_facilities( | ||
| self.dummy_reverse_geo_lookup_svc, | ||
| self.dummy_geolocation_query_json, | ||
| self._patient_query_lookup_svc_using_repo, | ||
| ) | ||
| assert providers | ||
| self.assertIsInstance(providers, list) | ||
| self.assertIs(len(providers), 1) |
There was a problem hiding this comment.
suggestion (testing): Implemented test_fetch_facilities
Good implementation of the TODO. Consider adding more assertions to check the content of the returned providers, not just the length and type.
providers = await GeolocationQueryService.fetch_facilities(
self.dummy_reverse_geo_lookup_svc,
self.dummy_geolocation_query_json,
self._patient_query_lookup_svc_using_repo,
)
self.assertIsInstance(providers, list)
self.assertEqual(len(providers), 1)
provider = providers[0]
self.assertIsInstance(provider, dict)
self.assertIn('name', provider)
self.assertIn('address', provider)
self.assertIn('coordinates', provider)
xcov19/tests/start_server.py
Outdated
Comment on lines
26
to
56
| class setUpTestDatabase: | ||
| def __init__(self) -> None: | ||
| self._stack = AsyncExitStack() | ||
| self._session: AsyncSession | AsyncSessionWrapper | None = None | ||
| self._container: ContainerProtocol = Container() | ||
|
|
||
| async def setup_test_database(self) -> None: | ||
| """Database setup for integration tests.""" | ||
| if not isinstance(self._container, Container): | ||
| raise RuntimeError("container not of type Container.") | ||
| configure_database_session(self._container, load_settings()) | ||
| engine = self._container.resolve(AsyncEngine) | ||
| await setup_database(engine) | ||
|
|
||
| async def start_async_session(self) -> AsyncSession | AsyncSessionWrapper: | ||
| if not isinstance(self._container, Container): | ||
| raise RuntimeError("container not of type Container.") | ||
| self._session = await self._stack.enter_async_context( | ||
| start_db_session(self._container) | ||
| ) | ||
| if not isinstance(self._session, AsyncSessionWrapper): | ||
| raise RuntimeError(f"{self._session} is not a AsyncSessionWrapper value.") | ||
| return self._session | ||
|
|
||
| async def aclose(self) -> None: | ||
| print("async closing test server db session closing.") | ||
| if not isinstance(self._session, AsyncSessionWrapper): | ||
| raise RuntimeError(f"{self._session} is not a AsyncSessionWrapper value.") | ||
| await self._session.commit() | ||
| await self._stack.aclose() | ||
| print("async test server closing.") |
There was a problem hiding this comment.
suggestion (testing): New setUpTestDatabase class for managing test database lifecycle
This is a good abstraction for managing the test database. Consider adding docstrings to methods and handling potential exceptions in the aclose method.
class SetUpTestDatabase:
"""Manages the lifecycle of the test database."""
async def setup_test_database(self) -> None:
"""Set up the database for integration tests."""
if not isinstance(self._container, Container):
raise ValueError("Container is not of type Container.")
configure_database_session(self._container, load_settings())
engine = self._container.resolve(AsyncEngine)
await setup_database(engine)
async def aclose(self) -> None:
"""Close the test database session."""
try:
if isinstance(self._session, AsyncSessionWrapper):
await self._session.commit()
await self._stack.aclose()
except Exception as e:
print(f"Error closing test database: {e}")
xcov19/tests/test_geolocation_api.py
Outdated
| @@ -4,7 +4,7 @@ | |||
| from blacksheep import Content, Response | |||
|
|
|||
|
|
|||
| @pytest.mark.integration | |||
There was a problem hiding this comment.
question (testing): Changed test marker from integration to api
Ensure that this change in test categorization is intentional and consistent with your testing strategy. Update any test runners or CI/CD pipelines that might rely on these markers.
There was a problem hiding this comment.
Hey @codecakes - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider implementing the TODO in the
_patient_query_lookup_svc_using_repomethod instead of leaving it as a placeholder. - In the
test_fetch_facilitiesmethod, either implement the full test or remove the commented-out code to avoid confusion.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
xcov19/tests/test_services.py
Outdated
Comment on lines
221
to
226
| def _patient_query_lookup_svc_using_repo( | ||
| self, address: Address, query: LocationQueryJSON | ||
| ) -> Callable[[Address, LocationQueryJSON], List[FacilitiesResult]]: ... | ||
| ) -> List[FacilitiesResult]: | ||
| # TODO: Implement a patient query lookup service | ||
| # that returns type List[FacilitiesResult] | ||
| ... |
There was a problem hiding this comment.
issue (testing): Implement the _patient_query_lookup_svc_using_repo method
This method is currently a stub. Implement it to return a List[FacilitiesResult] as indicated in the TODO comment. This is crucial for testing the patient query lookup functionality.
xcov19/tests/test_services.py
Outdated
Comment on lines
228
to
237
| async def test_fetch_facilities(self): | ||
| # TODO Implement test_fetch_facilities like this: | ||
| # providers = await GeolocationQueryService.fetch_facilities( | ||
| # dummy_reverse_geo_lookup_svc, | ||
| # dummy_geolocation_query_json, | ||
| # self._patient_query_lookup_svc_using_repo | ||
| # ) | ||
| ... | ||
| providers = await GeolocationQueryService.fetch_facilities( | ||
| self.dummy_reverse_geo_lookup_svc, | ||
| self.dummy_geolocation_query_json, | ||
| self._patient_query_lookup_svc_using_repo, | ||
| ) | ||
| assert providers | ||
| self.assertIsInstance(providers, list) | ||
| self.assertIs(len(providers), 1) |
There was a problem hiding this comment.
suggestion (testing): Good implementation of test_fetch_facilities
The test now properly calls GeolocationQueryService.fetch_facilities and asserts the result. Consider adding more specific assertions about the content of the returned providers.
async def test_fetch_facilities(self):
providers = await GeolocationQueryService.fetch_facilities(
self.dummy_reverse_geo_lookup_svc,
self.dummy_geolocation_query_json,
self._patient_query_lookup_svc_using_repo,
)
assert providers
self.assertIsInstance(providers, list)
self.assertEqual(len(providers), 1)
self.assertIsInstance(providers[0], dict)
self.assertIn('name', providers[0])
self.assertIn('address', providers[0])
xcov19/tests/start_server.py
Outdated
Comment on lines
38
to
74
| class setUpTestDatabase: | ||
| """Manages the lifecycle of the test database.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self._stack = AsyncExitStack() | ||
| self._session: AsyncSession | AsyncSessionWrapper | None = None | ||
| self._container: ContainerProtocol = Container() | ||
|
|
||
| async def setup_test_database(self) -> None: | ||
| """Database setup for integration tests.""" | ||
| if not isinstance(self._container, Container): | ||
| raise InvalidDIContainerTypeError("Container not of valid type.") | ||
| configure_database_session(self._container, load_settings()) | ||
| engine = self._container.resolve(AsyncEngine) | ||
| await setup_database(engine) | ||
|
|
||
| async def start_async_session(self) -> AsyncSession | AsyncSessionWrapper: | ||
| if not isinstance(self._container, Container): | ||
| raise InvalidDIContainerTypeError("Container not of valid type.") | ||
| self._session = await self._stack.enter_async_context( | ||
| start_db_session(self._container) | ||
| ) | ||
| if not isinstance(self._session, AsyncSessionWrapper): | ||
| raise InvalidSessionTypeError( | ||
| f"{self._session} is not a AsyncSessionWrapper value." | ||
| ) | ||
| return self._session | ||
|
|
||
| async def aclose(self) -> None: | ||
| print("async closing test server db session closing.") | ||
| if not isinstance(self._session, AsyncSessionWrapper): | ||
| raise InvalidSessionTypeError( | ||
| f"{self._session} is not a AsyncSessionWrapper value." | ||
| ) | ||
| await self._session.commit() | ||
| await self._stack.aclose() | ||
| print("async test server closing.") |
There was a problem hiding this comment.
suggestion (testing): Good refactoring of test database setup
The new setUpTestDatabase class improves the organization of test database setup and teardown. Consider adding more error handling and logging to improve debugging of test setup issues.
class SetUpTestDatabase:
"""Manages the lifecycle of the test database."""
def __init__(self) -> None:
self._stack = AsyncExitStack()
self._session: AsyncSession | AsyncSessionWrapper | None = None
self._container: ContainerProtocol = Container()
self._logger = logging.getLogger(__name__)
async def setup_test_database(self) -> None:
try:
if not isinstance(self._container, Container):
raise InvalidDIContainerTypeError("Container not of valid type.")
configure_database_session(self._container, load_settings())
engine = self._container.resolve(AsyncEngine)
await setup_database(engine)
except Exception as e:
self._logger.error(f"Error setting up test database: {e}")
raise
… service (#75) resolves #73 adds: - Dockerfile.build: This is the base multi-stage image. - Dockerfile: Main image to run the service using docker compose. - Dockerfile.test-integration: For running integration tests. <!-- Generated by sourcery-ai[bot]: start summary --> ## Summary by Sourcery Add Docker support for integration testing and running the service in a containerized environment. Update documentation to guide developers on setting up and using Docker for local testing and service emulation. Introduce Dockerfiles for building base, main, and integration test images, and add a Docker Compose configuration for service deployment. New Features: - Introduce Docker support for integration testing and running the service in a containerized environment using Docker Compose. Enhancements: - Update CONTRIBUTING.md and README.md to include instructions for setting up and running the containerized application, enhancing the documentation for developers. Build: - Add Dockerfile.build for creating a base multi-stage image, Dockerfile for the main service image, and Dockerfile.test-integration for integration testing. Deployment: - Add docker-compose.yml to define the containerized service setup, including shared configuration for ports, volumes, and logging. Documentation: - Enhance user-facing documentation in CONTRIBUTING.md and README.md to guide developers on using Docker for local integration testing and service emulation. <!-- Generated by sourcery-ai[bot]: end summary -->
…abels are created in container and spatialite is loaded successfully
Introduces changes to fix spatialite extension loading issue to support data fields in sqlachemy model type `POINT`. This change directly supports the work in #26 - Fixes spatialite extension as loadable with the updated dockerfile setup and correct implementation using aiosqlite compatibility - Temporary disables integration test. The repository return type needs to be refactored and return type changed to list dto facilities results <!-- Generated by sourcery-ai[bot]: start summary --> Fix SpatiaLite extension loading issue to support spatial data fields in SQLAlchemy models. Introduce a new `PointType` for handling geopoint data. Refactor test database setup and temporarily disable integration tests pending refactoring. Update build scripts and Makefile for improved Docker handling. New Features: - Introduce a new `PointType` class to handle geopoint data types in SQLAlchemy models, enabling support for spatial data fields like `POINT`. Bug Fixes: - Fix the loading of the SpatiaLite extension in the database setup to support spatial data operations. Enhancements: - Refactor the test database setup to use a new `SetUpTestDatabase` class, improving the management of test database lifecycle and session handling. Build: - Update the Makefile to include a new `set-docker` target and modify the `docker-run-server` target to remove orphan containers. Tests: - Temporarily disable integration tests due to the need for refactoring repository return types and changing return types to list DTO facilities results. Chores: - Update the `run.sh` script to include additional logging for file listing and database removal. <!-- Generated by sourcery-ai[bot]: end summary --> --------- Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @codecakes - I've reviewed your changes - here's some feedback:
Overall Comments:
- Can you provide some context on the removal of the SpatiaLite extension loading? Please confirm that all geospatial functionality and tests are still working as expected after this change.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Comment on lines
84
to
90
| async with engine.begin() as conn: | ||
| # Enable extension loading | ||
| await conn.execute(text("PRAGMA load_extension = 1")) | ||
| # db_logger.info("SQLAlchemy setup to load the SpatiaLite extension.") | ||
| # await conn.execute(text("SELECT load_extension('/opt/homebrew/Cellar/libspatialite/5.1.0_1/lib/mod_spatialite.dylib')")) | ||
| # await conn.execute(text("SELECT load_extension('mod_spatialite')")) | ||
| # see: https://sqlmodel.tiangolo.com/tutorial/relationship-attributes/cascade-delete-relationships/#enable-foreign-key-support-in-sqlite | ||
| await conn.execute(text("PRAGMA foreign_keys=ON")) | ||
| # test_result = await conn.execute(text("SELECT spatialite_version() as version;")) | ||
| # print(f"==== Spatialite Version: {test_result.fetchone()} ====") | ||
|
|
||
| await conn.run_sync(SQLModel.metadata.create_all) | ||
| await conn.commit() | ||
| db_logger.info("===== Database tables setup. =====") |
There was a problem hiding this comment.
suggestion: Consider implementing explicit error handling for database setup operations.
Wrapping the operations in a try-except block would make error handling more robust and aid in debugging if issues occur during database setup.
Suggested change
| async with engine.begin() as conn: | |
| # Enable extension loading | |
| await conn.execute(text("PRAGMA load_extension = 1")) | |
| # db_logger.info("SQLAlchemy setup to load the SpatiaLite extension.") | |
| # await conn.execute(text("SELECT load_extension('/opt/homebrew/Cellar/libspatialite/5.1.0_1/lib/mod_spatialite.dylib')")) | |
| # await conn.execute(text("SELECT load_extension('mod_spatialite')")) | |
| # see: https://sqlmodel.tiangolo.com/tutorial/relationship-attributes/cascade-delete-relationships/#enable-foreign-key-support-in-sqlite | |
| await conn.execute(text("PRAGMA foreign_keys=ON")) | |
| # test_result = await conn.execute(text("SELECT spatialite_version() as version;")) | |
| # print(f"==== Spatialite Version: {test_result.fetchone()} ====") | |
| await conn.run_sync(SQLModel.metadata.create_all) | |
| await conn.commit() | |
| db_logger.info("===== Database tables setup. =====") | |
| async with engine.begin() as conn: | |
| try: | |
| await conn.execute(text("PRAGMA load_extension = 1")) | |
| await conn.execute(text("PRAGMA foreign_keys=ON")) | |
| await conn.run_sync(SQLModel.metadata.create_all) | |
| await conn.commit() | |
| db_logger.info("===== Database tables setup. =====") | |
| except SQLAlchemyError as e: | |
| db_logger.error(f"Database setup failed: {str(e)}") | |
| raise |
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.
Implements test coverage and resolves #26
Summary by Sourcery
Clean up the database setup function by removing commented-out code related to SpatiaLite extension loading, improving code readability and maintainability.
Enhancements: