Testing architecture unit tests#479
Conversation
hanna-paasivirta
changed the base branch from
main
to
testing-architecture-service-tests
| - `yaml_assertions.py` — pure-function YAML structural assertions, safe for | ||
| every tier (unit included). | ||
|
|
||
| ## Why under `services/` and not a top-level `tests/`? |
There was a problem hiding this comment.
hmm. Aren't we better off fixing the imports or even restructuring?
Looking over the codebase there's actually quite a lot in services which isn't really a service. But I like that clean divide where services folders are expected to have a strong structure convention
I wonder if we should do a bigger restructure like:
/py
- /services
- /util
- /entry.py
- /models.py
Then you'd configure imports to run from py. And shouldn't we be able to do relative imports within a service?
This feels like quite a bit of work but we're adding so much non-service python code that I'm keen to get it right.
Let's discuss next week. Might be good to pull Elias in too
| @@ -0,0 +1,49 @@ | |||
| from workflow_chat.gen_project_prompt import build_prompt | |||
There was a problem hiding this comment.
This file feels good: it's importing a single function and setting up a bunch of independent unit tests on it.
The file name is pretty awkward though. Should we introduce more folders here? workflow_chat/tests/unit/gen_project/test_prompt_build.py
| from pathlib import Path | ||
| from typing import Dict, List, Optional, Any | ||
|
|
||
| # YAML helpers live in `testing.yaml_assertions`; re-exported so existing |
There was a problem hiding this comment.
I find this comment a bit sus. Why do we need to explain a basic import like this?
|
Tests are now running against this branch, which is cool. I'm actually a bit surprised by that because I thought the action had to be on main for hooks to trigger. I must be getting my wires crossed there. Anyway it's failing on the poetry install step. I don't have time to investigate this afternoon - I'll sort it next week. |
| from workflow_chat.workflow_chat import AnthropicClient | ||
|
|
||
|
|
||
| def _make_client(): |
There was a problem hiding this comment.
This is a strange pattern
I get it, but it's not pretty
What's happening is we're using a weird python syntax to create a client instance without triggering the constructor. We have to do this because the constructor creates an Anthropic client and blows up in test.
But it's all backwards that we're working around this. We're in total control of the code.
The best option I think is to make extract_and_preserve_components a static method. Now you don't need to instantiate a client instance to use it.
We could also modify the constructor to disable client creation, or even better accept a mock client instance, just for tests. We may need to do this one day.
Finally we could declare extract_and_preserve_components as a standard python function (not in the class). Since it doesn't use any class state it might make more sense. Then we either remove it from the class, or just proxy into it if self.extract_and_preserve_components is a useful thing to have.
There was a problem hiding this comment.
I have split it into a client folder, and made the extraction and sanitization into static methods.
We don't really have simple unit tests yet in job_chat or global_chat. Should I add a few under this new architecture?
|
Replaced with #486 |
2 participants
Short Description
Implements the unit test architecture according to the testing plan section 1 apollo/agent-team-architecture-plan/1-unit-tests.md
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy