Conversation
|
@KRRT7 Is this indeed caused by your #231? (And undiscovered due to me not running the tests manually.) Do you agree with the fix-upon-fix that Claude came up with for me? It passes all tests and doesn't look completely implausible but I admit that I've lost track of all the details around the env vars we use (by old convention established in microsoft/TypeAgent) and I didn't review #231 well enough. @bmerkle Once @KRRT7 approves the changes can you also review it? |
|
let me take a quick look |
Did you use Azure? How did you specify the endpoints? We have a convention used in our Maybe it would be useful to run make coverage` and see what code is or isn't hit here. |
|
|
in my opinion every single line of code should have as close to 100% test coverage, so maybe make it a requirement, internally, I started doing this by establishing a baseline, and increasing that baseline over time for an incremental migration, this helps catch these sort of issues more easily. |
Nice theory. In practice this would add a huge amount of tests which would require a lot of effort (and LLM tokens) to add. Let's not go overboard here. |
There are different test coverage metrics and we have to be realistic here...effort vs. ROI High function cov should BE doable I have kicked off a branch with additional Test for function coverages. Lets start with this. |
|
Fair point — I'm not suggesting writing tests for every line all at once. The approach I've found works well is: establish the current baseline (whatever @bmerkle that sounds like a great starting point — function coverage first is exactly the right priority. Happy to review the PR when it's up. |
Addresses review comment from microsoft#241.
## Summary - Adds 3 tests for `resolve_azure_model_name` covering: deployment name extraction from endpoint, fallback to provided model name, and default `AZURE_OPENAI_ENDPOINT` envvar usage - Fixes `test_create_embedding_model_uses_azure_deployment_name` — wasn't clearing `OPENAI_EMBEDDING_MODEL` from the environment, so the assertion failed when that envvar was set - Addresses review comment from #241 ## Test plan - [x] `make check` (pyright) — 0 errors - [x] `pytest tests/test_utils.py tests/test_model_adapters.py` — 33/33 pass - [x] `make test` — 496 pass, 3 fail (all transient 429 rate limits, addressed in #245)
4 participants
Looks like some regressions were caused by #231 that only showed when using real API keys. Shame on me for not running tests locally before approving PRs.