Repository navigation
docs(tests): add makefile commands for test standardisation, update documentation on usage - #251
Conversation
…ocumentation on usage
github-actions
Bot
added
the
documentation
frayle-ons
left a comment
frayle-ons
left a comment
There was a problem hiding this comment.
looks good, well laid out. I think one typo and 2 optional suggestions for thought
|
|
||
| - `make run-tests-all` : run all unit tests | ||
| - `make run-tests-vectorisers` : run unit tests associated with the vectorisers module | ||
| - `make run-tests-all` : run unit tests associated with the indexers module (WIP) |
There was a problem hiding this comment.
typo here - its the same as the first command to run all tests
There was a problem hiding this comment.
Thanks for catching, I'll push a correction now
| uv run --extra all pytest tests/test_vectorisers | ||
|
|
||
| run-tests-indexers: ## run all available unit tests | ||
| uv run --extra all pytest tests/test_indexers |
There was a problem hiding this comment.
do we need the --extra all flag here? If we're mocking the vectorisers during the indexers and other tests modules then i would think no? And maybe that would mean a slight speed up?
There was a problem hiding this comment.
I wasn't sure, thought I'd play it safe - the indexers tests are empty at the moment, pending the PR introducing them, and wasn't sure if it would be needed after any changes we make.
I think you're right though, and we won't need it for this one.
I'll update it do disable this command until they're added though, like I have with the servers one.
There was a problem hiding this comment.
Following a chat about this - I'll add to this PR to include updates to CONTRIBUTING.md and README.md to clarify developer environment requirements more explicitly
| make run-tests-evaluation # evaluation module tests (Planned - not yet available) | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
Is it worth adding a note about what our makefile is doing under the hood (using UV and Pytest)? And that we recommend/require running through make?
It would probably help developers understand system requirements and possibly allow them to carefully run tests locally even if they don't have access to libraries like uv but do have access to pytest another way.
There was a problem hiding this comment.
Part of the decision to standardise on uv for development, and the introduction of these make commands, is to standardise the developer setup as far as possible to minimise any developer-specific issues / environment differences.
So, I'd be against deviation from this approach in general.
frayle-ons
left a comment
frayle-ons
left a comment
There was a problem hiding this comment.
Looks really good. I've test the new make commands on my machine and they work well. The new CONTRIBUTING.md text renders fine in my vs code preview.
Small punctuation question, but having looked at the rest of the repo not sure we have a super consistent way of formatting lists.
Should we resolve #250 before merging this?
| ### Testing | ||
| We currently do not have a testing pipeline setup for this repository. | ||
| We are developing a testing harness of `pytest` unit tests to ensure changes to one area to not silently cause problems in another. | ||
| Developers can run these tests locally as they work to confirm their understanding of how their changes impact other areas of the codebase using the following commands; |
There was a problem hiding this comment.
hypercritical point here - shouldn't it be a colon, not a semi-colon preceding the list? "using the following commands:"
and should each item in the list end with a comma, before the final one ends with a full stop?
…developer-expectations
frayle-ons
left a comment
frayle-ons
left a comment
There was a problem hiding this comment.
I've re-run the tests on my machine and they work after #250 was merged. Also changes for documentation look good.
📌 Pull Request Template
✨ Summary
Update documentation to reflect introduction of unit tests, and expected developer behaviour surrounding them.
📜 Changes Introduced
run-tests-...|all/vectorisers/indexers/...)CONTRIBUTING.mdto clarify expectations around test maintenance and improvementREADME.mdto clarify standardised local running of unit tests viamakecommands✅ Checklist
terraform fmt&terraform validate)🔍 How to Test
makecommands locally