Add initial unit tests #763
Conversation
slifty
force-pushed
the
33-add-unit-tests
branch
from
1083e54 to
793d4eb
Compare
slifty
force-pushed
the
33-add-unit-tests
branch
3 times, most recently
from
f6c6276 to
8afbdb4
Compare
AGENTS.md
Outdated
| ## Testing | ||
|
|
||
| Tests use Jest with ts-jest preset. Test files should be named `*.test.ts` and placed alongside the source files they test. | ||
| Tests use Jest with ts-jest preset. Test files should be named `*.test.ts` and placed in `__tests__` directories close to the source files they test. |
There was a problem hiding this comment.
Aesthetically I dislike __tests__ directories, but if they are standard for Typescript I'm fine with them as long as we still keep test files close to the files they test
There was a problem hiding this comment.
@liam-lloyd actually I believe either way goes in the land of TypeScript and I'm happy to defer to your preferences.
I personally tended to aesthetically prefer __tests__ because I felt it reduces the clutter / makes it easier to know "this file contains code" vs "this file contains tests". I suppose one other thing is sometimes you're writing things like integration tests that don't map as nicely to individual files and in that case a src/__tests__/foo.int.test.ts pattern is arguably the right approach
On the flip side, __tests__ makes it harder to realize you forgot to write tests for something, and probably other things too 😅
All this is to say: now is exactly the right time to choose a path here.
There was a problem hiding this comment.
I asked the robot for a summary for our consideration: https://chatgpt.com/share/695c2193-77a4-800f-af5f-3b68c7af8147
Just to underscore that both are fine so if you'd like to lean into the preference you shared above let's go for it!
There was a problem hiding this comment.
I think the main driver of my preference (outside of familiarity; Go doesn't tend to use __tests__ directories) is I like test files to be easy to access from the source files and vice versa. Keeping them in the same directory provides that. Having something like
- controllers
- controller.ts
- __tests__
- controller.test.ts
isn't bad on that front either but if the test folder gets nested any further I think it becomes an annoyance. I think integration tests are usually well described as tests of the top-level thing they cover (e.g. stela's tests are mostly integration tests of the controller, even though they cover the service too).
There was a problem hiding this comment.
The robot's summary reinforces my preference on the grounds that the drawbacks of .test.ts are things you deal with once per project while the drawbacks of __tests__ are things you deal with continuously.
There was a problem hiding this comment.
OK this sounds reasonable to me (FWIW I also agree that in the __tests__ model a test for a file should be as you showed above, except for integration tests which should have the test at the most appropriate high-level location.)
Also this is easy to tweak at a later time as desired.
Thanks for thinking it through!
My takeaway from this thread right now is that the current PR is OK for now, with a directory model; if we find an LLM is creating tests anywhere except the immediate directory's __tests__ folder then we will update the instructions appropriately.~
There was a problem hiding this comment.
Whoops didn't see your second post!
I'll update to .test.ts
An AGENTS.md file helps LLM agents better understand a code base. This file is intentionally non-proprietary, and someone can symlink it to whatever file is expected by their local environment.
We didn't actually have tests, which means some of our setup was not quite right for modern jest. These changes address some of the warnings jest was exposing when writing tests. Issue #33 We need tests
We want to be able to use multiple levels of `describe` in tests, which makes this eslint rule unhelpful for test files.
This narrowly scoped commit adds our first unit tests. This particular function was picked just because it was a good candidate and the real contribution here is moving from 0% coverage to >0% coverage. This allows us to ensure our tests actually run, locally and in CI, and sets the stage for additional unit tests in the future. Issue #33 We need tests
slifty
force-pushed
the
33-add-unit-tests
branch
from
8afbdb4 to
d62a6c4
Compare
This PR is intentionally narrow. It:
The intention is to ensure that tests work at all in a reviewable format. We will be adding more tests later.
This PR also adds an
AGENTS.mdfile which is a good thing to have around in this modern age of LLM augmentation.