Conversation
Greptile SummaryThis PR successfully implements the Quiz Generation Endpoint with comprehensive validation and testing. The implementation addresses all issues from previous review threads:
Key Implementation Details:
Architecture Note: The endpoint uses an inline prompt rather than the centralized The validation logic is duplicated from Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 5354637 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "answer" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'answer' field") | ||
| if "topic" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'topic' field") |
There was a problem hiding this comment.
Answer not validated against options
The endpoint validates that answer is present but never verifies it actually appears in the options list. If Gemini returns inconsistent data (e.g. an answer that doesn't match any option), the bad data passes through and is returned to the client — silently violating the invariant your prompt instructs the model to uphold.
| if "answer" not in q: | |
| raise ValueError(f"Quiz item {i} missing 'answer' field") | |
| if "topic" not in q: | |
| raise ValueError(f"Quiz item {i} missing 'topic' field") | |
| if "answer" not in q: | |
| raise ValueError(f"Quiz item {i} missing 'answer' field") | |
| if "topic" not in q: | |
| raise ValueError(f"Quiz item {i} missing 'topic' field") | |
| if q["answer"] not in q["options"]: | |
| raise ValueError( | |
| f"Quiz item {i} answer '{q['answer']}' not found in options" | |
| ) |
Note that test_answer_matche_an_option passes today only because the mock always returns well-formed data — there is no test case that sends a response where answer is absent from options and expects a 500.
|
|
||
| response = client.post( | ||
| "/api/v1/quiz", | ||
| json={"text": VALID_NOTES}, |
There was a problem hiding this comment.
sys.path.insert placed after the imports it is meant to enable
The sys.path.insert call on this line appears after the imports from main and services.gemini (lines 190–191) that require the backend/ directory to be on the path. Those imports would have already raised an ImportError if the path hadn't been configured beforehand. This makes the sys.path.insert effectively dead code — it only "works" because pytest is run from the backend/ directory anyway.
Move the path manipulation to the top of the file, before any project-level imports, or remove it entirely since the test suite already relies on being invoked from backend/.
| json={"text": VALID_NOTES}, | |
| import sys | |
| import os | |
| # Ensure the backend directory is on the path when running tests directly | |
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))) | |
| import pytest | |
| import json | |
| from unittest.mock import AsyncMock, patch | |
| from main import app, StudyPackRequest, MCQuiz, clean_response | |
| from services.gemini import GeminiService | |
| from fastapi.testclient import TestClient | |
| from pydantic import ValidationError |
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # TestQuizQuestionStructure | ||
| # --------------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
Incorrect filename in docstring run commands
Both example commands in the module docstring reference tests/test_quiz_unit.py, but the actual file is tests/test_generate_quiz_unit.py. A developer following these instructions would get a "no tests ran" result.
| # --------------------------------------------------------------------------- | |
| # TestQuizQuestionStructure | |
| # --------------------------------------------------------------------------- | |
| Run all tests: | |
| pytest tests/test_generate_quiz_unit.py -v | |
| Run a specific test: | |
| pytest tests/test_generate_quiz_unit.py::TestGenerateQuizEndpoint::test_endpoint_exists -v |
backend/main.py
Outdated
| if q["answer"] not in q["options"]: | ||
| raise ValueError(f"Quiz item {i} 'answer' not in 'options'") | ||
| if "topic" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'topic' field") |
There was a problem hiding this comment.
Empty topic string bypasses validation
The check on this line only verifies that a "topic" key exists in the dict, but it does not verify that the value is non-empty. A Gemini response containing "topic": "" or "topic": " " will pass this guard, get stored in MCQuiz, and be returned to the client — silently violating the invariant your prompt instructs the model to uphold.
Note that test_topic_is_non_empty does not catch this: it only verifies that the mock data (which always has valid topics) round-trips correctly. There is no negative test that sends a blank topic and expects a 500.
Add a content check immediately after the key presence check:
| raise ValueError(f"Quiz item {i} missing 'topic' field") | |
| if "topic" not in q or not q["topic"].strip(): | |
| raise ValueError(f"Quiz item {i} missing 'topic' field") |
| quiz_questions = [] | ||
| for i, q in enumerate(data.get("quiz", [])): | ||
| if not isinstance(q, dict): | ||
| raise ValueError(f"Quiz item {i} is not an object") | ||
| if "question" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'question' field") | ||
| if "options" not in q or not isinstance(q["options"], list): | ||
| raise ValueError(f"Quiz item {i} missing 'options' array") | ||
| if "answer" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'answer' field") | ||
| if q["answer"] not in q["options"]: | ||
| raise ValueError(f"Quiz item {i} 'answer' not in 'options'") | ||
| if "topic" not in q: | ||
| raise ValueError(f"Quiz item {i} missing 'topic' field") | ||
|
|
||
|
|
||
|
|
||
| quiz_questions.append(MCQuiz( | ||
| question=q["question"], | ||
| options=q["options"], | ||
| answer=q["answer"], | ||
| topic=q["topic"] | ||
| )) | ||
|
|
There was a problem hiding this comment.
Validation logic duplicated from validate_data()
The item-level validation loop here (checking isinstance(q, dict), presence of question, options, answer) replicates the logic already present in validate_data(). The only additions specific to the quiz endpoint are the answer in options guard and the topic check.
If validate_data() is ever updated (e.g., stricter type checks on option values), this endpoint would need a separate manual update.
Consider extracting a shared helper, or extending validate_data() to accept an optional extra_fields parameter, so both endpoints share the same base validation path.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # --------------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
Copy-pasted docstring is wrong
The docstring says "Test that a quiz item missing 'answer' causes a 500 mentioning the field" but this test actually verifies the answer not in options scenario, not a missing answer field. That case is already covered by test_missing_answer_field above.
| # --------------------------------------------------------------------------- | |
| def test_answer_not_in_options(self, mock_gemini, client, auth_headers): | |
| """Test that a quiz item whose answer is not one of the options causes a 500""" |
backend/main.py
Outdated
| ): | ||
| """Generate MC Quiz from user notes. Auth controlled by REQUIRE_AUTH_FOR_GENERATE.""" | ||
|
|
||
| prompt = f"""You are a study assistant. Based on the following notes, generate, 5-10 multiple choice questions where: |
There was a problem hiding this comment.
extra comma before "5-10"
| prompt = f"""You are a study assistant. Based on the following notes, generate, 5-10 multiple choice questions where: | |
| prompt = f"""You are a study assistant. Based on the following notes, generate 5-10 multiple choice questions where: |
backend/main.py
Outdated
| )) | ||
|
|
||
| if len(quiz_questions) < 5: | ||
| raise ValueError(f"Expected at least 5 quiz questions, got {len(quiz_questions)}.\n") |
There was a problem hiding this comment.
inconsistent with line 387 which has no trailing newline
| raise ValueError(f"Expected at least 5 quiz questions, got {len(quiz_questions)}.\n") | |
| raise ValueError(f"Expected at least 5 quiz questions, got {len(quiz_questions)}") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Feature(Quiz Generation Endpoint): Implement endpoint to generate 5–10 MCQ quiz questions via Gemini.
PR Summary
This PR introduces the Quiz Generation Endpoint, which generates 5–10 multiple-choice quiz questions using Gemini. The endpoint validates input, enforces structured JSON output, and stores the question, options, answer, and linked topic.
Overview
/api/v1/quizendpoint in FastAPI.MCQuizintroduced as a child ofQuizQuestionto hold a single multiple choice quiz question with a related linked topic.StudyPackRequestwas reused for the request body since quiz generation requires the user’s notes, which already include validation logic. This avoided duplicating validation code.test_generate_quiz_unit.py(mocked) to manage quota usageChecklist
Unit Test Evidence
Testing Method
Additional Notes
GEMINI_API_KEYis set in the root.envfile before running.Jira Ticket
Jira Ticket(s) - [SOC-28]