darthrubik
11073a12c8
Clarify that test flags should be minimal hooks into internal functionality, not contain test logic. All test logic (assertions, validation) belongs in Bash test scripts in tests/. This addresses the ambiguity mentioned in issue 109 regarding the distinction between test hooks in main.cpp and test scripts.
124 lines
4.8 KiB
Markdown
124 lines
4.8 KiB
Markdown
# Contributing to drharness
|
|
|
|
Thank you for your interest in contributing to **drharness**! We welcome
|
|
contributions that improve the harness's stability, add new tools, or enhance
|
|
its documentation.
|
|
|
|
## 🏗️ Building the Project
|
|
|
|
Prerequisites: CMake 3.16+, a C++23 compiler, `libcurl`, and the
|
|
`nlohmann_json` library.
|
|
|
|
```bash
|
|
# Configure
|
|
cmake -B build -DCMAKE_BUILD_TYPE=Release .
|
|
|
|
# Build
|
|
cmake --build build
|
|
```
|
|
|
|
|
|
## 📦 Dependencies
|
|
|
|
Do not add new dependencies to the project unless explicitly requested in the
|
|
issue. The project aims to minimize external dependencies to keep the codebase
|
|
simple and easy to build. If you believe a new dependency is necessary, discuss
|
|
it in the relevant issue first.
|
|
|
|
## 💻 Coding Style
|
|
|
|
### No Namespaces
|
|
|
|
This project does not use namespaces in C++ code. This policy makes it easier
|
|
to search the codebase (e.g., using `grep`) and keeps the code simpler.
|
|
|
|
Do not introduce new namespaces in your contributions.
|
|
|
|
## 🛡️ Error Handling
|
|
|
|
This project uses `std::expected<T, std::string>` (C++23) exclusively for error
|
|
handling. C++ exceptions (`throw`/`try`/`catch`) are not used.
|
|
|
|
### Rules
|
|
|
|
- **JSON parsing**: Always use `parse_json_safely()` — never call
|
|
`nlohmann::json::parse()` directly.
|
|
- **JSON deserialization**: Always use `json_to<T>()` — never call `j.get<T>()`
|
|
or `j.at()` directly.
|
|
- **No try/catch**: Handle errors explicitly via `std::expected` return values.
|
|
- **No exceptions**: Do not add new `throw` statements to the codebase.
|
|
- **Error type**: All errors use `std::expected<T, std::string>` where the
|
|
error is a human-readable `std::string` message.
|
|
- **Library vs top-level**: Library code returns `std::expected` and lets
|
|
callers decide how to handle errors. Use `expect_or_exit()` only in
|
|
top-level code (e.g., `main()`) for fatal errors.
|
|
|
|
## 🧪 Testing Guidelines
|
|
|
|
Tests in `drharness` are implemented as **Bash scripts**. They interact with
|
|
the compiled binary and use `stdin` to mock LLM responses, allowing for
|
|
deterministic, fast, and dependency-free testing.
|
|
|
|
### Running Tests
|
|
After building, run the full suite from the build directory:
|
|
```bash
|
|
cd build
|
|
ctest
|
|
```
|
|
|
|
### Adding a New Test
|
|
1. Create a new script in `tests/` named `test_<feature>.sh`.
|
|
2. Use the helper functions exported by `tests/run_test.sh`:
|
|
- `be_done "<response>"`: Mocks a final AI response.
|
|
- `tool_call <name> <args>`: Mocks a tool invocation request.
|
|
- `dr_result <chat_file> <index>`: Extracts a message from the saved JSON history.
|
|
- `check_tool_call <chat_file> <index> <expected_output>`: Asserts a tool call's result.
|
|
3. Pipe mock responses into `drharness` via stdin.
|
|
4. Validate stdout, stderr, or the generated JSON history file.
|
|
5. Register your test in `tests/CMakeLists.txt`:
|
|
```cmake
|
|
add_dr_test(test_<feature>)
|
|
```
|
|
**Note:** Test scripts execute in an isolated temporary directory
|
|
(`$TEST_TEMP_DIR`). Use relative paths or `$TEST_TEMP_DIR` explicitly when
|
|
reading/writing files.
|
|
|
|
**Note:** Do not do a bunch of json parsing in the test files. Most of your
|
|
json parsing should just be using the helper functions. If you need a new
|
|
helper function make one in `run_test.sh` instead of doing inline json parsing
|
|
in the tests. **If you do any json handling in the test files your changes
|
|
will be rejected**
|
|
|
|
**Note:** We will not accept untested code. Please write tests for your code.
|
|
Exception only if it is API calling code which is hard to test
|
|
deterministically.
|
|
|
|
### Adding new flags to the CLI for testing
|
|
Adding new flags to the CLI for testing is allowed and ENCOURAGED! These
|
|
flags serve as **hooks into internal functionality** to enable testing of
|
|
modules that don't have a natural CLI entry point.
|
|
|
|
For example, for the skill stuff, we added a flag -D that just dumps all skill
|
|
directories found. This is useless to the end user, but lets us test the fact
|
|
that our skill directory search was working.
|
|
|
|
**Test flags should be minimal hooks:**
|
|
- They should accept input, call the internal function, and output the result
|
|
- They should NOT contain test logic (assertions, validation, etc.)
|
|
- All test logic belongs in the Bash test scripts in `tests/`
|
|
- The flag's only job is to bridge CLI arguments to the internal function
|
|
|
|
If you add a new flag for testing you don't need to update the README.md (but
|
|
updating the -h message is still mandatory).
|
|
|
|
## 📝 Updating the README.md
|
|
|
|
If your contribution modifies the CLI behavior, adds a new tool, or changes
|
|
build dependencies, please update `README.md` to keep user-facing documentation
|
|
accurate:
|
|
|
|
- **CLI Flags**: Update the "💬 Starting & Continuing Conversations" section if you add new flags (e.g., `-t`, `-m`, `-F`).
|
|
- **Tools**: Update the "🏗️ Architectural Overview" or add a specific section if the tool is a major feature.
|
|
- **Build Steps**: Note any new system libraries or CMake requirements in the "🛠️ How to Build" section.
|
|
|