From 6de59697d816ab113edc999404251384a45d0307 Mon Sep 17 00:00:00 2001 From: CuriosityQuantified Date: Sun, 9 Aug 2026 17:59:29 -0400 Subject: [PATCH] =?UTF-8?q?fix=20#22:=20[13]=20Tests=20mock=20the=20wrong?= =?UTF-8?q?=20code=20path=20=E2=80=94=20assertions=20never=20fail=20even?= =?UTF-8?q?=20when=20code=20is=20broken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_create_idea_final_turn_conclusion_parsing (and its fallback companion) patched service.groq_client and asserted on groq_client.chat.completions.create, but create_idea never touches the Groq SDK — it drives LangChain's LLMChain.arun (a ChatGroq instance). The patch intercepted nothing, so the assertions could never fail even if the real code broke. The tests also asserted the stale model id "qwen-qwq-32b" and treated create_idea's single-dict return as a list. - Patch langchain.chains.LLMChain.arun with AsyncMock so the tests exercise the real code path; a mutation of models_to_try now fails the test, proving the assertions bite. - Remove the dead groq_client mocks/assertions. - Update model-name expectations to the current "moonshotai/kimi-k2-instruct". - Handle create_idea's single-dict return correctly (not a list). - Remove the dead nested duplicate test_create_idea_final_turn_fallback_parsing defined inside test_create_idea_live_groq_call (never collected). - CI: run the two now-fixed tests (remove their deselects), add a named "Run LLM service final-turn parsing regression" step, and fix the mislabeled deselect comment. Closes #22 Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 12 ++-- tests/unit/test_llm_service.py | 116 ++++++--------------------------- 2 files changed, 28 insertions(+), 100 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86df72d..a305edc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,11 +33,15 @@ jobs: run: pytest tests/unit/test_requirements_alignment.py -q - name: Run uvicorn reload gating regression run: pytest tests/unit/test_uvicorn_reload_regression.py -q + - name: Run LLM service final-turn parsing regression + run: pytest tests/unit/test_llm_service.py::test_create_idea_final_turn_conclusion_parsing tests/unit/test_llm_service.py::test_create_idea_final_turn_fallback_parsing -q - name: Run unit tests # Deselect known-broken tests that are tracked by their own open issues - # (leave them for those issue-workers; don't scope-creep or race them): - # - test_cloudflare_r2_service upload/get_url/error -> #22 - # - test_llm_service final-turn parsing (LLMChain) -> #24 + # (leave them for those issue-workers; don't scope-creep or race them). + # The llm_service final-turn parsing tests were fixed by #22 (they had mocked + # the wrong code path -- service.groq_client instead of LLMChain.arun) and now + # run in the general suite plus the named regression step above. + # - test_cloudflare_r2_service upload/get_url/error -> its own R2 issue # - test_media_utils::test_ensure_media_directories -> #13 (env/media dir) run: pytest tests/unit/ -q --deselect tests/unit/test_cloudflare_r2_service.py::test_upload_video_public @@ -45,8 +49,6 @@ jobs: --deselect tests/unit/test_cloudflare_r2_service.py::test_upload_audio_public --deselect tests/unit/test_cloudflare_r2_service.py::test_get_file_url_private --deselect tests/unit/test_cloudflare_r2_service.py::test_upload_video_error - --deselect tests/unit/test_llm_service.py::test_create_idea_final_turn_conclusion_parsing - --deselect tests/unit/test_llm_service.py::test_create_idea_final_turn_fallback_parsing --deselect tests/unit/test_media_utils.py::TestMediaUtils::test_ensure_media_directories regressions: diff --git a/tests/unit/test_llm_service.py b/tests/unit/test_llm_service.py index a328052..1578b15 100644 --- a/tests/unit/test_llm_service.py +++ b/tests/unit/test_llm_service.py @@ -29,8 +29,8 @@ def llm_service(): @pytest.mark.asyncio async def test_create_idea_final_turn_conclusion_parsing(): """ - Tests that create_idea correctly parses a slightly messy JSON output - for the final turn conclusion, using the specified 'qwen-qwq-32b' model. + Tests that create_idea correctly parses a slightly messy JSON output + for the final turn conclusion, using the specified 'moonshotai/kimi-k2-instruct' model. """ service = LLMService(api_key="test_api_key", huggingface_service=MockHuggingFaceService(api_key="fake_hf_key")) service.log_callback = AsyncMock() # Mock the log_callback @@ -44,9 +44,8 @@ async def test_create_idea_final_turn_conclusion_parsing(): "max_turns": 6 } - # Expected output structure for a final conclusion - # Based on FINAL_TURN_TEMPLATE, it should be a single object, not a list in the example - # The _parse_json_scenarios now wraps single dicts in a list. + # Expected output structure for a final conclusion. + # create_idea returns a single dict for the conclusion (final) turn. mock_llm_output_json = { "situation_description": "The world is saved, with a lingering scent of existential absurdity!", "rationale": "User displayed excellent problem-solving skills under bizarre pressure.", @@ -62,35 +61,14 @@ async def test_create_idea_final_turn_conclusion_parsing(): ``` """ - # Mock the Groq client's chat.completions.create method - # This is the primary method used for 'qwen-qwq-32b' as per current llm_service logic for final turn - mock_groq_completion = MagicMock() - mock_groq_completion.choices = [MagicMock()] - mock_groq_completion.choices[0].message.content = messy_llm_response_str - - # The Groq client itself - mock_groq_client = MagicMock() - mock_groq_client.chat.completions.create = AsyncMock(return_value=mock_groq_completion) # Use AsyncMock for async context - - # Patch the Groq client instance within the service or where it's instantiated. - # Assuming self.groq_client is used: - with patch.object(service, 'groq_client', mock_groq_client): - generated_scenario_list = await service.create_idea(final_turn_context) - - # Assertions - assert generated_scenario_list is not None, "create_idea should return a list of scenarios" - assert len(generated_scenario_list) == 1, "Expected one scenario for the conclusion" - - generated_scenario = generated_scenario_list[0] - - # Check that the correct model was intended to be used (qwen-qwq-32b for final turn) - # The actual call is mocked, but we check if the logic path for qwen-qwq-32b would be taken. - # We can infer this if groq_client.chat.completions.create was called. - service.groq_client.chat.completions.create.assert_called_once() - call_args = service.groq_client.chat.completions.create.call_args - assert call_args is not None, "Groq client was not called as expected" - assert call_args.kwargs.get('model') == "qwen-qwq-32b", "Incorrect model specified for final turn" + # Patch the actual code path: create_idea drives the LLM via LangChain's + # LLMChain.arun (with a ChatGroq instance), NOT service.groq_client. + with patch("langchain.chains.LLMChain.arun", new_callable=AsyncMock, return_value=messy_llm_response_str): + generated_scenario = await service.create_idea(final_turn_context) + # Assertions -- create_idea returns a single dict for the conclusion turn. + assert generated_scenario is not None, "create_idea should return a scenario dict" + assert isinstance(generated_scenario, dict), "create_idea should return a dictionary for the conclusion" # Check that the parsed scenario contains the expected fields and values assert generated_scenario['situation_description'] == mock_llm_output_json['situation_description'] @@ -101,7 +79,7 @@ async def test_create_idea_final_turn_conclusion_parsing(): # Ensure no extra fields like user_role or user_prompt are present for conclusion assert 'user_role' not in generated_scenario assert 'user_prompt' not in generated_scenario - + # Check that an ID was added during validation (even if not in LLM output) assert 'id' in generated_scenario assert generated_scenario['id'] == f"scenario_{final_turn_context['current_turn_number']}_1" @@ -110,14 +88,14 @@ async def test_create_idea_final_turn_conclusion_parsing(): service.log_callback.assert_called_once() log_args = service.log_callback.call_args[0] # Get positional arguments of the call assert log_args[1].operation_name == "create_idea" - assert log_args[1].model_name == "qwen-qwq-32b" + assert log_args[1].model_name == "moonshotai/kimi-k2-instruct" # Further checks on log_args[1].prompt and log_args[1].completion can be added @pytest.mark.asyncio async def test_create_idea_final_turn_fallback_parsing(): """ - Tests that create_idea correctly parses a final turn conclusion + Tests that create_idea correctly parses a final turn conclusion when the JSON is not perfectly clean and requires the find '{' '}' fallback. """ service = LLMService(api_key="test_api_key", huggingface_service=MockHuggingFaceService(api_key="fake_hf_key")) @@ -136,19 +114,12 @@ async def test_create_idea_final_turn_fallback_parsing(): # Simulate LLM response with leading garbage and no markdown, forcing find {' '}' messy_llm_response_str = f"Some unexpected text before the JSON... \n {json.dumps(mock_llm_output_json)} \n ...and some after." - mock_groq_completion = MagicMock() - mock_groq_completion.choices = [MagicMock()] - mock_groq_completion.choices[0].message.content = messy_llm_response_str - - mock_groq_client = MagicMock() - mock_groq_client.chat.completions.create = AsyncMock(return_value=mock_groq_completion) - - with patch.object(service, 'groq_client', mock_groq_client): - generated_scenario_list = await service.create_idea(final_turn_context) + # Patch the actual code path: LLMChain.arun drives the LLM, not service.groq_client. + with patch("langchain.chains.LLMChain.arun", new_callable=AsyncMock, return_value=messy_llm_response_str): + generated_scenario = await service.create_idea(final_turn_context) - assert generated_scenario_list is not None - assert len(generated_scenario_list) == 1 - generated_scenario = generated_scenario_list[0] + assert generated_scenario is not None + assert isinstance(generated_scenario, dict) assert generated_scenario['situation_description'] == mock_llm_output_json['situation_description'] assert generated_scenario['grade'] == mock_llm_output_json['grade'] @@ -156,7 +127,8 @@ async def test_create_idea_final_turn_fallback_parsing(): service.log_callback.assert_called_once() log_args = service.log_callback.call_args[0] - assert log_args[1].model_name == "qwen-qwq-32b" + assert log_args[1].operation_name == "create_idea" + assert log_args[1].model_name == "moonshotai/kimi-k2-instruct" @pytest.mark.asyncio @@ -211,49 +183,3 @@ async def test_create_idea_live_groq_call(): # For now, just check that a model_name is logged. assert log_args[1].model_name is not None and len(log_args[1].model_name) > 0, "Model name not logged" assert log_args[1].completion is not None # Ensure some completion was logged - - - # The following test is for final turn fallback parsing, ensure it's correctly placed - @pytest.mark.asyncio - async def test_create_idea_final_turn_fallback_parsing(): - """ - Tests that create_idea correctly parses a final turn conclusion - when the JSON is not perfectly clean and requires the find '{' '}' fallback. - """ - service = LLMService(api_key="test_api_key", huggingface_service=MockHuggingFaceService(api_key="fake_hf_key")) - service.log_callback = AsyncMock() - - final_turn_context = { - "simulation_history": "History...", "current_turn_number": 6, "max_turns": 6, - "previous_turn_number": 5, "user_prompt_for_this_turn": "Final response" - } - mock_llm_output_json = { - "situation_description": "Fallback test success.", - "rationale": "Fallback rationale.", - "grade": 75, - "grade_explanation": "Fallback explanation." - } - # Simulate LLM response with leading garbage and no markdown, forcing find {' '}' - messy_llm_response_str = f"Some unexpected text before the JSON... \n {json.dumps(mock_llm_output_json)} \n ...and some after." - - mock_groq_completion = MagicMock() - mock_groq_completion.choices = [MagicMock()] - mock_groq_completion.choices[0].message.content = messy_llm_response_str - - mock_groq_client = MagicMock() - mock_groq_client.chat.completions.create = AsyncMock(return_value=mock_groq_completion) - - with patch.object(service, 'groq_client', mock_groq_client): - generated_scenario_list = await service.create_idea(final_turn_context) - - assert generated_scenario_list is not None - assert len(generated_scenario_list) == 1 - generated_scenario = generated_scenario_list[0] - - assert generated_scenario['situation_description'] == mock_llm_output_json['situation_description'] - assert generated_scenario['grade'] == mock_llm_output_json['grade'] - assert 'id' in generated_scenario - - service.log_callback.assert_called_once() - log_args = service.log_callback.call_args[0] - assert log_args[1].model_name == "qwen-qwq-32b" \ No newline at end of file