fix(tests): guard OpenAI embedding tests and drop read-only property monkeypatch (#288)#291
Merged
Conversation
…monkeypatch - Extend skipif guard on TestOpenAIBatchSplitting to require both pydantic-ai and openai, since _build_model imports pydantic_ai.embeddings.openai which needs the openai package. - Remove monkeypatch.setattr on llm_model_name in the classification stub-fallback fixture — it's a read-only derived property that already resolves to None once llm_provider/llm_model are patched. Fixes Mr-DooSun#288
Mr-DooSun
self-requested a review
July 14, 2026 11:42
Mr-DooSun
approved these changes
Jul 14, 2026
Mr-DooSun
left a comment
Owner
There was a problem hiding this comment.
Thanks for the careful fix, @Diyaaa-12 — and for splitting these two latent failures out of the #287 review into their own tracked issue.
Reviewed against the source and confirmed both fixes:
- Bug 1 (embedding guard): Requiring
openaiin the skip guard is exactly right.PydanticAIEmbeddingAdapter.__init__→_build_modelrunsfrom pydantic_ai.embeddings.openai import OpenAIEmbeddingModelwhen the test builds anopenai:config with anapi_key, soTestOpenAIBatchSplittinggenuinely needsopenai, not justpydantic_ai. - Bug 2 (read-only property): Confirmed
Settings.llm_model_nameis a setter-less@propertyderived fromllm_provider/llm_model, somonkeypatch.setattr(..., "llm_model_name", None)raisedAttributeErrorat setup (reproduced locally). The two remaining lines already drive the property toNone, and the container's LLM selector reads that derived value at resolution time (core_container.py:56), sotest_core_container_llm_model_is_test_modelstill resolves toTestModel. Behavior-preserving.
Non-blocking note: the new guard checks pydantic_ai + openai but not tiktoken (needed by _init_tiktoken). No real gap — both the openai and pydantic-ai extras pull in tiktoken, so whenever openai resolves it's present. Fine to leave as-is.
Also agree with keeping the CI-matrix decision (whether the test job installs these extras) out of scope — worth remembering that until that lands, these two tests stay skipped in CI.
Approving — I'll merge shortly. Thanks again for the clean, well-documented contribution!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the
skipifguard onTestOpenAIBatchSplittingto require bothpydantic_aiandopenai, and removes a monkeypatch on a read-only property in the classification stub-fallback fixture. Both were latent bugs discovered during #287's CI review, tracked here per your request.Related Issue
Change Summary
skipifguard onTestOpenAIBatchSplitting(test_pydantic_ai_embedding_adapter.py) to require bothpydantic_aiandopenai, since_build_modelimportspydantic_ai.embeddings.openaiwhich needs theopenaipackage.monkeypatch.setattr(settings, "llm_model_name", None)in the classification stub-fallback fixture — it's a read-only derived property that already resolves toNoneoncellm_provider/llm_modelare patched.Type of Change
Checklist
ruff check src/)How to Test
Scope kept to just these two test fixes as requested — the CI-matrix decision (whether the test job should install
--extra pydantic-ai/openai) is left out of scope.Governor Footer