Fix deploy - #4
Conversation
- Use __getattr__ for lazy loading of kafka components - Shortener service no longer imports confluent_kafka unnecessarily - Fixes ModuleNotFoundError at startup
- Add comprehensive e2e tests for all API endpoints - Tests run inside Docker network via docker compose --profile test - Fix gateway healthcheck to use 127.0.0.1 instead of localhost - Covers shorten, redirect, delete, and health endpoints
Summary of ChangesHello, 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 primarily focuses on enhancing the reliability and maintainability of the URL shortener service by introducing a comprehensive suite of end-to-end tests. It also includes an optimization to the shared module's import mechanism, ensuring that Kafka-related dependencies are only loaded when explicitly required, which can improve performance for services not interacting with Kafka. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for the service and refactors the shared module for better dependency management. The changes are generally good, but I have a few suggestions to improve the new test setup and the lazy-loading implementation. Specifically, I recommend making the Dockerfile for the e2e tests more explicit, adopting pytest for a more robust testing framework, and refining the __getattr__ implementation for better code style and correctness.
| def run_tests(): | ||
| """Run all e2e tests.""" | ||
| print("=" * 60) | ||
| print("URL Shortener E2E Tests") | ||
| print("=" * 60) | ||
|
|
||
| # Wait for services | ||
| print("\nWaiting for services to be ready...") | ||
| wait_for_services() | ||
|
|
||
| # Run tests | ||
| all_passed = True | ||
| test_classes = [TestShorten, TestRedirect, TestDelete, TestHealth] | ||
|
|
||
| for test_class in test_classes: | ||
| print(f"\n--- {test_class.__name__} ---") | ||
| instance = test_class() | ||
| for method_name in dir(instance): | ||
| if method_name.startswith("test_"): | ||
| try: | ||
| getattr(instance, method_name)() | ||
| except AssertionError as e: | ||
| print(f"✗ {method_name}: FAILED - {e}") | ||
| all_passed = False | ||
| except Exception as e: | ||
| print(f"✗ {method_name}: ERROR - {e}") | ||
| all_passed = False | ||
|
|
||
| print("\n" + "=" * 60) | ||
| if all_passed: | ||
| print("ALL TESTS PASSED ✓") | ||
| return 0 | ||
| else: | ||
| print("SOME TESTS FAILED ✗") | ||
| return 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| exit(run_tests()) |
There was a problem hiding this comment.
This manual test runner implementation is quite verbose and could be fragile. A more robust and standard approach would be to use a dedicated test framework like pytest.
By using pytest, you can:
- Remove this entire
run_testsfunction and theif __name__ == "__main__"block. - Let
pytesthandle test discovery and execution automatically. - Add
pytesttoe2e_tests/requirements.txt. - Change the
CMDine2e_tests/DockerfiletoCMD ["pytest"].
The wait_for_services() function could also be converted into a pytest session-scoped, autouse fixture for cleaner setup.
| if name == "kafka_producer": | ||
| from shared.kafka_utils import kafka_producer | ||
| globals()["kafka_producer"] = kafka_producer | ||
| return kafka_producer | ||
| if name == "run_consumer": | ||
| from shared.kafka_consumer import run_consumer | ||
| globals()["run_consumer"] = run_consumer | ||
| return run_consumer | ||
| if name == "ensure_purge_topic": | ||
| from shared.kafka_utils import ensure_purge_topic | ||
| globals()["ensure_purge_topic"] = ensure_purge_topic | ||
| return ensure_purge_topic | ||
| if name == "send_purge_event": | ||
| from shared.kafka_utils import send_purge_event | ||
| globals()["send_purge_event"] = send_purge_event | ||
| return send_purge_event | ||
| if name == "_short_path_from_message": | ||
| from shared.kafka_utils import _short_path_from_message | ||
| globals()["_short_path_from_message"] = _short_path_from_message | ||
| return _short_path_from_message |
There was a problem hiding this comment.
The sequence of if statements should be an if/elif/... chain. While functionally similar in this case, using elif is more idiomatic and efficient as it avoids unnecessary checks once a condition has been met. It also more clearly communicates that the conditions are mutually exclusive.
| if name == "kafka_producer": | |
| from shared.kafka_utils import kafka_producer | |
| globals()["kafka_producer"] = kafka_producer | |
| return kafka_producer | |
| if name == "run_consumer": | |
| from shared.kafka_consumer import run_consumer | |
| globals()["run_consumer"] = run_consumer | |
| return run_consumer | |
| if name == "ensure_purge_topic": | |
| from shared.kafka_utils import ensure_purge_topic | |
| globals()["ensure_purge_topic"] = ensure_purge_topic | |
| return ensure_purge_topic | |
| if name == "send_purge_event": | |
| from shared.kafka_utils import send_purge_event | |
| globals()["send_purge_event"] = send_purge_event | |
| return send_purge_event | |
| if name == "_short_path_from_message": | |
| from shared.kafka_utils import _short_path_from_message | |
| globals()["_short_path_from_message"] = _short_path_from_message | |
| return _short_path_from_message | |
| if name == "kafka_producer": | |
| from shared.kafka_utils import kafka_producer | |
| globals()["kafka_producer"] = kafka_producer | |
| return kafka_producer | |
| elif name == "run_consumer": | |
| from shared.kafka_consumer import run_consumer | |
| globals()["run_consumer"] = run_consumer | |
| return run_consumer | |
| elif name == "ensure_purge_topic": | |
| from shared.kafka_utils import ensure_purge_topic | |
| globals()["ensure_purge_topic"] = ensure_purge_topic | |
| return ensure_purge_topic | |
| elif name == "send_purge_event": | |
| from shared.kafka_utils import send_purge_event | |
| globals()["send_purge_event"] = send_purge_event | |
| return send_purge_event | |
| elif name == "_short_path_from_message": | |
| from shared.kafka_utils import _short_path_from_message | |
| globals()["_short_path_from_message"] = _short_path_from_message | |
| return _short_path_from_message |
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for the URL shortener service, adding a new e2e-tests service to docker-compose.yml with a dedicated Dockerfile and Python test script. The tests cover URL shortening, redirection, deletion, and health checks. Review comments suggest improving the Dockerfile by copying only necessary files to keep the image minimal, removing a redundant wait_for_services function in the test script due to docker-compose's depends_on health check, and migrating the custom test runner to pytest for better maintainability and reporting.
| WORKDIR /app | ||
| COPY e2e_tests/requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
| COPY e2e_tests/ . |
There was a problem hiding this comment.
The COPY e2e_tests/ . command copies the entire e2e_tests directory from the build context into the image. This unnecessarily includes the Dockerfile itself and overwrites requirements.txt. To keep the Docker image clean and minimal, it's best practice to copy only the specific files needed for the test execution.
COPY e2e_tests/test_e2e.py .
| def wait_for_services(max_retries=30, delay=2): | ||
| """Wait for all services to be healthy.""" | ||
| for i in range(max_retries): | ||
| try: | ||
| resp = httpx.get(f"{BASE_URL}/health", timeout=5.0) | ||
| if resp.status_code == 200: | ||
| print(f"✓ Gateway is healthy") | ||
| return True | ||
| except Exception as e: | ||
| print(f"Waiting for services... ({i+1}/{max_retries}): {e}") | ||
| time.sleep(delay) | ||
| raise RuntimeError("Services did not become healthy in time") |
There was a problem hiding this comment.
This wait_for_services function is redundant. The docker-compose.yml configuration for the e2e-tests service already uses depends_on: gateway: condition: service_healthy. This ensures the test container won't start until the gateway service is healthy, making this wait loop unnecessary. You can remove this function and its call on line 214 to simplify the code and avoid a redundant wait period.
| def run_tests(): | ||
| """Run all e2e tests.""" | ||
| print("=" * 60) | ||
| print("URL Shortener E2E Tests") | ||
| print("=" * 60) | ||
|
|
||
| # Wait for services | ||
| print("\nWaiting for services to be ready...") | ||
| wait_for_services() | ||
|
|
||
| # Run tests | ||
| all_passed = True | ||
| test_classes = [TestShorten, TestRedirect, TestDelete, TestHealth] | ||
|
|
||
| for test_class in test_classes: | ||
| print(f"\n--- {test_class.__name__} ---") | ||
| instance = test_class() | ||
| for method_name in dir(instance): | ||
| if method_name.startswith("test_"): | ||
| try: | ||
| getattr(instance, method_name)() | ||
| except AssertionError as e: | ||
| print(f"✗ {method_name}: FAILED - {e}") | ||
| all_passed = False | ||
| except Exception as e: | ||
| print(f"✗ {method_name}: ERROR - {e}") | ||
| all_passed = False | ||
|
|
||
| print("\n" + "=" * 60) | ||
| if all_passed: | ||
| print("ALL TESTS PASSED ✓") | ||
| return 0 | ||
| else: | ||
| print("SOME TESTS FAILED ✗") | ||
| return 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| exit(run_tests()) |
There was a problem hiding this comment.
This script uses a custom test runner. For better maintainability, robustness, and reporting, consider using a standard test framework like pytest. Your existing test structure is already compatible.
To migrate, you would:
- Add
pytesttoe2e_tests/requirements.txt. - Change the
CMDine2e_tests/DockerfiletoCMD ["pytest"]. - Remove the
run_tests()function and theif __name__ == "__main__":block from this file.
- Build and start services in test job - Wait for services to be healthy - Run e2e tests before deployment - Deploy only if tests pass (needs: test)
- Add e2e-tests job to tests.yml for pull requests - Uses Docker services (postgres, redis, redpanda) on ubuntu-latest - Runs after unit tests pass - Add E2E_BASE_URL env var for test flexibility
- Migrate e2e tests to pytest framework - Remove redundant wait_for_services function (docker-compose handles health checks) - Fix Dockerfile to copy only test_e2e.py - Add pytest dependency - Change if to if/elif chain in shared/__init__.py
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for the service, running in a dedicated Docker container. It also includes a fix for the gateway's healthcheck in docker-compose.yml and an optimization in the shared package to lazy-load Kafka-related components. My feedback includes a suggestion to pin test dependencies for reproducibility and a refactoring suggestion to improve the maintainability of the new lazy-loading mechanism.
| httpx>=0.28.0 | ||
| pytest>=8.0.0 |
There was a problem hiding this comment.
For more reproducible builds, it's a good practice to pin dependencies to exact versions using ==. Using >= can introduce unexpected changes or breakages if a new version of a dependency is released with a breaking change. This ensures that the test environment is consistent across all runs and developer machines.
httpx==0.28.0
pytest==8.0.0
| # Lazy imports for kafka components (avoid loading confluent_kafka in services that don't need it) | ||
| def __getattr__(name): | ||
| if name == "kafka_producer": | ||
| from shared.kafka_utils import kafka_producer | ||
| globals()["kafka_producer"] = kafka_producer | ||
| return kafka_producer | ||
| elif name == "run_consumer": | ||
| from shared.kafka_consumer import run_consumer | ||
| globals()["run_consumer"] = run_consumer | ||
| return run_consumer | ||
| elif name == "ensure_purge_topic": | ||
| from shared.kafka_utils import ensure_purge_topic | ||
| globals()["ensure_purge_topic"] = ensure_purge_topic | ||
| return ensure_purge_topic | ||
| elif name == "send_purge_event": | ||
| from shared.kafka_utils import send_purge_event | ||
| globals()["send_purge_event"] = send_purge_event | ||
| return send_purge_event | ||
| elif name == "_short_path_from_message": | ||
| from shared.kafka_utils import _short_path_from_message | ||
| globals()["_short_path_from_message"] = _short_path_from_message | ||
| return _short_path_from_message | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
This __getattr__ implementation is a great way to lazy-load dependencies. To make it more concise and easier to maintain as more lazy-loaded components are added, you could consider refactoring it to be data-driven instead of using a long if/elif chain. This would reduce code duplication and make it easier to add new lazy-loaded imports in the future.
# Lazy imports for kafka components (avoid loading confluent_kafka in services that don't need it)
_LAZY_IMPORTS = {
"kafka_producer": "shared.kafka_utils",
"run_consumer": "shared.kafka_consumer",
"ensure_purge_topic": "shared.kafka_utils",
"send_purge_event": "shared.kafka_utils",
"_short_path_from_message": "shared.kafka_utils",
}
def __getattr__(name):
if name in _LAZY_IMPORTS:
module = __import__(_LAZY_IMPORTS[name], fromlist=[name])
obj = getattr(module, name)
globals()[name] = obj
return obj
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for the service, run via a new e2e-tests Docker Compose service. It also includes a performance optimization to lazy-load Kafka-related components in the shared module, preventing unnecessary imports in services that don't use Kafka. Additionally, the gateway healthcheck in docker-compose.yml is made more robust, and Python import statements are updated to follow PEP 8 style guidelines. My review includes a suggestion to improve dependency management for the new tests to ensure reproducibility.
| httpx>=0.28.0 | ||
| pytest>=8.0.0 |
There was a problem hiding this comment.
For better reproducibility and to avoid unexpected test failures caused by upstream dependency updates, it's a best practice to pin dependencies to exact versions using == instead of >=. This ensures that your test environment is stable and consistent across all runs.
httpx==0.28.0
pytest==8.0.0
1 participant
No description provided.