chore: consolidate test_data models#131
Conversation
WalkthroughConsolidates and expands unit tests for core data models: merges removed tests into a single module, adds Validation tests for new public models Changes
Sequence Diagram(s)(omitted — changes are test and model validations without multi-component runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/core/models/test_data.py (1)
273-286: Consider adding a positive test case forEvaluationData.The class only tests rejection scenarios. Adding a test for successful creation with valid data would improve coverage and serve as documentation for expected usage.
💡 Suggested addition
def test_valid_evaluation_data(self): """Test EvaluationData with valid fields.""" turn = TurnData(turn_id="turn1", query="Query") data = EvaluationData(conversation_group_id="conv1", turns=[turn]) assert data.conversation_group_id == "conv1" assert len(data.turns) == 1 assert data.turns[0].turn_id == "turn1"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/core/models/test_data.pytests/unit/core/models/test_data_additional.py
💤 Files with no reviewable changes (1)
- tests/unit/core/models/test_data_additional.py
🧰 Additional context used
📓 Path-based instructions (2)
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest mocking (mocker fixture) instead of unittest.mock
Files:
tests/unit/core/models/test_data.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test structure to source code organization, with test files named test_.py and test functions/classes named test_ and Test*
Files:
tests/unit/core/models/test_data.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: src/lightspeed_evaluation/core/output/generator.py:140-145
Timestamp: 2025-09-11T12:47:06.747Z
Learning: User asamal4 prefers that non-critical comments are sent when actual code changes are pushed, not on unrelated commits.
📚 Learning: 2025-10-31T11:54:59.126Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 90
File: src/lightspeed_evaluation/core/models/data.py:198-208
Timestamp: 2025-10-31T11:54:59.126Z
Learning: In the lightspeed_evaluation framework, the expected_tool_calls validator intentionally rejects a single empty set `[[]]` as the only alternative. This is by design: if no tool calls are expected, the tool_eval metric should not be configured for that turn. Empty sets are only valid as fallback alternatives (e.g., `[[[tool_call]], [[]]]`), representing optional tool call scenarios, not as primary or sole expectations.
Applied to files:
tests/unit/core/models/test_data.py
📚 Learning: 2025-09-18T23:59:37.026Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 55
File: src/lightspeed_evaluation/core/system/validator.py:146-155
Timestamp: 2025-09-18T23:59:37.026Z
Learning: In the lightspeed-evaluation project, the DataValidator in `src/lightspeed_evaluation/core/system/validator.py` is intentionally designed to validate only explicitly provided user evaluation data, not resolved metrics that include system defaults. When turn_metrics is None, the system falls back to system config defaults, and this validation separation is by design.
Applied to files:
tests/unit/core/models/test_data.py
📚 Learning: 2026-01-06T10:26:30.274Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 127
File: src/lightspeed_evaluation/core/api/streaming_parser.py:122-149
Timestamp: 2026-01-06T10:26:30.274Z
Learning: DataValidationError in src/lightspeed_evaluation/core/system/exceptions.py is used exclusively for data file validation scenarios (YAML parsing, Pydantic model validation, file existence checks) in validator.py, not for runtime API response validation errors.
Applied to files:
tests/unit/core/models/test_data.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests (3.13)
🔇 Additional comments (5)
tests/unit/core/models/test_data.py (5)
1-33: LGTM!The imports are minimal and appropriate.
TestTurnDatacovers the essential required field validations (minimal fields, empty turn_id, empty query) with correct use ofpytest.raises.
36-186: LGTM!Comprehensive coverage of
expected_tool_callsvalidation scenarios. The tests correctly verify:
- Format conversion (single set → multiple sets)
- Empty alternatives as valid fallbacks (but not as sole/first alternative)
- Rejection of invalid structures and redundant empty alternatives
This aligns with the retrieved learning that empty sets are only valid as fallback alternatives, not as primary expectations.
189-214: LGTM!Format detection tests correctly verify empty list rejection and single set format detection logic.
216-271: LGTM!Thorough validation coverage for
expected_keywordsincluding valid cases, None handling, and rejection of invalid inputs (non-list, empty inner lists, empty strings).
289-340: LGTM!Good coverage for
EvaluationResultincluding default values, status validation, execution time constraints, and conversation-level metrics withNoneturn_id. The tests appropriately verify the model's constraints.
asamal4
force-pushed
the
consolidate-testdatamodel
branch
from
d6fefe1 to
8d7add5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/core/models/test_data.py (2)
238-238: Consider adding aNonecheck for consistency.Other tests in this file (e.g., lines 52, 70-71, 87-88) assert
is not Nonebefore accessing properties. Adding this check here would improve test clarity and provide a better error message if the assertion fails.♻️ Suggested improvement
+ assert turn_data.expected_keywords is not None assert len(turn_data.expected_keywords) == 2
308-359: Good coverage of EvaluationResult validation.Tests correctly verify:
- Default values for optional fields
- Constraint on
resultstatus values- Non-negative
execution_timeenforcementNoneis valid forturn_id(conversation-level metrics)Consider adding a parameterized test for all valid result status values (e.g.,
PASS,FAIL,ERROR,SKIPPED) to ensure they're all accepted. This would complement the invalid status rejection test.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/core/models/test_data.pytests/unit/core/models/test_data_additional.py
💤 Files with no reviewable changes (1)
- tests/unit/core/models/test_data_additional.py
🧰 Additional context used
📓 Path-based instructions (2)
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest mocking (mocker fixture) instead of unittest.mock
Files:
tests/unit/core/models/test_data.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test structure to source code organization, with test files named test_.py and test functions/classes named test_ and Test*
Files:
tests/unit/core/models/test_data.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: src/lightspeed_evaluation/core/output/generator.py:140-145
Timestamp: 2025-09-11T12:47:06.747Z
Learning: User asamal4 prefers that non-critical comments are sent when actual code changes are pushed, not on unrelated commits.
📚 Learning: 2025-10-31T11:54:59.126Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 90
File: src/lightspeed_evaluation/core/models/data.py:198-208
Timestamp: 2025-10-31T11:54:59.126Z
Learning: In the lightspeed_evaluation framework, the expected_tool_calls validator intentionally rejects a single empty set `[[]]` as the only alternative. This is by design: if no tool calls are expected, the tool_eval metric should not be configured for that turn. Empty sets are only valid as fallback alternatives (e.g., `[[[tool_call]], [[]]]`), representing optional tool call scenarios, not as primary or sole expectations.
Applied to files:
tests/unit/core/models/test_data.py
📚 Learning: 2025-09-18T23:59:37.026Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 55
File: src/lightspeed_evaluation/core/system/validator.py:146-155
Timestamp: 2025-09-18T23:59:37.026Z
Learning: In the lightspeed-evaluation project, the DataValidator in `src/lightspeed_evaluation/core/system/validator.py` is intentionally designed to validate only explicitly provided user evaluation data, not resolved metrics that include system defaults. When turn_metrics is None, the system falls back to system config defaults, and this validation separation is by design.
Applied to files:
tests/unit/core/models/test_data.py
📚 Learning: 2026-01-06T10:26:30.274Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 127
File: src/lightspeed_evaluation/core/api/streaming_parser.py:122-149
Timestamp: 2026-01-06T10:26:30.274Z
Learning: DataValidationError in src/lightspeed_evaluation/core/system/exceptions.py is used exclusively for data file validation scenarios (YAML parsing, Pydantic model validation, file existence checks) in validator.py, not for runtime API response validation errors.
Applied to files:
tests/unit/core/models/test_data.py
🧬 Code graph analysis (1)
tests/unit/core/models/test_data.py (1)
src/lightspeed_evaluation/core/models/data.py (1)
TurnData(36-315)
🔇 Additional comments (5)
tests/unit/core/models/test_data.py (5)
1-11: LGTM!Imports are clean and correctly reference the three models under test. The consolidation of
EvaluationDataandEvaluationResultalongsideTurnDataaligns with the PR objective.
13-33: LGTM!Good coverage of
TurnDatarequired field validation. The tests correctly verifymin_length=1constraints onturn_idandquery, and confirm optional fields default toNone.
36-186: Comprehensive validation coverage.Tests thoroughly exercise the
expected_tool_callsvalidation logic including:
- Format conversion (single → multiple sets)
- Empty alternatives as fallback (correctly aligned with the design that empty sets are valid only as fallback alternatives, not as primary or sole expectations)
- Rejection of invalid structures, empty sequences, and redundant empty alternatives
The error message assertions (
match=...) correctly verify the specific validation paths are being triggered.
189-213: LGTM!Format detection tests correctly validate that:
- Empty list
[]is rejected as "the only alternative"- Single-set format is converted to one alternative containing multiple sequences
This aligns with the framework's intentional design where empty sets are only valid as fallback alternatives. Based on learnings, the validation correctly rejects
[[]]as the sole expectation.
273-305: Good foundational coverage for EvaluationData.Tests correctly validate required field constraints (non-empty
conversation_group_id, non-emptyturnslist). The test structure is clear and follows the established patterns.
2 participants
Description
Consolidate test cases for data models
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.