-
Notifications
You must be signed in to change notification settings - Fork 2
Fix tests that don't require an API key #138
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
Conversation
| ```bash | ||
| uv run pytest -v -m "not apikey" | ||
| ``` | ||
|
|
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.
Why do you delete it?
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.
In my opinion it is a useless section that will be constantly out of date (as it already is - there is no agents dir). I predicted that would happen here #76 (comment).
| @pytest.mark.parametrize("path", example_llm_config_paths, ids=[path.name for path in example_llm_config_paths]) | ||
| def test_example_llm_configs(path: Path) -> None: | ||
| def test_example_llm_configs(path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setenv("OPENAI_API_KEY", "test_key") |
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.
I remember we had a problem with monkeypatch. It overwrited the real API KEY for smoke tests.
Let's think how to keep tests independent.
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.
From the documentation:
All modifications will be undone after the requesting test function or fixture has finished.
I believe the problem before was with autouse=True in a fixture and not with monkeypatch.
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.
About autouse -
autouse –
If True, the fixture func is activated for all tests that can see it.
I'm not sure it should work for tests in different files, but it was the case.
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.
Really I can't reproduce this problem with monkeypatch + autouse. Probably I'm missing some details.
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.
@kosstbarz I figured it out. The problem was in @cached_property in LLMConfig: https://github.com/JetBrains/databao/blob/7fde8ffb8f23aeb2ec47e60c2d4ab1ab1021d985/databao/configs/llm.py#L61
LLMConfig.chat_model was returning the cached LLM from whatever was the first test because the cache is global across all tests.
I will make a PR (#141) to remove @cached_property to avoid similar future problems.
# Conflicts: # README.md
uv run pytest -v -m "not apikey"didn't work with some tests because ChatOpenAI raises an error if no api key is detected even if the model is not used.