diff --git a/tests/test_evaluate.py b/tests/test_evaluate.py index 755430c..d22481b 100644 --- a/tests/test_evaluate.py +++ b/tests/test_evaluate.py @@ -8,9 +8,13 @@ from __future__ import annotations +import json +import tempfile +from pathlib import Path + import pytest -from vaultrag.evaluate import CaseResult, EvalReport, GoldCase, diff, run_eval +from vaultrag.evaluate import CaseResult, EvalReport, GoldCase, diff, load_gold, run_eval from vaultrag.generate import FakeLLM _ANSWERS = FakeLLM('{"answer": "The engineering bonus is 10% of base.", "cited": [1], "conflict": false}') @@ -167,3 +171,202 @@ def test_diff_reports_a_fix(): def test_diff_reports_no_change_when_nothing_moved(): r = [CaseResult("c1", "alice", recall=1.0)] assert diff(_report("v1", r), _report("v2", list(r)))["verdict"] == "NO CHANGE" + + +# ----------------------------------------------------------------- gold file parsing & validation + + +def _write_temp_gold(content: str | dict) -> str: + with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False, encoding="utf-8") as f: + if isinstance(content, str): + f.write(content) + else: + json.dump(content, f) + return f.name + + +def test_load_gold_invalid_json_syntax(): + tmp_path = _write_temp_gold("{ invalid json: ") + try: + with pytest.raises(ValueError, match="Invalid JSON"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_missing_cases_key(): + tmp_path = _write_temp_gold({"not_cases": []}) + try: + with pytest.raises(ValueError, match="missing required 'cases' key"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_cases_not_a_list(): + tmp_path = _write_temp_gold({"cases": "not a list"}) + try: + with pytest.raises(ValueError, match="'cases' must be a list"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_root_not_dict(): + tmp_path = _write_temp_gold(["not", "a", "dict"]) + try: + with pytest.raises(ValueError, match="root must be a JSON object"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_case_not_a_dict(): + tmp_path = _write_temp_gold({"cases": ["not-a-dict"]}) + try: + with pytest.raises(ValueError, match="must be a dictionary"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_case_missing_required_field(): + # missing id + tmp_path = _write_temp_gold( + { + "cases": [ + { + "user_id": "alice", + "question": "what is the policy?", + } + ] + } + ) + try: + with pytest.raises(ValueError, match="Missing required field 'id'"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) + + # missing user_id + tmp_path2 = _write_temp_gold( + { + "cases": [ + { + "id": "c1", + "question": "what is the policy?", + } + ] + } + ) + try: + with pytest.raises(ValueError, match="Missing required field 'user_id'"): + load_gold(tmp_path2) + finally: + Path(tmp_path2).unlink(missing_ok=True) + + # missing question + tmp_path3 = _write_temp_gold( + { + "cases": [ + { + "id": "c1", + "user_id": "alice", + } + ] + } + ) + try: + with pytest.raises(ValueError, match="Missing required field 'question'"): + load_gold(tmp_path3) + finally: + Path(tmp_path3).unlink(missing_ok=True) + + +def test_load_gold_invalid_optional_field_types(): + # expected_docs not a list + tmp1 = _write_temp_gold( + {"cases": [{"id": "c1", "user_id": "u1", "question": "q", "expected_docs": "doc1"}]} + ) + try: + with pytest.raises(ValueError, match="Field 'expected_docs' must be a list"): + load_gold(tmp1) + finally: + Path(tmp1).unlink(missing_ok=True) + + # forbidden_docs not a list + tmp2 = _write_temp_gold( + {"cases": [{"id": "c1", "user_id": "u1", "question": "q", "forbidden_docs": 123}]} + ) + try: + with pytest.raises(ValueError, match="Field 'forbidden_docs' must be a list"): + load_gold(tmp2) + finally: + Path(tmp2).unlink(missing_ok=True) + + # should_answer not a bool + tmp3 = _write_temp_gold( + {"cases": [{"id": "c1", "user_id": "u1", "question": "q", "should_answer": "yes"}]} + ) + try: + with pytest.raises(ValueError, match="Field 'should_answer' must be a boolean"): + load_gold(tmp3) + finally: + Path(tmp3).unlink(missing_ok=True) + + +def test_load_gold_valid_file(): + valid_data = { + "cases": [ + { + "id": "c1", + "user_id": "alice", + "question": "what is the policy?", + "expected_docs": ["doc1"], + "forbidden_docs": ["doc2"], + "should_answer": True, + }, + { + "id": "c2", + "user_id": "bob", + "question": "where is the office?", + }, + ] + } + tmp_path = _write_temp_gold(valid_data) + try: + cases = load_gold(tmp_path) + assert len(cases) == 2 + assert isinstance(cases[0], GoldCase) + assert cases[0].id == "c1" + assert cases[0].user_id == "alice" + assert cases[0].question == "what is the policy?" + assert cases[0].expected_docs == ["doc1"] + assert cases[0].forbidden_docs == ["doc2"] + assert cases[0].should_answer is True + + assert isinstance(cases[1], GoldCase) + assert cases[1].id == "c2" + assert cases[1].user_id == "bob" + assert cases[1].question == "where is the office?" + assert cases[1].expected_docs == [] + assert cases[1].forbidden_docs == [] + assert cases[1].should_answer is True + finally: + Path(tmp_path).unlink(missing_ok=True) + + +def test_load_gold_duplicate_case_ids(): + tmp_path = _write_temp_gold( + { + "cases": [ + {"id": "duplicate-case-id", "user_id": "alice", "question": "q1"}, + {"id": "duplicate-case-id", "user_id": "bob", "question": "q2"}, + ] + } + ) + try: + with pytest.raises(ValueError, match="duplicate-case-id"): + load_gold(tmp_path) + finally: + Path(tmp_path).unlink(missing_ok=True) diff --git a/vaultrag/evaluate.py b/vaultrag/evaluate.py index 693d067..adb6029 100644 --- a/vaultrag/evaluate.py +++ b/vaultrag/evaluate.py @@ -135,7 +135,79 @@ def to_json(self) -> str: def load_gold(path: str | Path) -> list[GoldCase]: - data = json.loads(Path(path).read_text()) + path_obj = Path(path) + try: + raw = path_obj.read_text(encoding="utf-8") + except Exception as e: + raise ValueError(f"Failed to read gold file '{path}': {e}") from e + + try: + data = json.loads(raw) + except json.JSONDecodeError as e: + raise ValueError(f"Invalid JSON in gold file '{path}': {e}") from e + + if not isinstance(data, dict): + raise ValueError( + f"Invalid gold file format in '{path}': root must be a JSON object, got {type(data).__name__}" + ) + + if "cases" not in data: + raise ValueError(f"Invalid gold file format in '{path}': missing required 'cases' key") + + if not isinstance(data["cases"], list): + raise ValueError( + f"Invalid gold file format in '{path}': 'cases' must be a list, got {type(data['cases']).__name__}" + ) + + required_fields = ("id", "user_id", "question") + seen_ids: set[str] = set() + + for i, case in enumerate(data["cases"]): + ctx = f"case index {i}" + if not isinstance(case, dict): + raise ValueError( + f"Invalid case at index {i} in '{path}': case must be a dictionary, got {type(case).__name__}" + ) + + if "id" in case and isinstance(case["id"], str): + ctx = f"case '{case['id']}' (index {i})" + + for req_field in required_fields: + if req_field not in case: + raise ValueError(f"Missing required field '{req_field}' in {ctx} in '{path}'") + if not isinstance(case[req_field], str): + raise ValueError( + f"Field '{req_field}' must be a string in {ctx} in '{path}', got {type(case[req_field]).__name__}" + ) + + case_id = case["id"] + if case_id in seen_ids: + raise ValueError( + f"Duplicate case id '{case_id}' in {ctx} in '{path}'" + ) + seen_ids.add(case_id) + + if "expected_docs" in case and ( + not isinstance(case["expected_docs"], list) + or not all(isinstance(d, str) for d in case["expected_docs"]) + ): + raise ValueError( + f"Field 'expected_docs' must be a list of strings in {ctx} in '{path}'" + ) + + if "forbidden_docs" in case and ( + not isinstance(case["forbidden_docs"], list) + or not all(isinstance(d, str) for d in case["forbidden_docs"]) + ): + raise ValueError( + f"Field 'forbidden_docs' must be a list of strings in {ctx} in '{path}'" + ) + + if "should_answer" in case and not isinstance(case["should_answer"], bool): + raise ValueError( + f"Field 'should_answer' must be a boolean in {ctx} in '{path}', got {type(case['should_answer']).__name__}" + ) + return [GoldCase(**c) for c in data["cases"]]