Add tests for cache case resolution (from PR #4822)#4823
Conversation
Tests for resolve_cached_repo_id_case and get_model_config case resolution, separated from the runtime changes in PR #4822.
There was a problem hiding this comment.
Code Review
This pull request introduces test suites for cache case resolution and model configuration routing. The feedback points out a potential test failure on case-insensitive filesystems, an incorrect mock for is_local_path due to local imports, and a limitation in the _DummyLogger mock that prevents method chaining.
| _mk_cache_repo(tmp_path, "Org/Model") | ||
| _mk_cache_repo(tmp_path, "org/model") |
There was a problem hiding this comment.
This test will fail on case-insensitive filesystems (like macOS or Windows) because models--Org--Model and models--org--model resolve to the same directory. Consequently, only one variant will exist on disk, and stats["tie_breaks"] will be 0 instead of 1.
To make this test cross-platform, consider mocking the directory listing logic (e.g., os.listdir or Path.iterdir) instead of relying on the actual filesystem, or skip the test if the filesystem is case-insensitive.
| calls["from_identifier"] = model_name | ||
| return _DummyModelConfig() | ||
|
|
||
| monkeypatch.setattr(models_route, "is_local_path", lambda _: False) |
There was a problem hiding this comment.
The mock for is_local_path is applied to the models_route module, but the get_model_config function in routes/models.py imports is_local_path locally from utils.models.model_config (line 600).
Because of this local import, the function will use the real is_local_path from the source module rather than the one you've patched on models_route. You should patch it on model_config_module instead.
| monkeypatch.setattr(models_route, "is_local_path", lambda _: False) | |
| monkeypatch.setattr(model_config_module, "is_local_path", lambda _: False) |
| class _DummyLogger: | ||
| def __getattr__(self, _name): | ||
| return lambda *args, **kwargs: None |
There was a problem hiding this comment.
The _DummyLogger mock is too simplistic and will cause tests to crash if the code uses method chaining (e.g., logger.bind(...).info(...)), which is a common pattern with structlog.
Returning self from the mocked methods allows for such chaining.
| class _DummyLogger: | |
| def __getattr__(self, _name): | |
| return lambda *args, **kwargs: None | |
| class _DummyLogger: | |
| def __getattr__(self, _name): | |
| return lambda *args, **kwargs: self |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f099dbb0b
ℹ️ 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".
| from utils.paths.path_utils import ( | ||
| resolve_cached_repo_id_case, | ||
| get_cache_case_resolution_stats, | ||
| reset_cache_case_resolution_state, |
There was a problem hiding this comment.
This test module imports resolve_cached_repo_id_case, get_cache_case_resolution_stats, and reset_cache_case_resolution_state, but none of those symbols exist in studio/backend/utils/paths/path_utils.py at this commit, so test collection will raise ImportError before any test runs. Unless the implementation is included in the same change, this commit breaks the backend test suite.
Useful? React with 👍 / 👎.
| monkeypatch.setattr(models_route, "is_local_path", lambda _: False) | ||
| monkeypatch.setattr( | ||
| models_route, "resolve_cached_repo_id_case", lambda _: "Org/Model" | ||
| ) |
There was a problem hiding this comment.
monkeypatch.setattr here targets routes.models.is_local_path and routes.models.resolve_cached_repo_id_case, but those attributes are not defined on routes.models in this commit. With default raising=True, setup fails with AttributeError, so the regression test never executes. Patch the real import locations (or add the runtime symbols in the same change) to keep the test runnable.
Useful? React with 👍 / 👎.
…ai#4823) Tests for resolve_cached_repo_id_case and get_model_config case resolution, separated from the runtime changes in PR unslothai#4822.
Summary
resolve_cached_repo_id_casecovering exact-case hits, case-variant hits, tie-breaking, no-cache fallback, memoization, and late cache populationget_model_confignormalizes casing before vision/embedding/audio checksTest plan
pytest studio/backend/tests/test_cache_case_resolution.py studio/backend/tests/test_models_get_model_config_case_resolution.py