Add tests for is_vision_model() caching behaviour#4855
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test suite for the is_vision_model caching mechanism, covering cache hits, misses, subprocess paths, and token-based isolation. The review identifies an unused helper function _make_config that should be removed and suggests modifying test_exception_result_cached to raise an actual exception to properly verify the fallback logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a25c064dea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Remove unused _make_config() helper function (dead code) - Fix test_exception_result_cached to actually exercise the exception path by mocking load_model_config to raise OSError instead of using side_effect=[False] which only tested normal False returns
8fb09c4 to
14d6731
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f04582de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Use MagicMock(spec=[]) for all config mocks so hasattr() only returns True for explicitly set attributes. Without this, MagicMock defaults make all hasattr checks truthy, allowing tests to pass via unintended detection paths (e.g. img_processor instead of vision_config).
* Add tests for is_vision_model() caching behaviour * Fix review feedback: remove dead helper, fix exception test - Remove unused _make_config() helper function (dead code) - Fix test_exception_result_cached to actually exercise the exception path by mocking load_model_config to raise OSError instead of using side_effect=[False] which only tested normal False returns * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Use strict mock specs so tests exercise intended detection paths Use MagicMock(spec=[]) for all config mocks so hasattr() only returns True for explicitly set attributes. Without this, MagicMock defaults make all hasattr checks truthy, allowing tests to pass via unintended detection paths (e.g. img_processor instead of vision_config). --------- Co-authored-by: Roland Tannous <rolandtannous@gravityq.ai> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Summary
is_vision_model()caching layer introduced in [Studio][Optimization]Add vision detection cache to is_vision_model() #4853Test plan
pytest studio/backend/tests/test_vision_cache.py -vafter merging [Studio][Optimization]Add vision detection cache to is_vision_model() #4853_vision_detection_cacheand_is_vision_model_uncachedexports from [Studio][Optimization]Add vision detection cache to is_vision_model() #4853