fix: correct test assertions for ProjectCodeGenerator return structure#101
fix: correct test assertions for ProjectCodeGenerator return structure#101groupthinking merged 6 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add BuildPlan Pydantic model with structured output schema - Add extract_build_plan() to GeminiVideoService with JSON schema support - Update RealVideoProcessor to extract BuildPlan from video analysis - Modify ProjectCodeGenerator to consume BuildPlan with fallback to legacy logic - BuildPlan includes ordered steps with dependencies, action types, and metadata Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
- Add unit tests for BuildPlan, BuildStep, and schema generation - Add integration tests for BuildPlan extraction and code generation - Test structured output validation and fallback logic - All tests validate Stage 2 semantic parsing implementation Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements Stage 2 “Semantic Logic Parsing” by introducing a structured BuildPlan artifact extracted from Gemini and wiring it into the video-processing + code-generation pipeline, with unit/integration tests.
Changes:
- Added
BuildPlan/BuildStepPydantic models plus a Gemini JSON schema helper. - Added
GeminiVideoService.extract_build_plan()and invoked it fromRealVideoProcessor, passingbuild_planthrough toProjectCodeGenerator. - Added unit + integration tests for BuildPlan validation and BuildPlan→generation-context behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_build_plan.py | New unit tests for BuildPlan models/enums and schema generation. |
| tests/integration/test_build_plan_integration.py | New integration tests for BuildPlan usage in code generation + context extraction. |
| src/youtube_extension/backend/services/real_video_processor.py | Calls Gemini to extract BuildPlan during processing and attaches it to results. |
| src/youtube_extension/backend/models/build_plan.py | New BuildPlan/BuildStep models and Gemini response schema builder. |
| src/youtube_extension/backend/code_generator.py | Prefers build_plan when building generation context; falls back to legacy extraction. |
| src/integration/gemini_video.py | Adds extract_build_plan() with responseSchema JSON-mode generation config. |
| src/youtube_extension.egg-info/top_level.txt | Updates generated packaging metadata (should not be committed). |
| src/youtube_extension.egg-info/SOURCES.txt | Updates generated packaging metadata (should not be committed). |
| build_plan = await gemini_service.extract_build_plan(video_url) | ||
| await gemini_service.close() |
There was a problem hiding this comment.
If extract_build_plan() raises, gemini_service.close() is skipped, leaving the underlying httpx.AsyncClient unclosed. Wrap the call in a try/finally (or use an async context manager) so close() is always awaited, including on failure.
| build_plan = await gemini_service.extract_build_plan(video_url) | |
| await gemini_service.close() | |
| try: | |
| build_plan = await gemini_service.extract_build_plan(video_url) | |
| finally: | |
| await gemini_service.close() |
| # Verify result structure | ||
| assert result["success"] is True | ||
| assert "project_path" in result | ||
|
|
There was a problem hiding this comment.
ProjectCodeGenerator.generate_project() does not include a success key in its return dict (it returns framework/entry_point/etc.), so assert result["success"] is True will raise KeyError. Update the test to assert against fields that are actually returned (e.g., project_path, framework, or generated file contents).
| # Verify BuildPlan was used | ||
| assert "Todo App with React and Tailwind" in result.get("title", "") | ||
|
|
There was a problem hiding this comment.
ProjectCodeGenerator.generate_project() does not return a title, so assert "Todo App..." in result.get("title", "") will always fail. If you want to verify BuildPlan usage, assert via README.md/App.js contents under result["project_path"] or by checking video_analysis["extracted_info"] after context building.
| generator = ProjectCodeGenerator() | ||
| project_config = {"type": "react"} | ||
|
|
||
| # Generate project | ||
| result = await generator.generate_project( | ||
| video_analysis_with_build_plan, project_config | ||
| ) | ||
|
|
There was a problem hiding this comment.
This test calls generate_project(), which creates a real temp directory via tempfile.mkdtemp() and the test never deletes it. Consider monkeypatching tempfile.mkdtemp to use tmp_path (as tests/test_code_generator.py does) or cleaning up result["project_path"] with shutil.rmtree to avoid leaking temp dirs during CI runs.
| # Verify result structure | ||
| assert result["success"] is True | ||
| assert "project_path" in result | ||
|
|
There was a problem hiding this comment.
Same issue here: generate_project() does not return a success key, so this assertion will fail with KeyError. Align the assertions with the actual return shape.
| def test_build_plan_requires_minimum_steps(self): | ||
| """Test that BuildPlan requires at least one step.""" | ||
| with pytest.raises(Exception): | ||
| BuildPlan( | ||
| title="Test", | ||
| description="Test", | ||
| project_type=ProjectType.WEB, | ||
| technologies=["javascript"], | ||
| steps=[], # Empty steps should fail | ||
| ) |
There was a problem hiding this comment.
These validation tests use pytest.raises(Exception), which can mask unrelated failures and reduce test signal. Since this is Pydantic validation, assert pytest.raises(ValidationError) (as the unit tests do) so the test only passes for the intended failure mode.
| def test_build_plan_requires_technologies(self): | ||
| """Test that BuildPlan requires at least one technology.""" | ||
| with pytest.raises(Exception): | ||
| BuildPlan( | ||
| title="Test", | ||
| description="Test", | ||
| project_type=ProjectType.WEB, | ||
| technologies=[], # Empty technologies should fail | ||
| steps=[ |
There was a problem hiding this comment.
Same here: catching a broad Exception makes the test pass even if something other than Pydantic validation breaks. Prefer pytest.raises(ValidationError) to keep the assertion precise.
| "project_type": { | ||
| "type": "string", | ||
| "enum": ["web", "react", "nextjs", "api", "mobile", "desktop", "agent"], | ||
| "description": "Type of project to generate" | ||
| }, | ||
| "difficulty_level": { | ||
| "type": "string", | ||
| "enum": ["beginner", "intermediate", "advanced"], | ||
| "description": "Complexity level of the project" |
There was a problem hiding this comment.
build_plan_to_gemini_schema() hard-codes the enum values for project_type, difficulty_level, and step action. This can drift from ProjectType/DifficultyLevel/ActionType over time; derive these enum lists from the actual Enum classes to keep the schema and model consistent automatically.
| backend | ||
| connectors | ||
| core | ||
| dataconnect-generated | ||
| integration |
There was a problem hiding this comment.
src/youtube_extension.egg-info/* are build artifacts and are already ignored by .gitignore (*.egg-info/). These diffs indicate regenerated packaging metadata got committed; please remove these files from the PR (and ideally from version control) to avoid churn and accidental inclusion of local environment state.
| src/uvai/api/main.py | ||
| src/uvai/api/server.py | ||
| src/uvai/api/v1/services/issue_tracker.py | ||
| src/uvai/ml/__init__.py | ||
| src/uvai/ml/serve.py | ||
| src/uvai/ml/models/__init__.py | ||
| src/uvai/ml/models/action_priority_ranker.py | ||
| src/uvai/ml/models/transcript_quality_scorer.py | ||
| src/uvai/security_protocol/__init__.py |
There was a problem hiding this comment.
This SOURCES.txt change is part of the generated *.egg-info metadata (which is .gitignored). It should not be updated in a feature PR; please revert/remove src/youtube_extension.egg-info changes to prevent noisy diffs and inconsistent packaging manifests.
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> Agent-Logs-Url: https://github.com/groupthinking/EventRelay/sessions/175766a0-007b-43c9-81ad-b1f1d573718e
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> Agent-Logs-Url: https://github.com/groupthinking/EventRelay/sessions/175766a0-007b-43c9-81ad-b1f1d573718e
🔍 PR Validation |
| """ | ||
|
|
||
| # Determine if YouTube URL | ||
| is_youtube = "youtube.com" in video_url or "youtu.be" in video_url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the fix is to stop checking for "youtube.com" and "youtu.be" as arbitrary substrings of the full URL string, and instead parse the URL and validate its hostname against an explicit allowlist of hosts (and potentially handle missing schemes robustly). This avoids accidental matches where youtube.com appears in the path, query, or as part of another domain.
Concretely, in GeminiVideoService near line 431, replace:
is_youtube = "youtube.com" in video_url or "youtu.be" in video_urlwith logic that:
- Parses
video_urlusingurllib.parse.urlparse. - Handles URLs without scheme by prepending
https://before parsing again. - Extracts
hostnameand normalizes it (lowercase). - Checks whether the hostname is in a small allowlist such as
{"youtube.com", "www.youtube.com", "m.youtube.com", "youtu.be", "www.youtu.be"}or ends with.youtube.com.
We need to add an import for urlparse (from urllib.parse) at the top of src/integration/gemini_video.py. The rest of the method’s behavior stays the same: if the parsed hostname is a YouTube host, is_youtube is True and the existing request payload for YouTube URLs is used; otherwise, the non-YouTube path runs as before.
| @@ -18,6 +18,7 @@ | ||
| from typing import Any, Literal, Optional, cast | ||
|
|
||
| import httpx | ||
| from urllib.parse import urlparse | ||
|
|
||
|
|
||
| @dataclass | ||
| @@ -429,7 +430,19 @@ | ||
| """ | ||
|
|
||
| # Determine if YouTube URL | ||
| is_youtube = "youtube.com" in video_url or "youtu.be" in video_url | ||
| parsed = urlparse(video_url) | ||
| # Handle URLs that may be missing a scheme (e.g., "youtube.com/...") | ||
| if not parsed.scheme: | ||
| parsed = urlparse("https://" + video_url) | ||
| host = (parsed.hostname or "").lower() | ||
| youtube_hosts = { | ||
| "youtube.com", | ||
| "www.youtube.com", | ||
| "m.youtube.com", | ||
| "youtu.be", | ||
| "www.youtu.be", | ||
| } | ||
| is_youtube = host in youtube_hosts or host.endswith(".youtube.com") | ||
|
|
||
| if is_youtube: | ||
| payload = { |
| ) | ||
|
|
||
| assert plan.video_metadata["video_id"] == "test123" | ||
| assert "youtube.com" in plan.video_metadata["source_url"] |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Copilot Autofix
AI 1 day ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| # Parse JSON response | ||
| try: | ||
| import json |
Check notice
Code scanning / CodeQL
Module is imported more than once Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix "Module is imported more than once" issues, remove duplicate imports and rely on a single, consistent import for each module. Keep imports at the top of the file unless there is a strong, explicit reason to import within a function (e.g., heavy optional dependencies).
For this file, json is already imported at line 15. Inside the method that parses the JSON response (the try block around lines 481–487), there is a second import json. The best fix is to remove this inner import and directly use the already-imported json module. Concretely, in src/integration/gemini_video.py, within the try: block starting around line 481, delete the import json line and leave the build_plan = json.loads(text) and except json.JSONDecodeError lines unchanged. No new imports, methods, or definitions are needed.
| @@ -480,8 +480,6 @@ | ||
|
|
||
| # Parse JSON response | ||
| try: | ||
| import json | ||
|
|
||
| build_plan = json.loads(text) | ||
| return build_plan | ||
| except json.JSONDecodeError as e: |
| or metadata.get("title") | ||
| or metadata.get("video_title") | ||
| or "UVAI Generated Project" | ||
| ) |
Check notice
Code scanning / CodeQL
Unused local variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix an unused local variable, either (a) remove the assignment if the right-hand side has no necessary side effects, or (b) if the variable is intentionally unused, rename it to something like _ or unused_ai_analysis to make that intention explicit.
Here, the variable is assigned as ai_analysis = video_analysis.get("ai_analysis") or {} inside the fallback else block, and there is no subsequent use of ai_analysis in the provided code. The right-hand side is a pure dictionary lookup with a simple or {} default and has no side effects. Therefore, the best fix that preserves behavior is to delete this line entirely. We do not need to keep the expression, and we do not need to introduce an “unused” name, because there is no documentation value in binding it.
Concretely: in src/youtube_extension/backend/code_generator.py, inside _build_generation_context, within the else fallback block that logs “BuildPlan not found, falling back to legacy extraction”, remove the line that assigns ai_analysis. No additional imports, methods, or definitions are needed.
| @@ -1250,7 +1250,6 @@ | ||
|
|
||
| extracted_info = dict(video_analysis.get("extracted_info") or {}) | ||
| metadata = video_analysis.get("metadata") or video_analysis.get("video_data") or {} | ||
| ai_analysis = video_analysis.get("ai_analysis") or {} | ||
|
|
||
| title = ( | ||
| project_config.get("title") |
Integration tests were asserting on a non-existent
successkey in theProjectCodeGenerator.generate_project()return value. The method actually returns a dict withproject_path,project_type,technologies, andfeatures.Changes
project_path,project_type) instead of checking forsuccesskeypytest.raises(Exception)topytest.raises(ValidationError)for proper Pydantic validation testingVerified Behavior
All 32 tests now pass (19 unit + 13 integration).
Original prompt