Break out GraphiQL tests #51
Conversation
WalkthroughThe changes update the frontend Dockerfile template's example port mapping, refactor the backend's GraphiQL HTML offline generation with improved error handling, and introduce a dedicated exception for missing CDN URLs. Backend tests are revised: a healthcheck test is removed, and new unit tests for GraphiQL offline rendering and error handling are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI_App
participant OfflineGraphQLRouter
participant app_def.generate_offline_graphiql_html
Client->>FastAPI_App: GET /api/graphql
FastAPI_App->>OfflineGraphQLRouter: Handle request
OfflineGraphQLRouter->>app_def.generate_offline_graphiql_html: Generate offline GraphiQL HTML
app_def.generate_offline_graphiql_html->>app_def._get_graphiql_html_base: Read base HTML
app_def.generate_offline_graphiql_html->>app_def.CdnUrlNotFoundInHtmlError: (If CDN URL missing) Raise error
app_def.generate_offline_graphiql_html-->>OfflineGraphQLRouter: Return offline HTML
OfflineGraphQLRouter-->>FastAPI_App: Render response
FastAPI_App-->>Client: Return HTML
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR extracts GraphiQL tests into their own file, enhances offline GraphiQL HTML generation, and cleans up related test and configuration files.
- Adds dedicated
test_graphiql.pywith tests for missing CDN URLs and rendering of the GraphQL route. - Refactors
generate_offline_graphiql_htmlinapp_def.py.jinja, introducing a helper, custom exception, and middleware adjustments. - Cleans up
test_healthcheck.pyby removing duplicate GraphQL tests and fixes comment in the frontend Dockerfile.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| template/backend/tests/unit/test_graphiql.py | New GraphiQL-specific unit tests |
| template/backend/tests/unit/test_healthcheck.py | Removed raw tags, deduplicated GraphQL test |
| template/backend/src/backend_api/app_def.py.jinja | Extracted HTML helper, added exception and loop logic |
| template/frontend/Dockerfile.jinja | Updated port mapping comment for Nuxt container |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
template/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja (1)
102-126: Proper error handling and middleware setup.Good practices observed:
- Wrapping initialization in try-except for better error visibility
- Correct ordering with static files mounted after route definitions
- Clean override of the GraphQL IDE rendering
Note: The permissive CORS configuration (
allow_origins=["*"]) is appropriate for the stated intranet use case but should be reconsidered if this template is used for internet-facing applications.Consider adding a more descriptive comment about the CORS configuration:
- allow_origins=["*"], # Super permissive CORS setting since this is for intranet + allow_origins=["*"], # WARNING: Permissive CORS for intranet use only. Restrict origins for internet-facing deployments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
template/frontend/{% if not deploy_as_executable %}Dockerfile{% endif %}.jinja(1 hunks)template/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja(2 hunks)template/{% if has_backend %}backend{% endif %}/tests/unit/test_healthcheck.py(3 hunks)template/{% if has_backend %}backend{% endif %}/tests/unit/{% if backend_uses_graphql %}test_graphiql.py{% endif %}(1 hunks)
🔇 Additional comments (12)
template/{% if has_backend %}backend{% endif %}/tests/unit/test_healthcheck.py (4)
1-1: LGTM: Import is correctly used.The
timeimport is needed for thetime.sleep(0.001)call in the shutdown test.
30-30: LGTM: Formatting adjustment.The assertion logic remains correct and the formatting is clean.
45-45: LGTM: Formatting adjustment.The assertion logic remains correct and the formatting is clean.
1-46: Approve refactoring: GraphQL test appropriately moved.The removal of the GraphQL test function aligns well with the PR objective to break out GraphiQL tests into a separate dedicated file. This improves test organization and maintainability by keeping healthcheck tests focused on their core responsibility.
template/{% if has_backend %}backend{% endif %}/tests/unit/{% if backend_uses_graphql %}test_graphiql.py{% endif %} (3)
1-8: LGTM! Well-organized import structure.The imports are clean, relevant, and follow standard Python conventions. The inclusion of the new
CdnUrlNotFoundInHtmlErrorexception aligns with the refactored error handling functionality.
10-18: LGTM! Well-designed error handling test.The test properly validates the new
CdnUrlNotFoundInHtmlErrorexception handling. The mocking approach, while accessing a private method, is appropriately documented and necessary for testing the error condition.Note: The test may be fragile if the internal implementation of
_get_graphiql_html_basechanges, but this appears to be an acceptable trade-off for comprehensive error testing.
21-28: LGTM! Comprehensive integration test for GraphQL route.The test effectively validates the offline GraphiQL rendering by checking:
- Successful response (200 OK)
- Presence of GraphiQL content
- Absence of external CDN URLs
This provides good coverage for the refactored GraphiQL functionality and properly validates the offline rendering behavior.
template/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja (4)
66-66: Clean implementation of the shutdown response.The return statement correctly instantiates the response model without unnecessary parameters, relying on the default values defined in the model.
69-72: Well-structured custom exception class.The
CdnUrlNotFoundInHtmlErrorfollows Python best practices with clear error messaging that includes the problematic URL for easier debugging.
74-77: Good separation of concerns for testability.Extracting the file reading logic into a separate function enables easy mocking in unit tests, which aligns with the PR's objective of improving test management.
79-99: Improve maintainability of offline GraphiQL HTML generation– Extract CDN URLs dynamically instead of hard-coding versions to avoid breakage when Strawberry bumps GraphiQL.
– Removing SRI for local files is low-risk (SRI’s main threat model is CDN tampering), but if you want defense-in-depth you could compute and inject fresh hashes for your bundled assets.
– Ensure your generated project’s/static/graphiql/directory contains all required files (see README “Obtaining the GraphiQL files to bundle” for curl commands).Suggested diff:
def generate_offline_graphiql_html() -> str: offline_graphiql_html = _get_graphiql_html_base() - cdn_urls = [ - "https://unpkg.com/[email protected]/umd/react.production.min.js", - # …rest of hard-coded URLs - ] + import re + # pull in whatever JS/CSS URLs strawberry’s HTML is using today + cdn_pattern = r'https://unpkg\.com/[^"]+\.(?:js|css)' + cdn_urls = re.findall(cdn_pattern, offline_graphiql_html) + if not cdn_urls: + raise ValueError("No GraphiQL CDN URLs found in HTML; offline generation cannot proceed") for cdn_url in cdn_urls: if cdn_url not in offline_graphiql_html: raise CdnUrlNotFoundInHtmlError(cdn_url) filename = cdn_url.split("/")[-1] local_path = f"/static/graphiql/{filename}" offline_graphiql_html = offline_graphiql_html.replace(cdn_url, local_path) return "\n".join( # remove integrity attributes (they refer to CDN hashes) [line for line in offline_graphiql_html.splitlines() if not line.strip().startswith("integrity=")] )fix_required
template/frontend/{% if not deploy_as_executable %}Dockerfile{% endif %}.jinja (1)
36-38: Review Comment Update: EXPOSE Port and Nginx Listen Directive Are ConsistentThe
EXPOSE {{ frontend_deployed_port_number }}in the Dockerfile template matches thelisten {{ frontend_deployed_port_number }}in the Nginx server block template—there is no mismatch:• template/frontend/{% if not deploy_as_executable %}Dockerfile{% endif %}.jinja (line 37):
EXPOSE {{ frontend_deployed_port_number }}• template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja (line 2):
listen {{ frontend_deployed_port_number }};No changes needed.
Likely an incorrect or invalid review comment.
| # docker run -p {% endraw %}{{ frontend_deployed_port_number }}{% raw %}:3000 my-nuxt-app | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Port mapping in the example command is likely wrong / misleading
docker run -p {{ frontend_deployed_port_number }}:3000 … assumes the container is listening on port 3000.
However, the production stage runs Nginx (which normally listens on 80) and the default.conf.template is rendered without any $FRONTEND_PORT substitution. Unless that template was also updated to listen 3000;, the container will still listen on 80, making the mapping (host:{{frontend_deployed_port_number}} → container:3000) invalid.
Recommended fix (minimum):
-# docker run -p {% endraw %}{{ frontend_deployed_port_number }}{% raw %}:3000 my-nuxt-app
+# docker run -p {% endraw %}{{ frontend_deployed_port_number }}{% raw %}:80 my-nuxt-appor (better) update default.conf.template to listen on {{ frontend_deployed_port_number }} and keep the current example.
Please verify which side you really want to expose and keep the example + EXPOSE directive consistent with the Nginx config.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the Nginx default configuration template
nginx_template=$(fd default.conf.template template/frontend || true)
if [[ -n "$nginx_template" ]]; then
echo "== Nginx template found at: $nginx_template =="
grep -R -n "listen" "$nginx_template" || echo "No 'listen' directives in $nginx_template"
else
echo "No default.conf.template found under template/frontend"
fi
# Locate and inspect the Dockerfile Jinja template for EXPOSE directives
dockerfile_template=$(fd Dockerfile.jinja template/frontend || true)
if [[ -n "$dockerfile_template" ]]; then
echo "== Dockerfile template found at: $dockerfile_template =="
grep -R -n "EXPOSE" "$dockerfile_template" || echo "No 'EXPOSE' directives in $dockerfile_template"
else
echo "No Dockerfile.jinja found under template/frontend"
fiLength of output: 896
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# List all Jinja template files under template/frontend
echo "=== .jinja files in template/frontend ==="
fd -e jinja template/frontend
# Search for EXPOSE directives in those Jinja files
echo "=== Searching for EXPOSE in Jinja templates ==="
grep -R -n "EXPOSE" template/frontend/*.jinja || echo "No 'EXPOSE' directives found"Length of output: 518
Update Docker run example to match Nginx listen port
The Nginx template (template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja, line 2) sets:
listen {{ frontend_deployed_port_number }};
so the container listens on the same {{ frontend_deployed_port_number }}, not port 3000. The example here therefore maps the wrong container port.
• File: template/frontend/{% if not deploy_as_executable %}Dockerfile{% endif %}.jinja
Lines 5–6 (example run command)
• File: template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
Line 2 (listen {{ frontend_deployed_port_number }};)
Recommended changes:
-# docker run -p {{ frontend_deployed_port_number }}:3000 my-nuxt-app
+# docker run -p {{ frontend_deployed_port_number }}:{{ frontend_deployed_port_number }} my-nuxt-appAdditionally, ensure your Dockerfile template exposes that port:
EXPOSE {{ frontend_deployed_port_number }}This keeps the EXPOSE, Nginx listen, and docker run example all consistent.
🤖 Prompt for AI Agents
In template/frontend/{% if not deploy_as_executable %}Dockerfile{% endif
%}.jinja around lines 5 to 6, the docker run example incorrectly maps the
container port 3000 instead of {{ frontend_deployed_port_number }} which is the
port Nginx listens on as set in template/frontend/{% if not deploy_as_executable
%}default.conf.template{% endif %}.jinja line 2. Update the docker run command
to map {{ frontend_deployed_port_number }} to {{ frontend_deployed_port_number
}} (e.g., -p {{ frontend_deployed_port_number }}:{{
frontend_deployed_port_number }}). Also, ensure the Dockerfile template includes
an EXPOSE directive for {{ frontend_deployed_port_number }} to keep port
configuration consistent.
Why is this change necessary?
Add some more graphiql tests and break them into separate file for easier management
How does this change address the issue?
Does it
What side effects does this change have?
None
How is this change tested?
Downstream repo
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation