From 453afd1fdc6b7b2d01f771fac44bac9cb3a002b7 Mon Sep 17 00:00:00 2001 From: Pavel Punsky Date: Mon, 20 Apr 2026 21:15:12 -0700 Subject: [PATCH] Add Unity-based unit test scaffolding (#1875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Introduces an opt-in unit test layer for coturn using [Unity](https://github.com/ThrowTheSwitch/Unity) — a single-header pure-C test framework that matches coturn's C11 toolchain, portability bar, and zero-C++ production tree. - Unity v2.6.0 is fetched on demand via CMake `FetchContent` (nothing vendored). - Tests are gated behind `-DBUILD_TESTING=ON` (off by default), so the standard build and OSS-Fuzz pipeline are unaffected. - Two test binaries cover pure C-callable code in `libturnclient`: - `test_ioaddr` (6 cases) — `make_ioa_addr`, `addr_get_port`/`addr_set_port`, `addr_eq` variants, `addr_to_string`, IPv4/IPv6/garbage input - `test_stun_msg` (7 cases) — STUN header construction, request/indication/success/error response classification, transaction-ID round-trip, channel message parsing, truncated/zeroed buffer rejection - New `check` cmake target builds tests before running ctest (avoids the `make test` footgun where the auto-generated `test` target only runs already-built binaries). - Legacy `Makefile.in` gets a `unit-tests` target that bootstraps `build/unit-tests/` and delegates to the cmake `check` target. `make check` and `make test` now run the RFC 5769 conformance suite **plus** the Unity unit tests. - CLAUDE.md documents the new workflow plus the one-liner for adding a new `test_.c`. ## Why The existing test story is shell-script integration suites under `examples/scripts/` — they exercise the binary end-to-end but can't pin down behavior of individual functions, can't run without a full build environment, and don't fail loudly when a unit-level invariant breaks. A lightweight unit layer gives us: - Targeted regression coverage for protocol parsing/encoding (the highest bug-yield area). - A natural home for tests of the kinds of subtle invariants already documented in CLAUDE.md (port-counter overflow safety, port-bounds inclusivity, HMAC buffer initialization). - Sub-second feedback for contributors. ## Usage ```bash # CMake direct cmake -S . -B build -DBUILD_TESTING=ON cmake --build build -j --target check # build + run all unit tests ctest --test-dir build --output-on-failure # run already-built tests # Legacy Makefile bridge (after ./configure) make unit-tests # bootstraps build/unit-tests/, builds + runs Unity tests make check # RFC 5769 conformance + unit tests ``` Adding a new test: 1. Drop `tests/test_.c` 2. Append `coturn_add_test(test_)` in `tests/CMakeLists.txt` 3. The `check` target picks it up automatically. ## Test plan - [x] Clean cmake build with `-DBUILD_TESTING=ON` succeeds; full source tree (turnserver, turnadmin, turnclient, turn_server, all turnutils) still builds - [x] `cmake --build build --target check` builds and runs both test binaries — 13/13 cases pass - [x] `ctest --verbose` shows per-case PASS lines for all 13 cases - [x] Default build (`-DBUILD_TESTING` unset) does not fetch Unity or build any test binary ## Notes for reviewers - Why Unity over GoogleTest/Catch2: pure C, single source file, no C++ toolchain dependency, runs anywhere coturn does (incl. exotic CMake targets like Solaris/AIX). GoogleTest would force `extern "C"` wrappers and a C++ compiler everywhere. --- .../actions/ubuntu-build-deps/action.yml | 1 + .github/workflows/cmake.yml | 5 +- CLAUDE.md | 21 ++++ CMakeLists.txt | 6 + Makefile.in | 7 +- tests/CMakeLists.txt | 25 +++++ tests/test_ioaddr.c | 63 +++++++++++ tests/test_stun_msg.c | 104 ++++++++++++++++++ 8 files changed, 230 insertions(+), 2 deletions(-) create mode 100644 tests/CMakeLists.txt create mode 100644 tests/test_ioaddr.c create mode 100644 tests/test_stun_msg.c diff --git a/.github/workflows/actions/ubuntu-build-deps/action.yml b/.github/workflows/actions/ubuntu-build-deps/action.yml index 95f06beb..dfe5da3a 100644 --- a/.github/workflows/actions/ubuntu-build-deps/action.yml +++ b/.github/workflows/actions/ubuntu-build-deps/action.yml @@ -34,6 +34,7 @@ runs: clang \ clang-tidy \ cmake \ + git \ iwyu \ ninja-build \ pkgconf \ diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index e6c0555f..5489d732 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -33,10 +33,13 @@ jobs: uses: ./.github/workflows/actions/ubuntu-build-deps - name: Configure - run: cmake -S . -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} + run: cmake -S . -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DBUILD_TESTING=ON - name: Build run: cmake --build build --parallel $(nproc) + - name: Unit tests + run: ctest --test-dir build --output-on-failure + - run: ./run_tests.sh working-directory: examples/ - run: ./run_tests_conf.sh diff --git a/CLAUDE.md b/CLAUDE.md index 6d05b0c2..004e1c81 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,6 +53,27 @@ cd examples && ./scripts/basic/udp_c2c_client.sh cd examples && ./run_tests.sh ``` +### Unit tests (Unity, opt-in via `BUILD_TESTING=ON`) + +Unity is fetched on demand via CMake `FetchContent`; nothing is vendored. +Tests live under [tests/](tests/) and link against the existing +`turnclient` static library. + +```bash +# CMake direct +cmake -S . -B build -DBUILD_TESTING=ON +cmake --build build -j --target check # builds tests, runs ctest +cmake --build build -j --target test_ioaddr # build a single binary +ctest --test-dir build --output-on-failure # run already-built tests + +# Legacy Makefile bridge (after ./configure; requires cmake on PATH) +make unit-tests # bootstraps build/unit-tests/, builds + runs Unity tests +``` + +Adding a new test: drop `tests/test_.c` and append +`coturn_add_test(test_)` in [tests/CMakeLists.txt](tests/CMakeLists.txt). +The `check` target picks it up automatically. + See [docs/Testing.md](docs/Testing.md) for database setup and extended test scenarios. ## Source layout diff --git a/CMakeLists.txt b/CMakeLists.txt index 4a63ddd8..424621e4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -183,3 +183,9 @@ if(FUZZER) add_subdirectory(fuzzing) endif() + +option(BUILD_TESTING "Build unit tests" OFF) +if(BUILD_TESTING) + enable_testing() + add_subdirectory(tests) +endif() diff --git a/Makefile.in b/Makefile.in index f4706a6e..6d66996a 100644 --- a/Makefile.in +++ b/Makefile.in @@ -36,7 +36,7 @@ SERVERAPP_DEPS = ${SERVERTURN_MODS} ${SERVERTURN_DEPS} ${SERVERAPP_MODS} ${SERVE TURN_BUILD_RESULTS = bin/turnutils_oauth bin/turnutils_natdiscovery bin/turnutils_stunclient bin/turnutils_rfc5769check bin/turnutils_uclient bin/turnserver bin/turnutils_peer lib/libturnclient.a include/turn/ns_turn_defs.h sqlite_empty_db -.PHONY: all test check clean distclean sqlite_empty_db install deinstall uninstall reinstall +.PHONY: all test check unit-tests clean distclean sqlite_empty_db install deinstall uninstall reinstall all: ${TURN_BUILD_RESULTS} @@ -45,6 +45,11 @@ test: check check: bin/turnutils_rfc5769check bin/turnutils_rfc5769check +### Unit tests (Unity, configured via CMake — opt-in, requires cmake): +unit-tests: + ${MKDIR} build/unit-tests + cd build/unit-tests && cmake -DBUILD_TESTING=ON ${CURDIR} && $(MAKE) check + format: find . -iname "*.c" -o -iname "*.h" | xargs clang-format -i diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt new file mode 100644 index 00000000..776c1927 --- /dev/null +++ b/tests/CMakeLists.txt @@ -0,0 +1,25 @@ +include(FetchContent) + +FetchContent_Declare( + Unity + GIT_REPOSITORY https://github.com/ThrowTheSwitch/Unity.git + GIT_TAG v2.6.0 +) +FetchContent_MakeAvailable(Unity) + +function(coturn_add_test name) + add_executable(${name} ${name}.c) + target_link_libraries(${name} PRIVATE turnclient unity) + add_test(NAME ${name} COMMAND ${name}) + list(APPEND COTURN_TEST_TARGETS ${name}) + set(COTURN_TEST_TARGETS ${COTURN_TEST_TARGETS} PARENT_SCOPE) +endfunction() + +coturn_add_test(test_ioaddr) +coturn_add_test(test_stun_msg) + +add_custom_target(check + COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure + DEPENDS ${COTURN_TEST_TARGETS} + WORKING_DIRECTORY ${CMAKE_BINARY_DIR} +) diff --git a/tests/test_ioaddr.c b/tests/test_ioaddr.c new file mode 100644 index 00000000..3960db4a --- /dev/null +++ b/tests/test_ioaddr.c @@ -0,0 +1,63 @@ +#include "ns_turn_ioaddr.h" + +#include + +#include +#include + +void setUp(void) {} +void tearDown(void) {} + +static void test_make_ioa_addr_ipv4_sets_family_and_port(void) { + ioa_addr addr = {0}; + TEST_ASSERT_EQUAL_INT(0, make_ioa_addr((const uint8_t *)"127.0.0.1", 3478, &addr)); + TEST_ASSERT_EQUAL_INT(AF_INET, addr.ss.sa_family); + TEST_ASSERT_EQUAL_UINT16(3478, addr_get_port(&addr)); +} + +static void test_make_ioa_addr_ipv6_sets_family_and_port(void) { + ioa_addr addr = {0}; + TEST_ASSERT_EQUAL_INT(0, make_ioa_addr((const uint8_t *)"::1", 5349, &addr)); + TEST_ASSERT_EQUAL_INT(AF_INET6, addr.ss.sa_family); + TEST_ASSERT_EQUAL_UINT16(5349, addr_get_port(&addr)); +} + +static void test_make_ioa_addr_rejects_garbage(void) { + ioa_addr addr = {0}; + TEST_ASSERT_NOT_EQUAL(0, make_ioa_addr((const uint8_t *)"not-an-address", 1234, &addr)); +} + +static void test_addr_set_port_max_value_roundtrips(void) { + ioa_addr addr = {0}; + TEST_ASSERT_EQUAL_INT(0, make_ioa_addr((const uint8_t *)"127.0.0.1", 0, &addr)); + addr_set_port(&addr, 65535); + TEST_ASSERT_EQUAL_UINT16(65535, addr_get_port(&addr)); +} + +static void test_addr_eq_distinguishes_ports(void) { + ioa_addr a = {0}, b = {0}; + make_ioa_addr((const uint8_t *)"10.0.0.1", 1000, &a); + make_ioa_addr((const uint8_t *)"10.0.0.1", 1001, &b); + TEST_ASSERT_FALSE(addr_eq(&a, &b)); + TEST_ASSERT_TRUE(addr_eq_no_port(&a, &b)); +} + +static void test_addr_to_string_roundtrip_ipv4(void) { + ioa_addr addr = {0}; + char buf[MAX_IOA_ADDR_STRING] = {0}; + make_ioa_addr((const uint8_t *)"192.168.1.42", 8080, &addr); + TEST_ASSERT_EQUAL_INT(0, addr_to_string(&addr, buf)); + TEST_ASSERT_NOT_NULL(strstr(buf, "192.168.1.42")); + TEST_ASSERT_NOT_NULL(strstr(buf, "8080")); +} + +int main(void) { + UNITY_BEGIN(); + RUN_TEST(test_make_ioa_addr_ipv4_sets_family_and_port); + RUN_TEST(test_make_ioa_addr_ipv6_sets_family_and_port); + RUN_TEST(test_make_ioa_addr_rejects_garbage); + RUN_TEST(test_addr_set_port_max_value_roundtrips); + RUN_TEST(test_addr_eq_distinguishes_ports); + RUN_TEST(test_addr_to_string_roundtrip_ipv4); + return UNITY_END(); +} diff --git a/tests/test_stun_msg.c b/tests/test_stun_msg.c new file mode 100644 index 00000000..a54cea06 --- /dev/null +++ b/tests/test_stun_msg.c @@ -0,0 +1,104 @@ +#include "ns_turn_ioaddr.h" +#include "ns_turn_msg.h" +#include "ns_turn_msg_defs.h" + +#include + +#include + +void setUp(void) {} +void tearDown(void) {} + +static void test_init_request_produces_valid_stun_header(void) { + uint8_t buf[1024] = {0}; + size_t len = 0; + + stun_init_request_str(STUN_METHOD_BINDING, buf, &len); + + TEST_ASSERT_EQUAL_size_t(STUN_HEADER_LENGTH, len); + TEST_ASSERT_TRUE(stun_is_command_message_str(buf, len)); + TEST_ASSERT_TRUE(stun_is_request_str(buf, len)); + TEST_ASSERT_EQUAL_UINT16(STUN_METHOD_BINDING, stun_get_method_str(buf, len)); +} + +static void test_init_indication_is_not_request(void) { + uint8_t buf[1024] = {0}; + size_t len = 0; + + stun_init_indication_str(STUN_METHOD_BINDING, buf, &len); + + TEST_ASSERT_TRUE(stun_is_command_message_str(buf, len)); + TEST_ASSERT_FALSE(stun_is_request_str(buf, len)); + TEST_ASSERT_TRUE(stun_is_indication_str(buf, len)); +} + +static void test_success_response_carries_transaction_id(void) { + uint8_t req[1024] = {0}; + size_t req_len = 0; + stun_init_request_str(STUN_METHOD_ALLOCATE, req, &req_len); + + stun_tid tid = {0}; + stun_tid_from_message_str(req, req_len, &tid); + + uint8_t resp[1024] = {0}; + size_t resp_len = 0; + stun_init_success_response_str(STUN_METHOD_ALLOCATE, resp, &resp_len, &tid); + + TEST_ASSERT_TRUE(stun_is_success_response_str(resp, resp_len)); + TEST_ASSERT_EQUAL_UINT16(STUN_METHOD_ALLOCATE, stun_get_method_str(resp, resp_len)); + + stun_tid resp_tid = {0}; + stun_tid_from_message_str(resp, resp_len, &resp_tid); + TEST_ASSERT_EQUAL_MEMORY(tid.tsx_id, resp_tid.tsx_id, STUN_TID_SIZE); +} + +static void test_error_response_carries_error_code(void) { + uint8_t buf[1024] = {0}; + size_t len = 0; + stun_tid tid = {0}; + + stun_init_error_response_str(STUN_METHOD_ALLOCATE, buf, &len, 401, (const uint8_t *)"Unauthorized", &tid, true); + + TEST_ASSERT_TRUE(stun_is_command_message_str(buf, len)); + + int err_code = 0; + uint8_t err_msg[128] = {0}; + TEST_ASSERT_TRUE(stun_is_error_response_str(buf, len, &err_code, err_msg, sizeof(err_msg))); + TEST_ASSERT_EQUAL_INT(401, err_code); +} + +static void test_truncated_buffer_is_not_command_message(void) { + uint8_t buf[10] = {0}; + TEST_ASSERT_FALSE(stun_is_command_message_str(buf, sizeof(buf))); +} + +static void test_zeroed_buffer_is_not_command_message(void) { + uint8_t buf[STUN_HEADER_LENGTH] = {0}; + TEST_ASSERT_FALSE(stun_is_command_message_str(buf, sizeof(buf))); +} + +static void test_channel_message_roundtrip(void) { + uint8_t buf[1024] = {0}; + size_t len = 0; + const uint16_t channel = 0x4000; + const int payload_len = 200; + + TEST_ASSERT_TRUE(stun_init_channel_message_str(channel, buf, &len, payload_len, false)); + + uint16_t parsed_channel = 0; + size_t blen = len; + TEST_ASSERT_TRUE(stun_is_channel_message_str(buf, &blen, &parsed_channel, false)); + TEST_ASSERT_EQUAL_UINT16(channel, parsed_channel); +} + +int main(void) { + UNITY_BEGIN(); + RUN_TEST(test_init_request_produces_valid_stun_header); + RUN_TEST(test_init_indication_is_not_request); + RUN_TEST(test_success_response_carries_transaction_id); + RUN_TEST(test_error_response_carries_error_code); + RUN_TEST(test_truncated_buffer_is_not_command_message); + RUN_TEST(test_zeroed_buffer_is_not_command_message); + RUN_TEST(test_channel_message_roundtrip); + return UNITY_END(); +}