-
Notifications
You must be signed in to change notification settings - Fork 2
Strengthen test validation for generated field structure in orchestrate_content tests #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Strengthen test validation for generated field structure in orchestrate_content tests #208
Conversation
…ture Co-authored-by: 73junito <86015877+73junito@users.noreply.github.com>
73junito
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR strengthens test validation in test_orchestrate_persist by adding assertions that validate the structure and contents of the generated field created by process_module(). Previously, the test only verified that the generated key existed without checking its actual structure or data.
Changes:
- Add validation for
generatedfield structure as a dictionary - Add validation for
humanized_textfield (type, presence, and non-empty content) - Add validation for
ttft_sfield (presence, numeric type, and non-negative value)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify TTFT is present and valid when returned from call_ollama | ||
| assert "ttft_s" in generated, "generated should contain ttft_s" | ||
| assert isinstance(generated["ttft_s"], (int, float)), "ttft_s should be a number" | ||
| assert generated["ttft_s"] >= 0, "ttft_s should be non-negative" |
Copilot
AI
Jan 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertions for ttft_s assume it will always be present in the generated field, but this doesn't match the actual implementation behavior.
Looking at process_module() in scripts/orchestrate_content.py (lines 129-130), ttft_s is only set conditionally: if "ttft" in res: module["generated"]["ttft_s"] = float(res["ttft"]).
The real call_ollama() only returns ttft when stream=True (line 77), but NOT in non-streaming mode (lines 84-88 only return text). This test uses stream=False (line 39), so with the real implementation, ttft would not be returned and ttft_s would not be set.
While the test's fake_call returns ttft even in non-streaming mode, this doesn't reflect realistic behavior. Consider either:
- Changing the test to use
stream=Trueto match a realistic scenario, or - Making the
ttft_svalidation conditional by checking if it exists before asserting its type and value
| # Verify TTFT is present and valid when returned from call_ollama | |
| assert "ttft_s" in generated, "generated should contain ttft_s" | |
| assert isinstance(generated["ttft_s"], (int, float)), "ttft_s should be a number" | |
| assert generated["ttft_s"] >= 0, "ttft_s should be non-negative" | |
| # Verify TTFT is valid when returned from call_ollama (optional field) | |
| if "ttft_s" in generated: | |
| assert isinstance(generated["ttft_s"], (int, float)), "ttft_s should be a number" | |
| assert generated["ttft_s"] >= 0, "ttft_s should be non-negative" |
Summary
test_orchestrate_persistto validate the structure and contents of thegeneratedfieldprocess_module()Changes
generatedis a dictionary with expected schema:humanized_text: non-empty stringttft_s: non-negative number"generated" in data[0], which could pass with incorrect dataTesting
test_orchestrate_content.pypassChecklist
Reviewers
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.