From 71ec4ae8657587fb1b833c1d7e5457c7cea1173b Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 12:32:08 +0000 Subject: [PATCH 01/14] build: add unit-test harness (Catch2) and coverage tooling - New CMake options MADS_BUILD_TESTS and MADS_COVERAGE (both OFF by default) - tests/: Catch2 v3.7.1 via FetchContent, one executable per test_*.cpp, shared helpers (loopback endpoints, RunningGuard, wait_for) and README - Coverage: per-target instrumentation of MadsCore, gcovr.cfg with curated denominator, coverage CMake target, codecov.yml (informational for now) - CI: .github/workflows/coverage.yml builds with clang, runs ctest and uploads to codecov.io (tolerates missing CODECOV_TOKEN) - Fix: exclude mongo_fetch.cpp from the build when MADS_ENABLE_MONGOCXX=OFF - Remove orphaned legacy test/ directory (referenced dead targets) --- .github/workflows/coverage.yml | 62 +++++++++++++++++++++++++ .gitignore | 9 +++- CMakeLists.txt | 58 +++++++++++++++++++++++ codecov.yml | 35 ++++++++++++++ gcovr.cfg | 24 ++++++++++ test/CMakeLists.txt | 22 --------- test/c_test.c | 65 -------------------------- test/client.cpp | 58 ----------------------- test/https.cpp | 85 ---------------------------------- test/server.cpp | 54 --------------------- tests/CMakeLists.txt | 51 ++++++++++++++++++++ tests/README.md | 40 ++++++++++++++++ tests/mads_test_helpers.hpp | 59 +++++++++++++++++++++++ tests/test_sanity.cpp | 11 +++++ 14 files changed, 348 insertions(+), 285 deletions(-) create mode 100644 .github/workflows/coverage.yml create mode 100644 codecov.yml create mode 100644 gcovr.cfg delete mode 100644 test/CMakeLists.txt delete mode 100644 test/c_test.c delete mode 100644 test/client.cpp delete mode 100644 test/https.cpp delete mode 100644 test/server.cpp create mode 100644 tests/CMakeLists.txt create mode 100644 tests/README.md create mode 100644 tests/mads_test_helpers.hpp create mode 100644 tests/test_sanity.cpp diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml new file mode 100644 index 0000000..21f9a99 --- /dev/null +++ b/.github/workflows/coverage.yml @@ -0,0 +1,62 @@ +name: Coverage + +on: + push: + branches: + - v2 + - code_coverage + pull_request: + branches: + - v2 + workflow_dispatch: + +jobs: + coverage: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # git describe needs the full tag history + + - name: Install tools + run: | + sudo apt-get update -qq + sudo apt-get install -y ninja-build gcovr clang llvm libclang-rt-dev libssl-dev + + - name: Cache FetchContent dependencies + uses: actions/cache@v4 + with: + path: build-cov/_deps + key: deps-coverage-${{ hashFiles('vendors/CMakeLists.txt', 'tests/CMakeLists.txt') }} + restore-keys: deps-coverage- + + - name: Configure + env: + CC: clang + CXX: clang++ + run: | + cmake -B build-cov -GNinja \ + -DCMAKE_BUILD_TYPE=Debug \ + -DMADS_BUILD_TESTS=ON \ + -DMADS_COVERAGE=ON \ + -DMADS_BUILD_APPS=OFF \ + -DMADS_DIRECTOR=OFF \ + -DMADS_ENABLE_MONGOCXX=OFF + + - name: Build + run: cmake --build build-cov -j$(nproc) + + - name: Test + run: ctest --test-dir build-cov --output-on-failure + + - name: Coverage report + run: | + gcovr --config gcovr.cfg --object-directory build-cov \ + --gcov-executable "llvm-cov gcov" + + - name: Upload to Codecov + uses: codecov/codecov-action@v5 + with: + files: coverage.xml + token: ${{ secrets.CODECOV_TOKEN }} + fail_ci_if_error: false # tolerate the token not being set yet diff --git a/.gitignore b/.gitignore index b1c1d76..3b55d90 100644 --- a/.gitignore +++ b/.gitignore @@ -35,4 +35,11 @@ TODO.md _*.md -/*.json \ No newline at end of file +/*.json +# Coverage artifacts +coverage.xml +coverage_html/ +*.gcda +*.gcno +*.gcov +*.profraw diff --git a/CMakeLists.txt b/CMakeLists.txt index 415c3c9..fc2ebb2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -82,6 +82,8 @@ include(GNUInstallDirs) option(MADS_ENABLE_MONGOCXX "Enable MongoDB C++ driver dependency" ON) option(MADS_BUILD_APPS "Build executables" ${PROJECT_IS_TOP_LEVEL}) option(MADS_DIRECTOR "Also build the director app (requires graphical interface)" ON) +option(MADS_BUILD_TESTS "Build unit tests (Catch2)" OFF) +option(MADS_COVERAGE "Instrument MadsCore and tests for coverage analysis" OFF) set(APP_NAME ${LIB_NAME}) set(PREFIX "mads") @@ -221,6 +223,12 @@ else() list(REMOVE_ITEM LIB_SOURCES ${SOURCE_DIR}/https_client_macos.mm) endif() +# mongo_fetch.cpp unconditionally includes , so it can only be +# part of the library when the MongoDB driver is actually vendored in. +if(NOT MADS_ENABLE_MONGOCXX) + list(REMOVE_ITEM LIB_SOURCES ${SOURCE_DIR}/mongo_fetch.cpp) +endif() + add_library( ${LIB_NAME} SHARED @@ -288,6 +296,18 @@ if(APPLE) endif() endif() +if(MADS_COVERAGE) + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") + # Instrument only MadsCore (PRIVATE flags): vendored dependencies stay out + # of the report and build fast. Atomic profile updates are required because + # Agent spawns drain/remote-control/watchdog threads. + target_compile_options(${LIB_NAME} PRIVATE -O0 -g --coverage -fprofile-update=atomic) + target_link_options(${LIB_NAME} PRIVATE --coverage) + else() + message(WARNING "MADS_COVERAGE requires GCC or Clang; coverage flags not applied") + endif() +endif() + add_library(MadsMainDeps INTERFACE) if(WIN32) target_compile_definitions(${LIB_NAME} PUBLIC NOMINMAX WIN32_LEAN_AND_MEAN _HAS_STD_BYTE=0) @@ -330,6 +350,44 @@ if(DEFINED _zmqpp_binary_dir AND _zmqpp_binary_dir) ) endif() +# +# _____ _ +# |_ _|__ ___| |_ ___ +# | |/ _ \/ __| __/ __| +# | | __/\__ \ |_\__ \ +# |_|\___||___/\__|___/ +# +# Note: enable_testing() is used directly (not include(CTest)) because +# vendors/CMakeLists.txt forces BUILD_TESTING OFF in the cache to keep +# third-party test suites out of the build. +if(MADS_BUILD_TESTS) + enable_testing() + add_subdirectory(tests) +endif() + +if(MADS_COVERAGE) + find_program(GCOVR_EXECUTABLE gcovr) + if(GCOVR_EXECUTABLE) + # clang emits gcov-compatible data but needs its own reader tool. + if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + set(MADS_GCOV_TOOL --gcov-executable "llvm-cov gcov") + else() + set(MADS_GCOV_TOOL) + endif() + add_custom_target(coverage + COMMAND ${CMAKE_CTEST_COMMAND} --test-dir ${CMAKE_BINARY_DIR} --output-on-failure + COMMAND ${GCOVR_EXECUTABLE} --config ${CMAKE_SOURCE_DIR}/gcovr.cfg + --object-directory ${CMAKE_BINARY_DIR} ${MADS_GCOV_TOOL} + --html-details ${CMAKE_BINARY_DIR}/coverage_html/index.html + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} + COMMENT "Running tests and generating coverage report" + VERBATIM + ) + else() + message(WARNING "gcovr not found: the 'coverage' target will not be available") + endif() +endif() + if(MADS_BUILD_APPS) include(FetchContent) FetchContent_Populate(plugin diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000..f03ace2 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,35 @@ +coverage: + precision: 1 + round: down + range: "60...85" + status: + project: + default: + target: 80% + threshold: 2% + informational: true # flip to false once the suite stabilizes + patch: + default: + target: 70% + informational: true + +# Curated coverage denominator: keep in lockstep with the excludes in gcovr.cfg +ignore: + - "vendors/**" + - "tests/**" + - "smoke_test/**" + - "tools/**" + - "rust/**" + - "src/stb_image.h" + - "src/service_discovery.cpp" + - "src/https_client*" + - "src/mongo_fetch.*" + - "src/metadata.hpp" + - "src/keypress.hpp" + - "src/image.hpp" + - "src/dummy.hpp" + - "src/logger.hpp" + - "src/TerminalLogoRenderer.hpp" + - "src/agent_app.hpp" + - "src/main/*.cpp" + - "src/plugin/**" diff --git a/gcovr.cfg b/gcovr.cfg new file mode 100644 index 0000000..4de130e --- /dev/null +++ b/gcovr.cfg @@ -0,0 +1,24 @@ +# Coverage report configuration (used by the `coverage` CMake target and CI). +# The exclude list below defines the curated coverage denominator and must be +# kept in lockstep with the `ignore` list in codecov.yml. +root = . +filter = src/ +# Vendored third-party code +exclude = src/stb_image.h +# Network / DB / hardware-bound units that cannot run in unit tests +exclude = src/service_discovery.cpp +exclude = src/https_client.* +exclude = src/mongo_fetch.* +exclude = src/metadata.hpp +exclude = src/keypress.hpp +exclude = src/image.hpp +exclude = src/dummy.hpp +exclude = src/logger.hpp +exclude = src/TerminalLogoRenderer.hpp +exclude = src/agent_app.hpp +# Executable glue and runtime-loaded plugins (integration-tested via smoke_test) +exclude = src/main/.*\.cpp +exclude = src/plugin/.* +print-summary = yes +xml = coverage.xml +xml-pretty = yes diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt deleted file mode 100644 index b6671a4..0000000 --- a/test/CMakeLists.txt +++ /dev/null @@ -1,22 +0,0 @@ -# _____ _ -# |_ _|__ ___| |_ _ __ _ __ ___ __ _ _ __ __ _ _ __ ___ ___ -# | |/ _ \/ __| __| | '_ \| '__/ _ \ / _` | '__/ _` | '_ ` _ \/ __| -# | | __/\__ \ |_ | |_) | | | (_) | (_| | | | (_| | | | | | \__ \ -# |_|\___||___/\__| | .__/|_| \___/ \__, |_| \__,_|_| |_| |_|___/ -# |_| |___/ -# This subproject builds test programs, not intended for installation. - -include_directories(${CMAKE_SOURCE_DIR}/src) - -# CURVE authentication test programs -add_executable(curve_server server.cpp) -target_link_libraries(curve_server PRIVATE ${LIB_LIST}) - -add_executable(curve_client client.cpp) -target_link_libraries(curve_client PRIVATE ${LIB_LIST}) - -add_executable(c_test c_test.c) -target_link_libraries(c_test PRIVATE mads-lib) - -add_executable(https https.cpp) -target_link_libraries(https PRIVATE madslib) \ No newline at end of file diff --git a/test/c_test.c b/test/c_test.c deleted file mode 100644 index b80dc8c..0000000 --- a/test/c_test.c +++ /dev/null @@ -1,65 +0,0 @@ -/* - ____ _ _ - / ___| / \ __ _ ___ _ __ | |_ - | | / _ \ / _` |/ _ \ '_ \| __| - | |___ / ___ \ (_| | __/ | | | |_ - \____| /_/ \_\__, |\___|_| |_|\__| - |___/ - -Test file -Paolo Bosetti 2026 -*/ - -#include "agent_c.h" -#include -#include -#ifndef _WIN32 -#include -#else -#include -#define sleep(o) Sleep(o) -#define usleep(o) Sleep(o) -#endif - -int main(int argc, const char **argv) { - agent_t agent = NULL; - printf("Using MADS library version %s\n", mads_version()); - if (argc == 1) - agent = agent_create("c_test", "tcp://localhost:9092"); - else - agent = agent_create("c_test", argv[1]); - - printf("Reading settings from %s\n", agent_settings_uri(agent)); - agent_set_settings_timeout(agent, 2000); - printf("Settings timeout: %d ms\n", agent_settings_timeout(agent)); - - if (agent_init(agent, false)) { - printf("Fatal : %s\n", agent_last_error(agent)); - return -1; - } - agent_set_id(agent, "Test C agent"); - agent_connect(agent, 1000); - agent_get_settings(agent, 0); - agent_print_settings(agent, 0); - agent_register_event(agent, mads_startup, - "{\"message\":\"Feedback agent started\"}"); - - int i = 0; - char j[256]; - for (i = 0; i < 10; i++) { - snprintf(j, 256, "{\"i\": %d}", i); - agent_publish(agent, "test", j); - usleep(100); - } - - int to = 2000; - printf("Waiting %d ms to receive a message\n", to); - agent_set_receive_timeout(agent, to); - message_type_t mt = agent_receive(agent, false); - printf("Message type: %d\n", mt); - - agent_register_event(agent, mads_shutdown, NULL); - sleep(1); - agent_disconnect(agent); - return 0; -} \ No newline at end of file diff --git a/test/client.cpp b/test/client.cpp deleted file mode 100644 index 1ae2b5a..0000000 --- a/test/client.cpp +++ /dev/null @@ -1,58 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include "curve.hpp" - - -using namespace std; - -#define CURVE_AUTH - -bool Running = true; - -int main(int argc, char *argv[]) { - string my_key = "client"; - if (argc > 1) { - my_key = argv[1]; - } - - // initialize the 0MQ context - zmqpp::context context; - ::signal(SIGTERM, [](int signum) { - cout << "Received signal " << signum << ", exiting." << endl; - Running = false; - }); - - // create and connect a client socket - zmqpp::socket publisher(context, zmqpp::socket_type::pub); - -#ifdef CURVE_AUTH - Mads::CurveAuth curve_auth(context); - curve_auth.allowed_ips.push_back("127.0.0.1"); - curve_auth.setup_auth(Mads::auth_verbose::on); - curve_auth.setup_curve_client(publisher, "client", "server"); -#endif - - publisher.connect("tcp://127.0.0.1:9000"); - - // Send a single message from server to publisher - size_t i = 0; - zmqpp::message request; - while (Running) { - request << "test" - << "Hello " + to_string(i++); - publisher.send(request); - this_thread::sleep_for(chrono::milliseconds(100)); - } - - publisher.disconnect("tcp://127.0.0.1:9000"); - publisher.close(); - - return 0; -} \ No newline at end of file diff --git a/test/https.cpp b/test/https.cpp deleted file mode 100644 index 084c2b2..0000000 --- a/test/https.cpp +++ /dev/null @@ -1,85 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include - -using namespace std; -using namespace rang; -using json = nlohmann::json; - -void describe_release(const json &release) { - std::string plat, arch; -#ifdef _WIN32 - plat = "Windows-"; -#elif defined(__APPLE__) - plat = "Darwin-"; -#else - plat = "Linux-"; -#endif - -#ifdef __x86_64__ - arch = "x86_64"; -#elif defined(__aarch64__) and defined(__APPLE__) - arch = "arm64"; -#elif defined(__aarch64__) - arch = "aarch64"; -#elif defined(__arm__) - arch = "arm"; -#elif defined(_M_X64) and defined(_WIN32) - arch = "AMD64"; -#elif defined(_M_X64) - arch = "x86_64"; -#elif defined(_M_ARM64) - arch = "aarch64"; -#else - arch = "unknown"; -#endif - if (release["prerelease"]) - cout << fg::yellow << "This is a pre-release!" << fg::reset << endl; - cout << release.value("name", "no name") << " (tag " << style::bold - << release.value("tag_name", "no tag") << style::reset << ") has " - << release["assets"].size() << " assets:" << endl; - for (json const a : release["assets"]) { - if (regex_match(a.value("name", ""), regex(".*" + plat + arch + ".*")) || - regex_match(a.value("name", ""), regex(".*" + plat + "universal.*"))) - cout << fg::green << "=> "; - else - cout << " - "; - cout << a.value("browser_download_url", "unnamed") << ", " - << a.value("size", 0) / 1024 << " kbytes" << fg::reset << endl; - } - cout << "Release URL: " << release.value("html_url", "unknown") << endl; -} - -int main(int argc, const char **argv) { - Mads::HttpsClient::Response response; - try { - Mads::HttpsClient client; - client.set_hostname("api.github.com"); - if (argc > 1) - client.set_path("/repos/pbosetti/mads/releases"); - else - client.set_path("/repos/pbosetti/mads/releases/latest"); - client.add_query_pair("per_page", "1"); - client.set_user_agent("MADS" LIB_VERSION); - - response = client.get(); - - json releases = json::parse(response.body); - if (releases.is_array()) - describe_release(releases[0]); - else - describe_release(releases); - - } catch (const json::exception &e) { - cerr << "Error parsing body: " << e.what() << "\n"; - // cerr << "body was: \n" << response.body << endl; - } catch (const std::exception &e) { - cerr << "Error: " << e.what() << "\n"; - } - - return 0; -} \ No newline at end of file diff --git a/test/server.cpp b/test/server.cpp deleted file mode 100644 index 9c0bcec..0000000 --- a/test/server.cpp +++ /dev/null @@ -1,54 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include -#include "curve.hpp" - -using namespace std; - -#define CURVE_AUTH - -bool Running = true; - -int main(int argc, char *argv[]) { - // initialize the 0MQ context - zmqpp::context context; - ::signal(SIGTERM, [](int signum) { - cout << "Received signal " << signum << ", exiting." << endl; - Running = false; - }); - -zmqpp::socket subscriber(context, zmqpp::socket_type::sub); - -#ifdef CURVE_AUTH - Mads::CurveAuth curve_auth(context); - curve_auth.allowed_ips.push_back("127.0.0.1"); - curve_auth.setup_auth(Mads::auth_verbose::on); - curve_auth.fetch_public_keys("."); - curve_auth.setup_curve_server(subscriber, "server"); -#endif - - subscriber.bind("tcp://*:9000"); - subscriber.subscribe("test"); - subscriber.set(zmqpp::socket_option::receive_timeout, 100); - - zmqpp::message response; - string msg_content; - string topic; - - while (Running) { - if (subscriber.receive(response)) { - response >> topic >> msg_content; - cout << "Topic: " << topic << endl; - cout << "Message Content: " << msg_content << endl; - } - } - - subscriber.unbind("tcp://*:9000"); - subscriber.close(); - - return 0; -} \ No newline at end of file diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt new file mode 100644 index 0000000..c7266fb --- /dev/null +++ b/tests/CMakeLists.txt @@ -0,0 +1,51 @@ +# +# _ _ _ _ _____ _ +# | | | |_ __ (_) |_ |_ _|__ ___| |_ ___ +# | | | | '_ \| | __| | |/ _ \/ __| __/ __| +# | |_| | | | | | |_ | | __/\__ \ |_\__ \ +# \___/|_| |_|_|\__| |_|\___||___/\__|___/ +# +# Catch2 is a test-only dependency: it is fetched here rather than in +# vendors/ so that it never enters the install/package set. +include(FetchContent) +FetchContent_Declare(catch2 + GIT_REPOSITORY https://github.com/catchorg/Catch2.git + GIT_TAG v3.7.1 + GIT_SHALLOW TRUE +) +FetchContent_MakeAvailable(catch2) +list(APPEND CMAKE_MODULE_PATH ${catch2_SOURCE_DIR}/extras) +include(Catch) + +add_library(mads_test_flags INTERFACE) +if(MADS_COVERAGE AND CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") + target_compile_options(mads_test_flags INTERFACE -O0 -g --coverage -fprofile-update=atomic) + target_link_options(mads_test_flags INTERFACE --coverage) +endif() + +# MadsCore leaves OpenSSL symbols (https_client) undefined on Linux; the +# executables in src/main link ssl/crypto themselves, and so must the tests. +if(UNIX AND NOT APPLE) + set(MADS_TEST_SSL_LIBS ssl crypto) +endif() + +# One test executable per test_*.cpp file: new suites are added by dropping a +# file in this directory, with no CMake edits required. +file(GLOB MADS_TEST_SOURCES ${CMAKE_CURRENT_LIST_DIR}/test_*.cpp) +foreach(test_src IN LISTS MADS_TEST_SOURCES) + get_filename_component(test_name ${test_src} NAME_WE) + add_executable(${test_name} ${test_src}) + target_link_libraries(${test_name} PRIVATE + Mads::Mads + MadsMainDeps + Catch2::Catch2WithMain + mads_test_flags + ${MADS_TEST_SSL_LIBS} + ) + target_include_directories(${test_name} PRIVATE ${CMAKE_SOURCE_DIR}/src) + target_compile_definitions(${test_name} PRIVATE + MADS_TEST_FIXTURES_DIR="${CMAKE_CURRENT_LIST_DIR}/fixtures" + MADS_PROJECT_SOURCE_DIR="${CMAKE_SOURCE_DIR}" + ) + catch_discover_tests(${test_name} PROPERTIES TIMEOUT 120) +endforeach() diff --git a/tests/README.md b/tests/README.md new file mode 100644 index 0000000..6a382bb --- /dev/null +++ b/tests/README.md @@ -0,0 +1,40 @@ +# MADS unit tests + +Unit tests use [Catch2 v3](https://github.com/catchorg/Catch2) and run under +CTest. They are self-contained: no broker, no MongoDB, no internet access. +Tests that exercise ZeroMQ messaging do so entirely in-process, over +`tcp://127.0.0.1` loopback sockets. + +## Building and running + +```sh +CC=clang CXX=clang++ cmake -B build-cov -GNinja \ + -DCMAKE_BUILD_TYPE=Debug \ + -DMADS_BUILD_TESTS=ON -DMADS_COVERAGE=ON \ + -DMADS_BUILD_APPS=OFF -DMADS_DIRECTOR=OFF -DMADS_ENABLE_MONGOCXX=OFF +cmake --build build-cov -j +ctest --test-dir build-cov --output-on-failure +``` + +With `MADS_COVERAGE=ON` and `gcovr` installed, a combined test + report run is +available as: + +```sh +cmake --build build-cov --target coverage +# report: build-cov/coverage_html/index.html and ./coverage.xml +``` + +The coverage denominator is curated in `gcovr.cfg` (mirrored by `codecov.yml`): +vendored code, network/DB/hardware-bound units, executable glue, and runtime +plugins are excluded. Integration-level behavior is covered separately by +`smoke_test/`. + +## Adding tests + +Drop a `test_.cpp` file in this directory — the build globs +`test_*.cpp` and creates one executable per file; no CMake edits are needed. +Fixture data goes in `tests/fixtures/` and is reachable at compile time via +the `MADS_TEST_FIXTURES_DIR` macro (`MADS_PROJECT_SOURCE_DIR` points at the +repository root). Shared conventions (loopback helper, port ranges per suite, +`RunningGuard` for `Agent::loop()` tests, `wait_for` polling) live in +`mads_test_helpers.hpp` — read it before writing a new suite. diff --git a/tests/mads_test_helpers.hpp b/tests/mads_test_helpers.hpp new file mode 100644 index 0000000..d29d5cf --- /dev/null +++ b/tests/mads_test_helpers.hpp @@ -0,0 +1,59 @@ +/* +Shared helpers for the MADS unit-test suite. + +Conventions for test authors: +- ZeroMQ endpoints must be in-process loopback only (tcp://127.0.0.1:). + inproc:// does NOT work between two Agent instances, because each Agent owns + its own zmqpp context. +- Tests run serially under ctest, but each suite must still use its own port + range to stay independent: + test_agent_* (pub/sub, wire, loop) 42100-42199 + test_agent_settings / broker 42200-42299 + test_agent_c / test_curve 42300-42399 + (spare) 42400-42499 +- Any test that touches Mads::Agent::loop() must instantiate RunningGuard, + since Mads::running is a process-global atomic. +*/ +#ifndef MADS_TEST_HELPERS_HPP +#define MADS_TEST_HELPERS_HPP + +#include +#include +#include +#include +#include +#include + +#include "../src/mads.hpp" + +namespace mads_test { + +inline std::string loopback(uint16_t port) { + return "tcp://127.0.0.1:" + std::to_string(port); +} + +// Restores the global run flag on scope exit so a test that stops the loop +// cannot poison later tests in the same binary. +struct RunningGuard { + RunningGuard() { Mads::running = true; } + ~RunningGuard() { Mads::running = true; } +}; + +// Polls a predicate until it returns true or the timeout expires. Use this +// instead of bare sleeps when waiting for messages or callbacks. +inline bool wait_for(const std::function &predicate, + std::chrono::milliseconds timeout = + std::chrono::milliseconds(2000), + std::chrono::milliseconds poll = + std::chrono::milliseconds(10)) { + auto const deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + if (predicate()) return true; + std::this_thread::sleep_for(poll); + } + return predicate(); +} + +} // namespace mads_test + +#endif // MADS_TEST_HELPERS_HPP diff --git a/tests/test_sanity.cpp b/tests/test_sanity.cpp new file mode 100644 index 0000000..b0af910 --- /dev/null +++ b/tests/test_sanity.cpp @@ -0,0 +1,11 @@ +#include + +#include "mads_test_helpers.hpp" + +TEST_CASE("Library version is available", "[sanity]") { + REQUIRE_FALSE(Mads::version().empty()); +} + +TEST_CASE("check_version accepts the library's own version", "[sanity]") { + REQUIRE(Mads::check_version(Mads::version())); +} From 74fb840e9c4141fab2d8544bf4fc2c52a7e0a5f4 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 12:37:10 +0000 Subject: [PATCH 02/14] test: add unit tests for mads core, execution-time stats, goback, exec_path, watcher Coverage (line %): mads.hpp 100% (46/46), execution_time_window_stats.hpp 100% (64/64), goback.hpp 100% (22/22), exec_path.hpp 84.6% (11/13), watcher.hpp 91.3% (21/23). --- tests/test_exec_path.cpp | 39 +++++ tests/test_execution_time_window_stats.cpp | 146 +++++++++++++++++ tests/test_goback.cpp | 72 +++++++++ tests/test_mads_core.cpp | 177 +++++++++++++++++++++ tests/test_watcher.cpp | 121 ++++++++++++++ 5 files changed, 555 insertions(+) create mode 100644 tests/test_exec_path.cpp create mode 100644 tests/test_execution_time_window_stats.cpp create mode 100644 tests/test_goback.cpp create mode 100644 tests/test_mads_core.cpp create mode 100644 tests/test_watcher.cpp diff --git a/tests/test_exec_path.cpp b/tests/test_exec_path.cpp new file mode 100644 index 0000000..d2bb041 --- /dev/null +++ b/tests/test_exec_path.cpp @@ -0,0 +1,39 @@ +#include + +#include + +#include "exec_path.hpp" + +namespace fs = std::filesystem; + +TEST_CASE("exec_path returns an existing file named after the test binary", + "[exec_path]") { + fs::path p = Mads::exec_path(); + REQUIRE(fs::exists(p)); + REQUIRE(p.is_absolute()); + REQUIRE(p.filename() == "test_exec_path"); +} + +TEST_CASE("exec_dir with no argument returns the binary's parent directory", + "[exec_path]") { + std::string dir = Mads::exec_dir(); + fs::path expected = Mads::exec_path().parent_path(); + REQUIRE(fs::path(dir) == fs::weakly_canonical(expected)); + REQUIRE(fs::is_directory(dir)); +} + +TEST_CASE("exec_dir with a relative subpath composes under the binary's directory", + "[exec_path]") { + std::string dir = Mads::exec_dir(); + std::string composed = Mads::exec_dir("subdir/file.txt"); + fs::path expected = fs::weakly_canonical(fs::path(dir) / "subdir/file.txt"); + REQUIRE(fs::path(composed) == expected); +} + +TEST_CASE("prefix returns the parent of the binary's parent directory", + "[exec_path]") { + fs::path expected = + fs::weakly_canonical(fs::path(Mads::exec_dir()) / ".."); + fs::path actual = Mads::prefix(); + REQUIRE(actual == expected); +} diff --git a/tests/test_execution_time_window_stats.cpp b/tests/test_execution_time_window_stats.cpp new file mode 100644 index 0000000..d025342 --- /dev/null +++ b/tests/test_execution_time_window_stats.cpp @@ -0,0 +1,146 @@ +#include +#include + +#include +#include +#include + +#include "execution_time_window_stats.hpp" + +using namespace std::chrono_literals; + +TEST_CASE("toc without tic returns 0 and stores nothing", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(10); + double elapsed = stats.toc(); + REQUIRE(elapsed == 0.0); + REQUIRE(stats.size() == 0); + REQUIRE(stats.average_ms() == 0.0); + REQUIRE(stats.stddev_ms() == 0.0); +} + +TEST_CASE("tic/toc records a sample with a plausible elapsed time", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(10); + stats.tic(); + std::this_thread::sleep_for(5ms); + double elapsed = stats.toc(); + + // Should be positive and in a generous range (avoid exact-time assertions + // under CI jitter). + REQUIRE(elapsed > 0.0); + REQUIRE(elapsed < 2000.0); + REQUIRE(stats.size() == 1); + REQUIRE(stats.average_ms() == elapsed); + REQUIRE(stats.stddev_ms() == Catch::Approx(0.0).margin(1e-6)); +} + +TEST_CASE("mean and stddev over several tic/toc cycles are in range", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(50); + for (int i = 0; i < 5; ++i) { + stats.tic(); + std::this_thread::sleep_for(3ms); + stats.toc(); + } + REQUIRE(stats.size() == 5); + // Mean should be a small positive number, comfortably below a generous + // upper bound to absorb CI jitter. + REQUIRE(stats.average_ms() > 0.0); + REQUIRE(stats.average_ms() < 2000.0); + // Stddev is non-negative and bounded by the same generosity. + REQUIRE(stats.stddev_ms() >= 0.0); + REQUIRE(stats.stddev_ms() < 2000.0); +} + +TEST_CASE("window trims oldest samples once more than window_width added", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(3); + // Feed synthetic durations directly via tic/toc is timing-dependent for + // exact values, so instead verify the trimming *mechanism*: size() never + // exceeds window_width, and average reflects only the most recent samples. + for (int i = 0; i < 6; ++i) { + stats.tic(); + std::this_thread::sleep_for(1ms); + stats.toc(); + REQUIRE(stats.size() <= 3); + } + REQUIRE(stats.size() == 3); + REQUIRE(stats.window_width() == 3); +} + +TEST_CASE("set_window_width shrinking drops oldest samples immediately", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(10); + for (int i = 0; i < 5; ++i) { + stats.tic(); + std::this_thread::sleep_for(1ms); + stats.toc(); + } + REQUIRE(stats.size() == 5); + + stats.set_window_width(2); + REQUIRE(stats.window_width() == 2); + REQUIRE(stats.size() == 2); + + // Growing the window again should not resurrect dropped samples. + stats.set_window_width(10); + REQUIRE(stats.window_width() == 10); + REQUIRE(stats.size() == 2); +} + +TEST_CASE("zero window width discards every sample", "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(0); + stats.tic(); + std::this_thread::sleep_for(1ms); + double elapsed = stats.toc(); + // toc() itself still reports the elapsed time... + REQUIRE(elapsed > 0.0); + // ...but nothing is retained because the window width is zero. + REQUIRE(stats.size() == 0); + REQUIRE(stats.average_ms() == 0.0); + REQUIRE(stats.stddev_ms() == 0.0); +} + +TEST_CASE("set_window_width(0) on a populated window empties it", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(5); + for (int i = 0; i < 3; ++i) { + stats.tic(); + std::this_thread::sleep_for(1ms); + stats.toc(); + } + REQUIRE(stats.size() == 3); + stats.set_window_width(0); + REQUIRE(stats.size() == 0); + REQUIRE(stats.average_ms() == 0.0); +} + +TEST_CASE("clear resets samples and pending tic state", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats(10); + stats.tic(); + std::this_thread::sleep_for(1ms); + stats.toc(); + REQUIRE(stats.size() == 1); + + stats.clear(); + REQUIRE(stats.size() == 0); + REQUIRE(stats.average_ms() == 0.0); + REQUIRE(stats.stddev_ms() == 0.0); + + // A tic() started before clear() must not leak into a toc() called after: + // clear() resets the ticking flag, so a bare toc() after clear() (with no + // fresh tic()) returns 0 and stores nothing. + stats.tic(); + stats.clear(); + double elapsed = stats.toc(); + REQUIRE(elapsed == 0.0); + REQUIRE(stats.size() == 0); +} + +TEST_CASE("default construction uses a window width of 100", + "[execution_time_window_stats]") { + Mads::ExecutionTimeWindowStats stats; + REQUIRE(stats.window_width() == 100); +} diff --git a/tests/test_goback.cpp b/tests/test_goback.cpp new file mode 100644 index 0000000..2992d79 --- /dev/null +++ b/tests/test_goback.cpp @@ -0,0 +1,72 @@ +#include + +#include +#include + +#include "goback.hpp" + +TEST_CASE("GoBack with zero lines is a no-op", "[goback]") { + std::ostringstream os; + os << Mads::GoBack(0); + REQUIRE(os.str().empty()); +} + +TEST_CASE("goback(0, true) produces zero lines and is a no-op", "[goback]") { + std::ostringstream os; + os << Mads::goback(0, true); + REQUIRE(os.str().empty()); +} + +TEST_CASE("goback(n, false) is disabled and produces no output regardless of n", + "[goback]") { + std::ostringstream os; + os << Mads::goback(5, false); + REQUIRE(os.str().empty()); +} + +TEST_CASE("GoBack(1) emits a single cursor-up + erase-line sequence", + "[goback]") { + std::ostringstream os; + os << Mads::GoBack(1); + // One repetition of "\x1b[1A\x1b[2K" (no embedded '\r' since it's the last + // line), followed by a trailing '\r'. + REQUIRE(os.str() == "\x1b[1A\x1b[2K\r"); +} + +TEST_CASE("GoBack(n) repeats the escape sequence n times with separators", + "[goback]") { + std::ostringstream os; + os << Mads::GoBack(3); + std::string expected = + "\x1b[1A\x1b[2K\r" + "\x1b[1A\x1b[2K\r" + "\x1b[1A\x1b[2K" + "\r"; + REQUIRE(os.str() == expected); +} + +TEST_CASE("goback(n, true) matches GoBack(n) output", "[goback]") { + std::ostringstream os1; + std::ostringstream os2; + os1 << Mads::GoBack(4); + os2 << Mads::goback(4, true); + REQUIRE(os1.str() == os2.str()); +} + +TEST_CASE("GoBack exposes its configured line count", "[goback]") { + Mads::GoBack gb(7); + REQUIRE(gb.lines() == 7); + + Mads::GoBack disabled = Mads::goback(9, false); + REQUIRE(disabled.lines() == 0); +} + +TEST_CASE("GoBack streamed twice accumulates independent output", + "[goback]") { + std::ostringstream os; + os << Mads::GoBack(1) << "middle" << Mads::GoBack(2); + std::string expected = "\x1b[1A\x1b[2K\r" + "middle" + "\x1b[1A\x1b[2K\r\x1b[1A\x1b[2K\r"; + REQUIRE(os.str() == expected); +} diff --git a/tests/test_mads_core.cpp b/tests/test_mads_core.cpp new file mode 100644 index 0000000..351223e --- /dev/null +++ b/tests/test_mads_core.cpp @@ -0,0 +1,177 @@ +#include +#include + +#include +#include +#include +#include +#include + +#include "mads_test_helpers.hpp" + +// --------------------------------------------------------------------------- +// version() / check_version() +// --------------------------------------------------------------------------- + +TEST_CASE("version returns the library version string", "[mads_core]") { + REQUIRE(Mads::version() == std::string(LIB_VERSION)); +} + +TEST_CASE("check_version matches on identical minor version", "[mads_core]") { + // The library's own version always matches its own check. + REQUIRE(Mads::check_version(Mads::version())); + // Same major.minor, different patch also matches, since only the part + // before the last dot is compared. + REQUIRE(Mads::check_version("v0.0.999")); +} + +TEST_CASE("check_version rejects a non-matching minor version", + "[mads_core]") { + REQUIRE_FALSE(Mads::check_version("v9.9.9")); + REQUIRE_FALSE(Mads::check_version("v1.0.1")); +} + +TEST_CASE("check_version throws on a dot-less version string", + "[mads_core]") { + REQUIRE_THROWS_AS(Mads::check_version("noversion"), std::runtime_error); + REQUIRE_THROWS_AS(Mads::check_version(""), std::runtime_error); +} + +// --------------------------------------------------------------------------- +// event_name() / event_map +// --------------------------------------------------------------------------- + +TEST_CASE("event_map contains every event_type with a readable name", + "[mads_core]") { + REQUIRE(Mads::event_map.size() == 6); + REQUIRE(Mads::event_name(Mads::event_type::marker) == "marker"); + REQUIRE(Mads::event_name(Mads::event_type::marker_in) == "marker in"); + REQUIRE(Mads::event_name(Mads::event_type::marker_out) == "marker out"); + REQUIRE(Mads::event_name(Mads::event_type::startup) == "startup"); + REQUIRE(Mads::event_name(Mads::event_type::shutdown) == "shutdown"); + REQUIRE(Mads::event_name(Mads::event_type::message) == "message"); +} + +// --------------------------------------------------------------------------- +// get_ISODate_time() +// --------------------------------------------------------------------------- + +TEST_CASE("get_ISODate_time produces the expected shape", "[mads_core]") { + auto now = std::chrono::system_clock::now(); + std::string s = Mads::get_ISODate_time(now); + + // "YYYY-MM-DDTHH:MM:SS.mmm+HHMM" (timezone suffix optional/platform + // dependent, but the date/time/millis prefix is fixed width). + std::regex re(R"(^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3})"); + REQUIRE(std::regex_search(s, re)); +} + +TEST_CASE("get_ISODate_time pads milliseconds to 3 digits", "[mads_core]") { + using namespace std::chrono; + // Construct a time_point at an exact second boundary, then add a small + // millisecond offset via the `offset` parameter to control the ms value. + auto now = system_clock::now(); + auto truncated = floor(now); + + std::string s5 = Mads::get_ISODate_time(truncated, 5); + std::string ms_part5 = s5.substr(20, 3); // after "...SS." + REQUIRE(ms_part5 == "005"); + + std::string s0 = Mads::get_ISODate_time(truncated, 0); + std::string ms_part0 = s0.substr(20, 3); + REQUIRE(ms_part0 == "000"); + + std::string s123 = Mads::get_ISODate_time(truncated, 123); + std::string ms_part123 = s123.substr(20, 3); + REQUIRE(ms_part123 == "123"); +} + +// --------------------------------------------------------------------------- +// timecode() +// --------------------------------------------------------------------------- + +TEST_CASE("timecode bins milliseconds to the fps grid", "[mads_core]") { + using namespace std::chrono; + + // Build a time_point for today at a known local wall-clock second, then + // add a millisecond offset to check the binning behavior. + auto now = system_clock::now(); + time_t now_c = system_clock::to_time_t(now); + auto sec_point = system_clock::from_time_t(now_c); + + // fps = 25 -> bin width 40ms. 47ms should bin down to 40ms -> 0.04s + // fractional part. + auto with_ms = sec_point + milliseconds(47); + double tc = Mads::timecode(with_ms, 25); + double frac = tc - std::floor(tc); + // 47 / 40 = 1.175 -> floor -> 1 -> 1*40 = 40ms = 0.04s + REQUIRE(frac == Catch::Approx(0.04).margin(0.001)); + + // 0ms should bin to 0. + auto with_zero = sec_point + milliseconds(0); + double tc0 = Mads::timecode(with_zero, 25); + double frac0 = tc0 - std::floor(tc0); + REQUIRE(frac0 == Catch::Approx(0.0).margin(0.001)); +} + +TEST_CASE("timecode integer part matches local wall-clock seconds-of-day", + "[mads_core]") { + using namespace std::chrono; + auto now = system_clock::now(); + time_t now_c = system_clock::to_time_t(now); + tm *lt = localtime(&now_c); + double expected_seconds_of_day = + lt->tm_hour * 3600 + lt->tm_min * 60 + lt->tm_sec; + + double tc = Mads::timecode(system_clock::from_time_t(now_c), 25); + REQUIRE(std::floor(tc) == Catch::Approx(expected_seconds_of_day)); +} + +// --------------------------------------------------------------------------- +// tv_to_milliseconds() / milliseconds_to_tv() round-trip +// --------------------------------------------------------------------------- + +#ifndef _WIN32 +TEST_CASE("tv_to_milliseconds converts a timeval to milliseconds", + "[mads_core]") { + struct timeval tv; + tv.tv_sec = 2; + tv.tv_usec = 500000; // 0.5s + auto ms = Mads::tv_to_milliseconds(tv); + REQUIRE(ms.count() == 2500); +} + +TEST_CASE("milliseconds_to_tv converts milliseconds to a timeval", + "[mads_core]") { + struct timeval tv{}; + Mads::milliseconds_to_tv(std::chrono::milliseconds(2500), tv); + REQUIRE(tv.tv_sec == 2); + REQUIRE(tv.tv_usec == 500000); +} + +TEST_CASE("tv_to_milliseconds / milliseconds_to_tv round-trip", + "[mads_core]") { + std::vector values_ms = {0, 1, 999, 1000, 1001, 123456, 7}; + for (auto v : values_ms) { + struct timeval tv{}; + Mads::milliseconds_to_tv(std::chrono::milliseconds(v), tv); + auto back = Mads::tv_to_milliseconds(tv); + REQUIRE(back.count() == v); + } +} +#endif + +// --------------------------------------------------------------------------- +// AgentError +// --------------------------------------------------------------------------- + +TEST_CASE("AgentError carries and reports its message", "[mads_core]") { + Mads::AgentError err("something went wrong"); + REQUIRE(std::string(err.what()) == "something went wrong"); + + try { + throw Mads::AgentError("boom"); + } catch (const std::exception &e) { + REQUIRE(std::string(e.what()) == "boom"); + } +} diff --git a/tests/test_watcher.cpp b/tests/test_watcher.cpp new file mode 100644 index 0000000..d99329a --- /dev/null +++ b/tests/test_watcher.cpp @@ -0,0 +1,121 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include "mads_test_helpers.hpp" +#include "watcher.hpp" + +namespace fs = std::filesystem; + +namespace { + +// Returns a fresh, non-colliding path under the system temp directory. Each +// call bumps a static counter so tests run in the same process never clash. +fs::path unique_temp_file(const std::string &tag) { + static std::atomic counter{0}; + auto n = counter.fetch_add(1); + auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); + return fs::temp_directory_path() / + ("mads_watcher_" + tag + "_" + std::to_string(stamp) + "_" + + std::to_string(n) + ".txt"); +} + +} // namespace + +TEST_CASE("Watcher can be constructed and destroyed without watching", + "[watcher]") { + auto file_path = unique_temp_file("ctor"); + { + std::ofstream ofs(file_path); + ofs << "initial\n"; + } + + { + Mads::Watcher watcher(file_path.string()); + // Constructor/destructor only: no watch() call, so this must not hang + // and must not throw. + } + + fs::remove(file_path); + SUCCEED("constructed and destroyed cleanly"); +} + +// NOTE: Watcher::watch() runs an unconditional `while (true)` loop with no +// stop/cancel primitive in this header. To drive it at all we must run it on +// a background thread and detach it (the process exits shortly after the +// test binary finishes, which reclaims the thread). Because the thread +// outlives the TEST_CASE scope, the Watcher and callback state below are +// deliberately heap-allocated and intentionally never freed: destroying them +// while the detached thread might still be reading from them would be a +// use-after-free. This is a test-only leak, acceptable for a short-lived +// test binary. + +TEST_CASE("Watcher invokes the callback when the watched file is modified", + "[watcher]") { + auto file_path = unique_temp_file("fire"); + { + std::ofstream ofs(file_path); + ofs << "initial\n"; + } + + auto *watcher = new Mads::Watcher(file_path.string()); + auto *called = new std::atomic(false); + auto *received = new std::string(); + + std::thread t([watcher, called, received]() { + watcher->watch([called, received](const std::string &fn) { + *received = fn; + called->store(true); + }); + }); + t.detach(); + + // Let the watch thread start and the OS-level watch (inotify/kqueue/etc.) + // be armed before we mutate the file. + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + + { + std::ofstream ofs(file_path, std::ios::app); + ofs << "modified\n"; + ofs.flush(); + } + + bool fired = mads_test::wait_for([called]() { return called->load(); }, + std::chrono::milliseconds(2000)); + REQUIRE(fired); + REQUIRE(*received == file_path.string()); + + fs::remove(file_path); +} + +TEST_CASE("Watcher does not fire when the file is left untouched", + "[watcher]") { + auto file_path = unique_temp_file("silent"); + { + std::ofstream ofs(file_path); + ofs << "initial\n"; + } + + auto *watcher = new Mads::Watcher(file_path.string()); + auto *called = new std::atomic(false); + + std::thread t([watcher, called]() { + watcher->watch( + [called](const std::string &) { called->store(true); }); + }); + t.detach(); + + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + + // No modification: the callback must not fire within a generous window. + bool fired = mads_test::wait_for([called]() { return called->load(); }, + std::chrono::milliseconds(600)); + REQUIRE_FALSE(fired); + + fs::remove(file_path); +} From f53c4adf57091edd458a9f40d465c115159922bb Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 12:57:40 +0000 Subject: [PATCH 03/14] test: add unit tests for Agent pub/sub, wire codec, loop/remote_control, and LazyPayload Covers src/agent.hpp's LazyPayload text/doc lazy caching (test_lazy_payload.cpp); Agent pub/sub messaging, blob publishing, topic filtering, accessors, error paths, disconnect/shutdown safety, and Delivery::LastKnownValue over an in-process ZeroMQ loopback (test_agent_pubsub.cpp); the WireFormat x Compression codec matrix, the Compression::Auto threshold, a large Compression::Snappy round trip, and dropped_messages() on malformed frames injected via a raw zmqpp socket (test_agent_wire.cpp); and Agent::loop() pacing/exception handling plus remote_control()'s restart/shutdown/info/no-op commands (test_agent_loop.cpp). All ports are in the assigned 42100-42199 range. Coverage (ctest --test-dir build-cov, full 82-test suite, gcovr): src/agent.cpp 58.5% lines (531/908) src/agent.hpp 96.4% lines (27/28) --- tests/test_agent_loop.cpp | 252 +++++++++++++++++++++++ tests/test_agent_pubsub.cpp | 388 ++++++++++++++++++++++++++++++++++++ tests/test_agent_wire.cpp | 264 ++++++++++++++++++++++++ tests/test_lazy_payload.cpp | 127 ++++++++++++ 4 files changed, 1031 insertions(+) create mode 100644 tests/test_agent_loop.cpp create mode 100644 tests/test_agent_pubsub.cpp create mode 100644 tests/test_agent_wire.cpp create mode 100644 tests/test_lazy_payload.cpp diff --git a/tests/test_agent_loop.cpp b/tests/test_agent_loop.cpp new file mode 100644 index 0000000..cceaf1f --- /dev/null +++ b/tests/test_agent_loop.cpp @@ -0,0 +1,252 @@ +// Unit tests for Mads::Agent's main loop machinery and remote_control() +// (src/agent.cpp ~995-1120). Every TEST_CASE that touches Agent::loop() or +// remote_control()'s effect on Mads::running instantiates +// mads_test::RunningGuard, since Mads::running is a process-global atomic +// (src/mads.hpp.in:193). +// +// The watchdog *firing* path (install_loop_watchdog()'s force-exit branch) +// is intentionally not exercised: it calls std::_Exit() and would kill the +// test binary. install_loop_watchdog() itself is exercised once, in a way +// that never lets the countdown start (Mads::running stays true throughout). +#include + +#include +#include +#include + +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +// --------------------------------------------------------------------------- +// loop() +// --------------------------------------------------------------------------- + +TEST_CASE("loop() invokes the lambda repeatedly until Mads::running is " + "cleared", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopA", "none"); + a.init(false, false); + + int count = 0; + const int target = 5; + a.loop( + [&]() -> std::chrono::nanoseconds { + ++count; + if (count >= target) + Mads::running = false; + return std::chrono::nanoseconds(0); + }, + std::chrono::nanoseconds(0)); + + REQUIRE(count == target); +} + +TEST_CASE("loop() paces iterations by the requested duration", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopB", "none"); + a.init(false, false); + + const auto period = std::chrono::milliseconds(20); + const int target = 5; + int count = 0; + auto t0 = std::chrono::steady_clock::now(); + a.loop( + [&]() -> std::chrono::nanoseconds { + ++count; + if (count >= target) + Mads::running = false; + return std::chrono::nanoseconds(0); // keep using the fixed `period` + }, + period); + auto elapsed = std::chrono::steady_clock::now() - t0; + + REQUIRE(count == target); + // 5 iterations at ~20ms apiece must take noticeably longer than max-speed + // (which would complete in well under 1ms); generous bounds avoid flakiness + // while still proving pacing is actually happening. + REQUIRE(elapsed >= 60ms); + REQUIRE(elapsed < 5000ms); +} + +TEST_CASE("loop()'s lambda-returned duration overrides the fixed duration " + "on subsequent iterations", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopE", "none"); + a.init(false, false); + + int count = 0; + auto t0 = std::chrono::steady_clock::now(); + // The wait before each iteration is picked from the *previous* iteration's + // returned duration (falling back to the fixed `duration` argument only + // while that is still zero, i.e. on the very first iteration). So with a + // fixed 1s duration and a lambda that requests 500us from then on, total + // runtime should be roughly one second (one long wait), not five. + a.loop( + [&]() -> std::chrono::nanoseconds { + ++count; + if (count >= 5) + Mads::running = false; + return std::chrono::microseconds(500); + }, + std::chrono::seconds(1)); + auto elapsed = std::chrono::steady_clock::now() - t0; + + REQUIRE(count == 5); + REQUIRE(elapsed >= 900ms); // the first iteration's wait used the 1s duration + REQUIRE(elapsed < 2500ms); // but not all five (that would take >=4s) +} + +TEST_CASE("loop() catches an exception thrown by the lambda and stops " + "Mads::running", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopF", "none"); + a.init(false, false); + + int count = 0; + a.loop( + [&]() -> std::chrono::nanoseconds { + ++count; + throw std::runtime_error("boom"); + }, + std::chrono::nanoseconds(0)); + + REQUIRE(count == 1); // the loop exits after the first, throwing iteration + REQUIRE_FALSE(Mads::running.load()); +} + +// --------------------------------------------------------------------------- +// high_res_loop +// --------------------------------------------------------------------------- + +TEST_CASE("enable_high_res_loop toggles high_res_loop()", "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopC", "none"); + a.init(false, false); + + REQUIRE_FALSE(a.high_res_loop()); // default off + a.enable_high_res_loop(true, std::chrono::microseconds(500)); + REQUIRE(a.high_res_loop()); + a.enable_high_res_loop(false); + REQUIRE_FALSE(a.high_res_loop()); +} + +// --------------------------------------------------------------------------- +// install_loop_watchdog(): install/orderly-stop path only (never trips) +// --------------------------------------------------------------------------- + +TEST_CASE("install_loop_watchdog() starts and joins cleanly on an orderly " + "shutdown", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("loopD", "none"); + // Default install_watchdog=true exercises install_loop_watchdog(). + // Mads::running stays true for the whole test (RunningGuard), so the + // watchdog's force-exit countdown never starts. + a.init(); + a.shutdown(); // stops and joins the watchdog thread promptly + SUCCEED("watchdog thread installed and joined without tripping"); +} + +// --------------------------------------------------------------------------- +// remote_control() +// --------------------------------------------------------------------------- + +TEST_CASE("remote_control(\"restart\") sets restart() and clears " + "Mads::running", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("rcA", "none"); + a.init(false, false); + + REQUIRE_FALSE(a.restart()); + a.remote_control(R"({"cmd":"restart"})"); + REQUIRE(a.restart()); + REQUIRE_FALSE(Mads::running.load()); +} + +TEST_CASE("remote_control(\"shutdown\") clears Mads::running without " + "setting restart()", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("rcB", "none"); + a.init(false, false); + + a.remote_control(R"({"cmd":"shutdown"})"); + REQUIRE_FALSE(a.restart()); + REQUIRE_FALSE(Mads::running.load()); +} + +TEST_CASE("remote_control() with malformed JSON is a no-op that leaves " + "Mads::running set", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("rcC", "none"); + a.init(false, false); + + a.remote_control("not json at all"); + REQUIRE(Mads::running.load()); + REQUIRE_FALSE(a.restart()); +} + +TEST_CASE("remote_control() with an unrecognized cmd is a no-op", + "[agent_loop]") { + mads_test::RunningGuard guard; + Mads::Agent a("rcE", "none"); + a.init(false, false); + + a.remote_control(R"({"cmd":"frobnicate"})"); + REQUIRE(Mads::running.load()); + REQUIRE_FALSE(a.restart()); +} + +TEST_CASE("remote_control(\"info\") publishes the agent's settings on the " + "\"info\" topic", + "[agent_loop]") { + mads_test::RunningGuard guard; + const uint16_t port = 42150; + + auto pub = std::make_unique("rcD", "none"); + pub->init(false, false); + pub->set_cross(true); + pub->set_sub_endpoint(mads_test::loopback(port)); + pub->set_sub_topic({}); + pub->connect(std::chrono::milliseconds(0)); + + auto sub = std::make_unique("rcDsub", "none"); + sub->init(false, false); + sub->set_sub_endpoint(mads_test::loopback(port)); + sub->set_pub_topic(""); + sub->set_sub_topic({""}); + sub->connect(std::chrono::milliseconds(0)); + + // Warm up the loopback pair (absorbs the ZMQ "slow joiner" delay) before + // relying on remote_control()'s internal publish() call. + bool warm = mads_test::wait_for( + [&] { + pub->publish(nlohmann::json{{"warm", 1}}, "warm"); + return mads_test::wait_for( + [&] { return sub->receive(true) == Mads::message_type::json; }, + 150ms, 10ms); + }, + 3000ms, 200ms); + REQUIRE(warm); + + pub->remote_control(R"({"cmd":"info"})"); + + bool got = mads_test::wait_for( + [&] { return sub->receive(true) == Mads::message_type::json; }, 2000ms, + 20ms); + REQUIRE(got); + auto [topic, doc] = sub->last_json(); + REQUIRE(topic == "info"); + REQUIRE(doc.at("agent") == "rcD"); + REQUIRE(Mads::running.load()); // "info" must not touch the running flag +} diff --git a/tests/test_agent_pubsub.cpp b/tests/test_agent_pubsub.cpp new file mode 100644 index 0000000..2f9f409 --- /dev/null +++ b/tests/test_agent_pubsub.cpp @@ -0,0 +1,388 @@ +// Unit tests for Mads::Agent pub/sub messaging (src/agent.cpp, src/agent.hpp) +// over an in-process ZeroMQ loopback pair (no broker). +// +// Topology used throughout: a "publisher" agent is initialized with +// settings_uri "none" (test mode, src/agent.hpp:232) and set_cross(true), +// which makes connect_pub() BIND using the port carried by _sub_endpoint +// (src/agent.cpp Agent::connect_pub). A plain (non-cross) "subscriber" agent +// then CONNECTs its _sub_endpoint to that same tcp://127.0.0.1:. Since +// init() overwrites _pub_endpoint/_sub_endpoint with defaults +// (src/agent.cpp:392-393), endpoint setters are always called AFTER init() +// and BEFORE connect(). +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +namespace { + +// Publisher: binds (cross=true) at loopback(port), publishing only. +std::unique_ptr make_pub(std::string name, uint16_t port) { + auto a = std::make_unique(name, "none"); + a->init(false, false); // crypto=false, install_watchdog=false + a->set_cross(true); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_sub_topic({}); // pure publisher: skip connect_sub entirely + a->connect(std::chrono::milliseconds(0)); + return a; +} + +// Subscriber: connects to loopback(port), subscribing only. +std::unique_ptr make_sub(std::string name, uint16_t port, + std::vector topics = {""}) { + auto a = std::make_unique(name, "none"); + a->init(false, false); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_pub_topic(""); // pure subscriber: skip connect_pub entirely + a->set_sub_topic(topics); + a->connect(std::chrono::milliseconds(0)); + return a; +} + +// Repeatedly invokes `publish_once` (re-publishing) until `check_received` +// becomes true, absorbing the ZMQ "slow joiner" propagation delay without a +// single fixed sleep as the sole synchronization mechanism. +template +bool retry_until(PublishFn publish_once, CheckFn check_received, + std::chrono::milliseconds total = 3000ms, + std::chrono::milliseconds settle = 150ms) { + auto deadline = std::chrono::steady_clock::now() + total; + while (std::chrono::steady_clock::now() < deadline) { + publish_once(); + if (mads_test::wait_for(check_received, settle, 10ms)) + return true; + } + return false; +} + +} // namespace + +// --------------------------------------------------------------------------- +// publish(json) / receive() +// --------------------------------------------------------------------------- + +TEST_CASE("publish(json) delivers message_type::json with matching topic, " + "json and message content", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42100; + auto pub = make_pub("pubA", port); + auto sub = make_sub("subA", port); + + nlohmann::json payload = {{"hello", "world"}, {"n", 7}}; + bool got = retry_until( + [&] { pub->publish(payload, "topicA"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got); + + auto [topic, doc] = sub->last_json(); + REQUIRE(topic == "topicA"); + REQUIRE(doc.at("hello") == "world"); + REQUIRE(doc.at("n") == 7); + // Agent::publish() auto-stamps these fields (src/agent.cpp:748-759). + REQUIRE(doc.contains("timestamp")); + REQUIRE(doc.contains("timecode")); + REQUIRE(doc.contains("agent_id")); + REQUIRE(doc.contains("hostname")); + + auto [topic2, text2] = sub->last_message(); + REQUIRE(topic2 == "topicA"); + REQUIRE(nlohmann::json::parse(text2).at("n") == 7); + + REQUIRE(sub->last_topic() == "topicA"); +} + +// --------------------------------------------------------------------------- +// publish(blob) overloads / receive() +// --------------------------------------------------------------------------- + +TEST_CASE("publish(const char*, len, meta) delivers message_type::blob with " + "matching bytes and metadata", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42101; + auto pub = make_pub("pubB1", port); + auto sub = make_sub("subB1", port); + + std::string data = "hello blob"; + nlohmann::json meta = {{"format", "raw"}, {"note", "test"}}; + + bool got = retry_until( + [&] { pub->publish(data.data(), data.size(), meta, "blobtopic"); }, + [&] { return sub->receive(true) == Mads::message_type::blob; }); + REQUIRE(got); + + auto [topic, meta_text, bytes] = sub->last_blob(); + REQUIRE(topic == "blobtopic"); + REQUIRE(std::string(bytes.begin(), bytes.end()) == data); + nlohmann::json meta_doc = nlohmann::json::parse(meta_text); + REQUIRE(meta_doc.value("note", std::string()) == "test"); + + auto [tv, fv, bv] = sub->last_blob_view(); + REQUIRE(tv == "blobtopic"); + REQUIRE(std::string(bv.begin(), bv.end()) == data); +} + +TEST_CASE("publish(vector, meta) delivers message_type::blob " + "with matching bytes", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42102; + auto pub = make_pub("pubB2", port); + auto sub = make_sub("subB2", port); + + std::vector data = {1, 2, 3, 4, 5, 250, 251, 252}; + nlohmann::json meta = {{"format", "raw"}}; + + bool got = retry_until( + [&] { pub->publish(data, meta, "vectopic"); }, + [&] { return sub->receive(true) == Mads::message_type::blob; }); + REQUIRE(got); + + auto [topic, meta_text, bytes] = sub->last_blob(); + REQUIRE(topic == "vectopic"); + REQUIRE(bytes == data); +} + +// --------------------------------------------------------------------------- +// receive() timeout path +// --------------------------------------------------------------------------- + +TEST_CASE("receive() returns message_type::none when nothing arrives before " + "the receive timeout", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42103; + // A lone, connected subscriber: ZMQ connect() is async, so no publisher + // needs to exist for the socket to reach the connected state. + auto sub = make_sub("subC", port); + sub->set_receive_timeout(150); + REQUIRE(sub->receive_timeout() == 150); + + auto start = std::chrono::steady_clock::now(); + auto mt = sub->receive(); // blocking, bounded by the 150ms timeout + auto elapsed = std::chrono::steady_clock::now() - start; + + REQUIRE(mt == Mads::message_type::none); + REQUIRE(elapsed < 2000ms); +} + +// --------------------------------------------------------------------------- +// Topic filtering +// --------------------------------------------------------------------------- + +TEST_CASE("a subscriber subscribed to one topic never receives a different " + "topic", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42104; + auto pub = make_pub("pubE", port); + auto sub = make_sub("subE", port, {"A"}); + + bool got_a = retry_until( + [&] { pub->publish(nlohmann::json{{"tag", "a"}}, "A"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got_a); + REQUIRE(std::get<0>(sub->last_message()) == "A"); + + // Drain any leftover queued "A" messages from the retry warm-up above. + while (sub->receive(true) == Mads::message_type::json) { + } + + pub->publish(nlohmann::json{{"tag", "b"}}, "B"); + std::this_thread::sleep_for(300ms); // allow time for an (unwanted) delivery + REQUIRE(sub->receive(true) == Mads::message_type::none); + REQUIRE(std::get<0>(sub->last_message()) == "A"); // unchanged by topic B +} + +// --------------------------------------------------------------------------- +// Accessors +// --------------------------------------------------------------------------- + +TEST_CASE("accessors: name/status/agent_id/topics/endpoints/is_connected/" + "info", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42105; + auto pub = make_pub("pubF", port); + auto sub = make_sub("subF", port); + + REQUIRE(pub->name() == "pubF"); + REQUIRE(sub->name() == "subF"); + REQUIRE(pub->is_connected()); + REQUIRE(sub->is_connected()); + + sub->set_agent_id("agent-123"); + REQUIRE(sub->get_agent_id() == "agent-123"); + + sub->set_sub_topic({"foo", "bar"}); + REQUIRE(sub->sub_topic() == std::vector{"foo", "bar"}); + + pub->set_pub_topic("mytopic"); + REQUIRE(pub->pub_topic() == "mytopic"); + + REQUIRE(sub->sub_endpoint() == mads_test::loopback(port)); + // In cross mode, connect_pub() rewrites _sub_endpoint into its bind form. + REQUIRE(pub->sub_endpoint().rfind("tcp://*:", 0) == 0); + + bool got = retry_until( + [&] { pub->publish(nlohmann::json{{"v", 1}}, "status_topic"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got); + auto st = sub->status(); + REQUIRE(st.count("status_topic") == 1); + REQUIRE(nlohmann::json::parse(st.at("status_topic")).at("v") == 1); + + std::ostringstream oss; + sub->info(oss); + REQUIRE_FALSE(oss.str().empty()); + REQUIRE(oss.str().find("subF") != std::string::npos); +} + +// --------------------------------------------------------------------------- +// Error paths +// --------------------------------------------------------------------------- + +TEST_CASE("operations on an uninitialized agent throw AgentError", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + Mads::Agent a("uninit", "none"); + REQUIRE_THROWS_AS(a.connect(), Mads::AgentError); + REQUIRE_THROWS_AS(a.publish(nlohmann::json{{"x", 1}}), Mads::AgentError); + REQUIRE_THROWS_AS(a.receive(), Mads::AgentError); + REQUIRE_THROWS_AS(a.disconnect(), Mads::AgentError); + REQUIRE_THROWS_AS(a.enable_remote_control(), Mads::AgentError); + REQUIRE_THROWS_AS(a.remote_control("{}"), Mads::AgentError); + REQUIRE_THROWS_AS(a.register_event(), Mads::AgentError); + REQUIRE_THROWS_AS(a.save_settings(), Mads::AgentError); + std::ostringstream oss; + REQUIRE_THROWS_AS(a.info(oss), Mads::AgentError); +} + +TEST_CASE("set_settings_timeout() throws AgentError once the agent is " + "initialized", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + Mads::Agent a("guardB", "none"); + a.init(false, false); + REQUIRE_THROWS_AS(a.set_settings_timeout(100), Mads::AgentError); + REQUIRE_THROWS_AS(a.set_settings_timeout(std::chrono::milliseconds(100)), + Mads::AgentError); +} + +TEST_CASE("set_delivery/set_high_watermark/enable_remote_control throw " + "AgentError once connected", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42106; + auto a = std::make_unique("guardA", "none"); + a->init(false, false); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_pub_topic(""); + a->set_sub_topic({""}); + a->connect(std::chrono::milliseconds(0)); // lone connect, no peer required + + REQUIRE(a->is_connected()); + REQUIRE_THROWS_AS(a->set_delivery(Mads::Delivery::LastKnownValue), + Mads::AgentError); + REQUIRE_THROWS_AS(a->set_high_watermark(10), Mads::AgentError); + REQUIRE_THROWS_AS(a->enable_remote_control(), Mads::AgentError); +} + +TEST_CASE("receive() throws AgentError while threaded remote control owns " + "the subscriber socket", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42107; + auto a = std::make_unique("guardC", "none"); + a->init(false, false); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_pub_topic(""); + // Must be called before connect(): it pushes "control" onto _sub_topic and + // (threaded=true) hands the subscriber socket to a background thread. + a->enable_remote_control(true); + a->connect(std::chrono::milliseconds(0)); + REQUIRE(a->is_connected()); + REQUIRE_THROWS_AS(a->receive(), Mads::AgentError); +} + +// --------------------------------------------------------------------------- +// disconnect() / shutdown() safety +// --------------------------------------------------------------------------- + +TEST_CASE("disconnect() and shutdown() are safe to call repeatedly", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42108; + auto a = std::make_unique("guardD", "none"); + a->init(false, false); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_pub_topic(""); + a->set_sub_topic({""}); + a->connect(std::chrono::milliseconds(0)); + + REQUIRE(a->is_connected()); + a->disconnect(); + REQUIRE_FALSE(a->is_connected()); + a->disconnect(); // no-op the second time, must not throw + REQUIRE_FALSE(a->is_connected()); + + a->shutdown(); + a->shutdown(); // idempotent, guarded by _shutdown_done + SUCCEED("disconnect()/shutdown() tolerated repeated calls"); +} + +// --------------------------------------------------------------------------- +// Delivery::LastKnownValue +// --------------------------------------------------------------------------- + +TEST_CASE("Delivery::LastKnownValue drains a burst of messages down to the " + "latest value", + "[agent_pubsub]") { + mads_test::RunningGuard guard; + const uint16_t port = 42109; + auto pub = make_pub("pubG", port); + + auto sub = std::make_unique("subG", "none"); + sub->init(false, false); + sub->set_sub_endpoint(mads_test::loopback(port)); + sub->set_pub_topic(""); + sub->set_sub_topic({""}); + sub->set_delivery(Mads::Delivery::LastKnownValue); + REQUIRE(sub->delivery() == Mads::Delivery::LastKnownValue); + sub->connect(std::chrono::milliseconds(0)); + + // Warm up the pair first (slow-joiner absorption). + bool warm = retry_until( + [&] { pub->publish(nlohmann::json{{"idx", 0}}, "lkv"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(warm); + + // Publish a fast burst; the background drain thread (src/agent.cpp + // Agent::connect_sub) should collapse it down to the last value. + for (int i = 1; i <= 5; ++i) { + pub->publish(nlohmann::json{{"idx", i}}, "lkv"); + } + + bool ok = mads_test::wait_for( + [&] { + auto mt = sub->receive(true); + if (mt != Mads::message_type::json) + return false; + auto [topic, doc] = sub->last_json(); + return doc.value("idx", -1) == 5; + }, + 3000ms, 20ms); + REQUIRE(ok); +} diff --git a/tests/test_agent_wire.cpp b/tests/test_agent_wire.cpp new file mode 100644 index 0000000..2f27e7a --- /dev/null +++ b/tests/test_agent_wire.cpp @@ -0,0 +1,264 @@ +// Unit tests for the Mads::Agent on-the-wire codec: the WireFormat x +// Compression matrix implemented in the anonymous namespace of +// src/agent.cpp:62-191, exercised indirectly through publish()/receive() +// round trips over an in-process ZeroMQ loopback pair. +#include + +#include +#include +#include +#include +#include + +#include +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +namespace { + +std::unique_ptr make_pub(std::string name, uint16_t port, + Mads::WireFormat wf, + Mads::Compression comp) { + auto a = std::make_unique(name, "none"); + a->init(false, false); + a->set_cross(true); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_sub_topic({}); + a->set_wire_format(wf); + a->set_compression(comp); + a->connect(std::chrono::milliseconds(0)); + return a; +} + +std::unique_ptr make_sub(std::string name, uint16_t port) { + auto a = std::make_unique(name, "none"); + a->init(false, false); + a->set_sub_endpoint(mads_test::loopback(port)); + a->set_pub_topic(""); + a->set_sub_topic({""}); + a->connect(std::chrono::milliseconds(0)); + return a; +} + +template +bool retry_until(PublishFn publish_once, CheckFn check_received, + std::chrono::milliseconds total = 3000ms, + std::chrono::milliseconds settle = 150ms) { + auto deadline = std::chrono::steady_clock::now() + total; + while (std::chrono::steady_clock::now() < deadline) { + publish_once(); + if (mads_test::wait_for(check_received, settle, 10ms)) + return true; + } + return false; +} + +// Hand-crafts a MADS extended-frame header identical to +// src/agent.cpp make_wire_header()/WIRE_HEADER_SIZE, used only to inject +// synthetic frames via a raw zmqpp PUB socket (bypassing Agent::publish()). +std::string wire_header_bytes(uint8_t fmt, uint8_t comp, bool blob) { + std::string h; + h.append("MADS", 4); + h.push_back(static_cast(1)); // header version + h.push_back(static_cast(fmt)); + h.push_back(static_cast(comp)); + h.push_back(static_cast(blob ? 0x01 : 0x00)); + h.push_back(static_cast(0)); // schema (4 bytes, unchecked by reader) + h.push_back(static_cast(0)); + h.push_back(static_cast(0)); + h.push_back(static_cast(0)); + return h; +} + +void check_round_trip(Mads::WireFormat wf, Mads::Compression comp, + uint16_t port) { + auto pub = make_pub("pubW", port, wf, comp); + auto sub = make_sub("subW", port); + + nlohmann::json payload = { + {"greeting", "hello wire"}, {"num", 123}, {"arr", {1, 2, 3}}}; + bool got = retry_until( + [&] { pub->publish(payload, "t"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got); + auto [topic, doc] = sub->last_json(); + REQUIRE(topic == "t"); + REQUIRE(doc.at("greeting") == "hello wire"); + REQUIRE(doc.at("num") == 123); + REQUIRE(doc.at("arr") == nlohmann::json({1, 2, 3})); + REQUIRE(sub->dropped_messages() == 0); +} + +} // namespace + +// --------------------------------------------------------------------------- +// WireFormat x Compression matrix +// --------------------------------------------------------------------------- + +TEST_CASE("wire codec: Json + Compression::None round-trips", "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::Json, Mads::Compression::None, 42120); +} + +TEST_CASE("wire codec: Json + Compression::Snappy round-trips", + "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::Json, Mads::Compression::Snappy, 42121); +} + +TEST_CASE("wire codec: Json + Compression::Auto round-trips (small payload)", + "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::Json, Mads::Compression::Auto, 42122); +} + +TEST_CASE("wire codec: MsgPack + Compression::None round-trips", + "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::MsgPack, Mads::Compression::None, 42123); +} + +TEST_CASE("wire codec: MsgPack + Compression::Snappy round-trips", + "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::MsgPack, Mads::Compression::Snappy, + 42124); +} + +TEST_CASE( + "wire codec: MsgPack + Compression::Auto round-trips (small payload)", + "[agent_wire]") { + mads_test::RunningGuard guard; + check_round_trip(Mads::WireFormat::MsgPack, Mads::Compression::Auto, 42125); +} + +// --------------------------------------------------------------------------- +// Compression::Auto threshold behavior +// --------------------------------------------------------------------------- + +TEST_CASE("Compression::Auto round-trips identically below and above the " + "256-byte threshold", + "[agent_wire]") { + mads_test::RunningGuard guard; + const uint16_t port = 42126; + auto pub = make_pub("pubAuto", port, Mads::WireFormat::Json, + Mads::Compression::Auto); + auto sub = make_sub("subAuto", port); + + nlohmann::json small = {{"tag", "small"}, {"n", 1}}; + REQUIRE(small.dump().size() < Mads::COMPRESSION_AUTO_THRESHOLD); + + std::string filler(400, 'x'); + nlohmann::json big = {{"tag", "big"}, {"filler", filler}}; + REQUIRE(big.dump().size() >= Mads::COMPRESSION_AUTO_THRESHOLD); + + bool got_small = retry_until( + [&] { pub->publish(small, "s"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got_small); + auto [t1, d1] = sub->last_json(); + REQUIRE(d1.at("tag") == "small"); + REQUIRE(d1.at("n") == 1); + + bool got_big = retry_until( + [&] { pub->publish(big, "b"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got_big); + auto [t2, d2] = sub->last_json(); + REQUIRE(d2.at("tag") == "big"); + REQUIRE(d2.at("filler") == filler); +} + +// --------------------------------------------------------------------------- +// Large Compression::Snappy payload +// --------------------------------------------------------------------------- + +TEST_CASE("Compression::Snappy round-trips a large payload byte-identical", + "[agent_wire]") { + mads_test::RunningGuard guard; + const uint16_t port = 42127; + auto pub = make_pub("pubBig", port, Mads::WireFormat::Json, + Mads::Compression::Snappy); + auto sub = make_sub("subBig", port); + + std::string filler; + filler.reserve(5000); + for (int i = 0; i < 5000; ++i) + filler.push_back(static_cast('a' + (i % 26))); + nlohmann::json payload = {{"filler", filler}}; + + bool got = retry_until( + [&] { pub->publish(payload, "big"); }, + [&] { return sub->receive(true) == Mads::message_type::json; }); + REQUIRE(got); + auto [topic, doc] = sub->last_json(); + REQUIRE(doc.at("filler").get() == filler); +} + +// --------------------------------------------------------------------------- +// dropped_messages() on malformed frames, and continued usability afterwards +// --------------------------------------------------------------------------- + +TEST_CASE("dropped_messages() counts malformed frames and the agent stays " + "usable afterwards", + "[agent_wire]") { + mads_test::RunningGuard guard; + const uint16_t port = 42128; + zmqpp::context ctx; + zmqpp::socket raw_pub(ctx, zmqpp::socket_type::pub); + raw_pub.bind(mads_test::loopback(port)); + + auto sub = make_sub("subMal", port); + // Settle: subscriber connect + ZMQ subscription propagation to raw_pub. + std::this_thread::sleep_for(300ms); + + size_t dropped_before = sub->dropped_messages(); + + // Malformed legacy 2-part frame: [topic]["not actually snappy data"]. + // decode_to_payload() assumes 2-part legacy frames are snappy(json); this + // payload fails to decompress, so it must be dropped, not delivered. + bool saw_drop = false; + for (int attempt = 0; attempt < 50 && !saw_drop; ++attempt) { + zmqpp::message bad; + bad << std::string("mal") + << std::string("this is definitely not snappy compressed data"); + raw_pub.send(bad); + // dropped_messages() only increments as a side effect of receive() + // actually parsing (and rejecting) a frame, so the predicate must drive + // receive() itself rather than passively poll the counter. + saw_drop = mads_test::wait_for( + [&] { + sub->receive(true); + return sub->dropped_messages() > dropped_before; + }, + 100ms, 10ms); + } + REQUIRE(saw_drop); + REQUIRE(sub->dropped_messages() > dropped_before); + // The bad frame must never surface as a normal message. + REQUIRE(sub->receive(true) != Mads::message_type::json); + + // A well-formed extended-header frame sent right after must still arrive: + // the agent is not wedged by the earlier malformed frame. + size_t dropped_after_bad = sub->dropped_messages(); + bool got_good = false; + for (int attempt = 0; attempt < 50 && !got_good; ++attempt) { + zmqpp::message good; + good << std::string("mal") << wire_header_bytes(0 /*Json*/, 0 /*None*/, + false) + << std::string(R"({"ok":true})"); + raw_pub.send(good); + got_good = mads_test::wait_for( + [&] { return sub->receive(true) == Mads::message_type::json; }, 150ms, + 10ms); + } + REQUIRE(got_good); + auto [topic, doc] = sub->last_json(); + REQUIRE(topic == "mal"); + REQUIRE(doc.at("ok") == true); + REQUIRE(sub->dropped_messages() == dropped_after_bad); // no further drops +} diff --git a/tests/test_lazy_payload.cpp b/tests/test_lazy_payload.cpp new file mode 100644 index 0000000..72fa829 --- /dev/null +++ b/tests/test_lazy_payload.cpp @@ -0,0 +1,127 @@ +// Unit tests for Mads::LazyPayload (src/agent.hpp), the tiny lazily-converting +// text<->json cache used internally by Agent::receive() to avoid needless +// dump()/parse() round trips. +#include + +#include + +#include "agent.hpp" + +using Mads::LazyPayload; + +// --------------------------------------------------------------------------- +// from_text(): text() is free, doc() parses lazily on first access +// --------------------------------------------------------------------------- + +TEST_CASE("LazyPayload::from_text exposes the original text verbatim", + "[lazy_payload]") { + auto p = LazyPayload::from_text(R"({"a":1,"b":"two"})"); + REQUIRE(p.text() == R"({"a":1,"b":"two"})"); +} + +TEST_CASE("LazyPayload::from_text parses doc() lazily from the cached text", + "[lazy_payload]") { + auto p = LazyPayload::from_text(R"({"a":1,"b":"two"})"); + const nlohmann::json &doc = p.doc(); + REQUIRE(doc.at("a") == 1); + REQUIRE(doc.at("b") == "two"); +} + +TEST_CASE("LazyPayload::from_text repeated doc() access returns equal, " + "consistent content", + "[lazy_payload]") { + auto p = LazyPayload::from_text(R"({"n":42})"); + const nlohmann::json &doc1 = p.doc(); + const nlohmann::json &doc2 = p.doc(); + // Second access returns the same cached object (same address), proving the + // parse only happens once. + REQUIRE(&doc1 == &doc2); + REQUIRE(doc1.at("n") == 42); +} + +// --------------------------------------------------------------------------- +// from_doc(): doc() is free, text() dumps lazily on first access +// --------------------------------------------------------------------------- + +TEST_CASE("LazyPayload::from_doc exposes the original object verbatim", + "[lazy_payload]") { + nlohmann::json j = {{"x", 1}, {"y", 2}}; + auto p = LazyPayload::from_doc(j); + REQUIRE(p.doc() == j); +} + +TEST_CASE("LazyPayload::from_doc dumps text() lazily from the cached object", + "[lazy_payload]") { + nlohmann::json j = {{"x", 1}, {"y", 2}}; + auto p = LazyPayload::from_doc(j); + std::string text = p.text(); + // Round-trip: re-parsing the dumped text must reproduce the same object. + REQUIRE(nlohmann::json::parse(text) == j); +} + +TEST_CASE("LazyPayload::from_doc repeated text() access returns the same " + "cached string", + "[lazy_payload]") { + nlohmann::json j = {{"k", "v"}}; + auto p = LazyPayload::from_doc(j); + const std::string &t1 = p.text(); + const std::string &t2 = p.text(); + REQUIRE(&t1 == &t2); + REQUIRE(t1 == j.dump()); +} + +// --------------------------------------------------------------------------- +// Default-constructed / empty payload: caching order affects the result, +// since text()<->doc() conversion is only ever driven by whichever cache is +// already populated. +// --------------------------------------------------------------------------- + +TEST_CASE("LazyPayload default construction: text() first yields empty text " + "and a null doc()", + "[lazy_payload]") { + LazyPayload p; + // text() sees no cached doc, so it caches an empty string. + REQUIRE(p.text().empty()); + // doc() then sees a cached (empty) text and, since it's empty, produces a + // null json rather than trying to parse an empty string. + REQUIRE(p.doc().is_null()); +} + +TEST_CASE("LazyPayload default construction: doc() first yields a null doc " + "and a \"null\" text", + "[lazy_payload]") { + LazyPayload p; + // doc() sees no cached text, so it caches a default-constructed (null) json. + REQUIRE(p.doc().is_null()); + // text() then dumps the cached null doc, producing the literal "null". + REQUIRE(p.text() == "null"); +} + +TEST_CASE("LazyPayload default construction: text() and doc() are each " + "idempotent once cached", + "[lazy_payload]") { + LazyPayload p; + REQUIRE(p.text().empty()); + REQUIRE(p.text().empty()); // repeated access, still cached empty string + REQUIRE(p.doc().is_null()); + REQUIRE(p.doc().is_null()); +} + +// --------------------------------------------------------------------------- +// Move semantics used by from_text/from_doc (both take by value and move in) +// --------------------------------------------------------------------------- + +TEST_CASE("LazyPayload::from_text accepts an rvalue without copying it first", + "[lazy_payload]") { + std::string s = "\"hello\""; + auto p = LazyPayload::from_text(std::move(s)); + REQUIRE(p.text() == "\"hello\""); + REQUIRE(p.doc() == "hello"); +} + +TEST_CASE("LazyPayload::from_doc accepts an rvalue without copying it first", + "[lazy_payload]") { + nlohmann::json j = {{"arr", {1, 2, 3}}}; + auto p = LazyPayload::from_doc(std::move(j)); + REQUIRE(p.doc().at("arr").size() == 3); +} From ca77230a58464cb4977fbfee9e6b27798aebc9e8 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:07:55 +0000 Subject: [PATCH 04/14] fix: set linger=0 on broker query socket to avoid shutdown hang With the default infinite linger, a settings request queued toward an unreachable broker keeps the ZMQ context alive and Agent::shutdown() blocks forever in zmq_ctx_term. Found by the new broker-timeout unit test. --- src/agent.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/agent.cpp b/src/agent.cpp index 2fdbdde..03ba178 100644 --- a/src/agent.cpp +++ b/src/agent.cpp @@ -231,6 +231,10 @@ Agent::query_broker(string uri, string name, int timeout) { // Single REQ socket reused for both the settings and timecode round-trips. zmqpp::socket socket(_context, zmqpp::socket_type::req); setup_curve_on(socket); + // Drop any undelivered request on close: with the default infinite linger, + // a request queued toward an unreachable broker keeps the context alive and + // context termination (Agent shutdown) blocks forever. + socket.set(zmqpp::socket_option::linger, 0); if (timeout > 0) { socket.set(zmqpp::socket_option::receive_timeout, timeout); socket.set(zmqpp::socket_option::send_timeout, timeout); From ae29ea4fbaf913c82d73c18f7eb22fed42b81852 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:07:56 +0000 Subject: [PATCH 05/14] test: add Agent settings and broker-protocol unit tests Local TOML settings loading (fixtures for valid/malformed/missing cases, sub_topic string vs array, save_settings guard) and an in-process fake broker covering the REQ/REP settings protocol: happy path, attachments, timecode offset, version mismatch, refusals, timeouts, start_agent. Coverage: src/agent.cpp 71% (from 58.5%). --- tests/test_agent_broker.cpp | 435 ++++++++++++++++++++++++++++++++++ tests/test_agent_settings.cpp | 194 +++++++++++++++ 2 files changed, 629 insertions(+) create mode 100644 tests/test_agent_broker.cpp create mode 100644 tests/test_agent_settings.cpp diff --git a/tests/test_agent_broker.cpp b/tests/test_agent_broker.cpp new file mode 100644 index 0000000..a001af5 --- /dev/null +++ b/tests/test_agent_broker.cpp @@ -0,0 +1,435 @@ +// Unit tests for the Mads::Agent <-> broker settings protocol +// (src/agent.cpp: Agent::query_broker() ~230-307, Agent::init() broker branch +// ~375-380, Agent::save_settings(), Mads::start_agent() ~193-213), exercised +// against an in-process fake broker (a std::thread running a zmqpp REP socket +// on tcp://127.0.0.1:, port range 42200-42299). +// +// Wire protocol transcribed from src/agent.cpp query_broker(): +// Settings round-trip (REQ->REP on one socket, reused for both exchanges): +// -> [LIB_VERSION]["settings"][agent_name] (3 parts) +// <- [broker_version][raw_toml_settings] (2 parts), or +// <- [broker_version][raw_toml_settings][attachment_bytes] (3 parts) +// parts()==3 is the ONLY trigger for attachment handling; anything +// else >= 2 parts is accepted with no attachment. +// Timecode round-trip (same socket, immediately after): +// -> ["v" + LIB_VERSION]["timecode"] (2 parts) +// <- [broker_timecode_as_string] (>=1 part; +// only get(0) is read, via std::stod) +// +// Failure modes read directly from the source: +// - msg_in.parts() < 2 -> AgentError "Broker refuses..." +// - !check_version(msg_in.get(0)) -> AgentError "...wrong version" +// - send/receive timeout (both legs) -> AgentError "Timed out in ..." +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +namespace { + +// A minimal settings broker: binds a REP socket and replies to the agent's +// "settings" and "timecode" requests with a configurable, exact multipart +// layout. Polls with a receive timeout so the thread can always be stopped +// promptly and joined (never a blocking recv without a timeout). +class FakeBroker { +public: + explicit FakeBroker(uint16_t port) + : _ctx(), _sock(_ctx, zmqpp::socket_type::rep) { + _sock.set(zmqpp::socket_option::receive_timeout, 100); + _sock.bind(mads_test::loopback(port)); + } + + ~FakeBroker() { stop(); } + + // --- configuration: set before start(), never mutated afterwards --- + std::string settings_version = LIB_VERSION; + std::string settings_body; + bool with_attachment = false; + std::string attachment_bytes; + bool refuse_settings = false; // reply with only the version part (<2 total) + double timecode_value = 0.0; + int timecode_delay_ms = 0; // artificial delay before the timecode reply + + void start() { + _thread = std::thread([this] { run(); }); + } + + void stop() { + if (_stopped.exchange(true)) return; + if (_thread.joinable()) _thread.join(); + } + +private: + void run() { + while (!_stopped) { + zmqpp::message msg; + if (!_sock.receive(msg)) continue; // 100ms poll timeout, check _stopped + if (msg.parts() < 2) continue; // malformed request, ignore + std::string kind = msg.get(1); + zmqpp::message reply; + if (kind == "settings") { + if (refuse_settings) { + reply << settings_version; + } else { + reply << settings_version << settings_body; + if (with_attachment) reply << attachment_bytes; + } + _sock.send(reply); + } else if (kind == "timecode") { + if (timecode_delay_ms > 0) + std::this_thread::sleep_for( + std::chrono::milliseconds(timecode_delay_ms)); + reply << std::to_string(timecode_value); + _sock.send(reply); + } else { + // Keep the REP socket's strict recv/send alternation intact even for + // requests this fake broker does not otherwise understand. + reply << std::string(LIB_VERSION) << std::string("{}"); + _sock.send(reply); + } + } + } + + zmqpp::context _ctx; + zmqpp::socket _sock; + std::thread _thread; + std::atomic _stopped{false}; +}; + +// Builds a settings TOML body mirroring the shape of the repo-root mads.ini: +// a fleet-wide [agents] section plus a per-agent section for `name`. +// Frontend/backend addresses are pinned into our port range so that +// Agent::connect() (which only *connects*, never binds, unless +// set_cross(true)) never touches a port outside 42200-42299. +std::string make_settings_toml(const std::string &name, + const std::string &extra_agent_lines = "") { + std::ostringstream ss; + ss << "[agents]\n" + << "timecode_fps = 25\n" + << "frontend_address = \"tcp://localhost:42290\"\n" + << "backend_address = \"tcp://localhost:42291\"\n" + << "\n[" << name << "]\n" + << "pub_topic = \"" << name << "_pub\"\n" + << "sub_topic = [\"in_a\", \"in_b\"]\n" + << extra_agent_lines; + return ss.str(); +} + +// Extracts the numeric value printed after "Timecode offset:" in +// Agent::info() (src/agent.cpp:617-618), stripping any ANSI styling +// (rang::style::bold/reset) that may surround it so a plain istream +// extraction lands on the actual number rather than an escape code digit. +double parse_timecode_offset(const std::string &info_text) { + static const std::string ESC = "\x1b["; + std::string clean; + clean.reserve(info_text.size()); + for (size_t i = 0; i < info_text.size();) { + if (info_text.compare(i, ESC.size(), ESC) == 0) { + size_t end = info_text.find('m', i); + if (end == std::string::npos) break; + i = end + 1; + } else { + clean.push_back(info_text[i]); + ++i; + } + } + auto pos = clean.find("Timecode offset:"); + REQUIRE(pos != std::string::npos); + std::istringstream iss(clean.substr(pos + std::string("Timecode offset:").size())); + double val = 0.0; + iss >> val; + return val; +} + +} // namespace + +// --------------------------------------------------------------------------- +// init() over the broker protocol: happy path +// --------------------------------------------------------------------------- + +TEST_CASE("init() against a fake broker loads settings via the REQ/REP " + "protocol", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42200; + FakeBroker broker(port); + broker.settings_body = make_settings_toml("bhappy"); + broker.timecode_value = 100.0; + broker.start(); + + Mads::Agent a("bhappy", mads_test::loopback(port)); + a.init(false, false); + + REQUIRE_FALSE(a.settings_are_local()); + REQUIRE(a.settings_uri() == mads_test::loopback(port)); + REQUIRE(a.pub_topic() == "bhappy_pub"); + REQUIRE(a.sub_topic() == std::vector{"in_a", "in_b"}); + + nlohmann::json j = a.get_settings(); + REQUIRE(j.at("pub_topic") == "bhappy_pub"); + REQUIRE(j.at("sub_topic") == nlohmann::json::array({"in_a", "in_b"})); + + // No attachment was served. + REQUIRE(a.attachment_path().empty()); + + // Host substitution (src/agent.cpp:394-401): the broker's host replaces the + // host portion of frontend_address/backend_address, keeping their ports. + REQUIRE(a.pub_endpoint() == "tcp://127.0.0.1:42290"); + REQUIRE(a.sub_endpoint() == "tcp://127.0.0.1:42291"); +} + +// --------------------------------------------------------------------------- +// save_settings() after a broker-backed init() +// --------------------------------------------------------------------------- + +TEST_CASE("save_settings() writes the exact raw settings text served by the " + "broker", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42201; + FakeBroker broker(port); + broker.settings_body = make_settings_toml("bsave"); + broker.timecode_value = 0.0; + broker.start(); + + Mads::Agent a("bsave", mads_test::loopback(port)); + a.init(false, false); + REQUIRE_FALSE(a.settings_are_local()); + + auto tmp = + std::filesystem::temp_directory_path() / "mads_test_broker_save.ini"; + a.save_settings(tmp.string()); + + std::ifstream in(tmp); + REQUIRE(in.good()); + std::ostringstream contents; + contents << in.rdbuf(); + REQUIRE(contents.str() == broker.settings_body); + std::filesystem::remove(tmp); +} + +// --------------------------------------------------------------------------- +// Attachment handling +// --------------------------------------------------------------------------- + +TEST_CASE("init() saves a served attachment under the default .plugin " + "extension", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42202; + FakeBroker broker(port); + broker.settings_body = make_settings_toml("battach1"); + broker.with_attachment = true; + broker.attachment_bytes = "BINARY-PLUGIN-DATA-1"; + broker.timecode_value = 0.0; + broker.start(); + + Mads::Agent a("battach1", mads_test::loopback(port)); + a.init(false, false); + + auto path = a.attachment_path(); + REQUIRE_FALSE(path.empty()); + REQUIRE(path.extension() == ".plugin"); + REQUIRE(std::filesystem::exists(path)); + + std::ifstream in(path, std::ios::binary); + std::ostringstream contents; + contents << in.rdbuf(); + REQUIRE(contents.str() == broker.attachment_bytes); +} + +TEST_CASE("init() renames a served attachment to a custom attachment_ext", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42203; + FakeBroker broker(port); + broker.settings_body = + make_settings_toml("battach2", "attachment_ext = \"bin\"\n"); + broker.with_attachment = true; + broker.attachment_bytes = "BINARY-PLUGIN-DATA-2"; + broker.timecode_value = 0.0; + broker.start(); + + Mads::Agent a("battach2", mads_test::loopback(port)); + a.init(false, false); + + auto path = a.attachment_path(); + REQUIRE_FALSE(path.empty()); + REQUIRE(path.extension() == ".bin"); + REQUIRE(std::filesystem::exists(path)); + + std::ifstream in(path, std::ios::binary); + std::ostringstream contents; + contents << in.rdbuf(); + REQUIRE(contents.str() == broker.attachment_bytes); +} + +// --------------------------------------------------------------------------- +// Timecode offset +// --------------------------------------------------------------------------- + +TEST_CASE("init() applies the broker's timecode offset", "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42204; + + auto now0 = std::chrono::system_clock::now(); + double local_tc0 = Mads::timecode(now0, 25); + const double artificial_offset = 100.5; + + FakeBroker broker(port); + broker.settings_body = make_settings_toml("btimecode"); + broker.timecode_value = local_tc0 + artificial_offset; + broker.start(); + + Mads::Agent a("btimecode", mads_test::loopback(port)); + a.init(false, false); + + std::ostringstream oss; + a.info(oss); + double measured_offset = parse_timecode_offset(oss.str()); + // Generous tolerance: only verifies the offset was applied at all, not + // sub-second precision (query_broker's `now` capture happens after our + // reference point, and Mads::timecode() buckets to ~40ms). + REQUIRE(std::abs(measured_offset - artificial_offset) < 1.0); +} + +// --------------------------------------------------------------------------- +// Error paths: broker response validation +// --------------------------------------------------------------------------- + +TEST_CASE("init() throws AgentError when the broker reports a mismatched " + "version", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42205; + FakeBroker broker(port); + broker.settings_version = "v9.9.9"; // != LIB_VERSION_CHECK ("v0.0") + broker.settings_body = make_settings_toml("bversion"); + broker.start(); + + Mads::Agent a("bversion", mads_test::loopback(port)); + bool threw = false; + try { + a.init(false, false); + } catch (const Mads::AgentError &e) { + threw = true; + REQUIRE(std::string(e.what()).find("wrong version") != std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("init() throws AgentError when the broker refuses to provide " + "settings", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42206; + FakeBroker broker(port); + broker.refuse_settings = true; // reply has < 2 parts + broker.start(); + + Mads::Agent a("brefuse", mads_test::loopback(port)); + bool threw = false; + try { + a.init(false, false); + } catch (const Mads::AgentError &e) { + threw = true; + REQUIRE(std::string(e.what()).find("refuses") != std::string::npos); + } + REQUIRE(threw); +} + +// --------------------------------------------------------------------------- +// Error paths: timeouts (deterministic via set_settings_timeout(), never a +// blocking wait without a bound) +// --------------------------------------------------------------------------- + +TEST_CASE("init() throws AgentError when the settings request times out " + "against an unbound port", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42207; // intentionally nothing bound here + Mads::Agent a("btimeout1", mads_test::loopback(port)); + a.set_settings_timeout(200); + REQUIRE(a.settings_timeout() == 200); + + bool threw = false; + try { + a.init(false, false); + } catch (const Mads::AgentError &e) { + threw = true; + REQUIRE(std::string(e.what()).find("Timed out") != std::string::npos); + REQUIRE(std::string(e.what()).find("settings") != std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("init() throws AgentError when the timecode request times out", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42208; + FakeBroker broker(port); + broker.settings_body = make_settings_toml("btimeout2"); + broker.timecode_delay_ms = 800; // longer than the agent's settings timeout + broker.start(); + + Mads::Agent a("btimeout2", mads_test::loopback(port)); + a.set_settings_timeout(200); + + bool threw = false; + try { + a.init(false, false); + } catch (const Mads::AgentError &e) { + threw = true; + REQUIRE(std::string(e.what()).find("Timed out") != std::string::npos); + REQUIRE(std::string(e.what()).find("timecode") != std::string::npos); + } + REQUIRE(threw); +} + +// --------------------------------------------------------------------------- +// start_agent() +// --------------------------------------------------------------------------- + +TEST_CASE("start_agent() against a fake broker returns a connected, " + "broker-backed agent", + "[agent_broker]") { + mads_test::RunningGuard guard; + const uint16_t port = 42209; + FakeBroker broker(port); + broker.settings_body = make_settings_toml("bstart"); + broker.start(); + + auto agent = Mads::start_agent("bstart", mads_test::loopback(port)); + REQUIRE(agent->is_connected()); + REQUIRE_FALSE(agent->settings_are_local()); + REQUIRE(agent->name() == "bstart"); + REQUIRE(agent->pub_topic() == "bstart_pub"); +} + +TEST_CASE("start_agent() throws AgentError for incomplete crypto_settings", + "[agent_broker]") { + mads_test::RunningGuard guard; + // No socket is ever touched: the crypto_settings keys are validated before + // any Agent is constructed (src/agent.cpp:196-203). + REQUIRE_THROWS_AS( + Mads::start_agent("bcrypto", "none", {{"key_dir", "/tmp/keys"}}), + Mads::AgentError); + REQUIRE_THROWS_AS( + Mads::start_agent("bcrypto", "none", + {{"key_dir", "/tmp/keys"}, {"key_client", "client"}}), + Mads::AgentError); +} diff --git a/tests/test_agent_settings.cpp b/tests/test_agent_settings.cpp new file mode 100644 index 0000000..9c5ef62 --- /dev/null +++ b/tests/test_agent_settings.cpp @@ -0,0 +1,194 @@ +// Unit tests for Mads::Agent settings loading from local TOML files +// (src/agent.cpp: Agent::init(), Agent::load_settings(), Agent::save_settings(), +// Agent::settings_are_local(), Agent::settings_uri(), Agent::get_settings()). +// +// These tests never touch the network: `settings_uri` is always either "none" +// (documented test mode, src/agent.hpp:232) or a local filesystem path, so +// `settings_are_local()` (src/agent.cpp:1122-1124, "no tcp://" in the URI) is +// always true here. The broker (tcp://) path is covered by +// test_agent_broker.cpp. +#include +#include + +#include +#include +#include +#include +#include + +#include +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +namespace { + +std::string fixture(const std::string &name) { + return std::string(MADS_TEST_FIXTURES_DIR) + "/settings/" + name; +} + +} // namespace + +// --------------------------------------------------------------------------- +// init() from a local TOML file +// --------------------------------------------------------------------------- + +TEST_CASE("init() from a local TOML file loads settings, endpoints, and " + "fleet-wide defaults", + "[agent_settings]") { + mads_test::RunningGuard guard; + std::string path = fixture("valid.toml"); + Mads::Agent a("settingstest", path); + a.init(false, false); + + REQUIRE(a.settings_are_local()); + REQUIRE(a.settings_uri() == path); + + // Fleet-wide [agents] defaults, inherited since not overridden per-agent. + REQUIRE(a.pub_endpoint() == "tcp://localhost:42210"); + REQUIRE(a.sub_endpoint() == "tcp://localhost:42211"); + REQUIRE(a.timecode_fps == Catch::Approx(25.0)); + REQUIRE(a.wire_format() == Mads::WireFormat::Json); + REQUIRE(a.compression() == Mads::Compression::Auto); + + // Per-agent section. + REQUIRE(a.pub_topic() == "settingstest_pub"); + REQUIRE(a.sub_topic() == std::vector{"topicA", "topicB"}); + + nlohmann::json j = a.get_settings(); + REQUIRE(j.at("pub_topic") == "settingstest_pub"); + REQUIRE(j.at("sub_topic") == nlohmann::json::array({"topicA", "topicB"})); + REQUIRE(j.at("time_step") == 100); + REQUIRE(j.at("queue_size") == 5); + REQUIRE(j.at("custom_string") == "custom_value"); + REQUIRE(j.at("custom_int") == 42); + REQUIRE(j.at("custom_bool") == true); + REQUIRE(j.at("custom_double").get() == Catch::Approx(3.14)); + + // No settings were read from a broker. + REQUIRE(a.attachment_path().empty()); +} + +TEST_CASE("init() accepts sub_topic as a single string", "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("strsub", fixture("string_subtopic.toml")); + a.init(false, false); + + REQUIRE(a.sub_topic() == std::vector{"onlytopic"}); + REQUIRE(a.get_settings().at("sub_topic") == "onlytopic"); +} + +TEST_CASE("init() leaves sub_topic empty when the key is absent", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("nosub", fixture("no_subtopic.toml")); + a.init(false, false); + + REQUIRE(a.sub_topic().empty()); +} + +// --------------------------------------------------------------------------- +// Error paths +// --------------------------------------------------------------------------- + +TEST_CASE("init() from a nonexistent settings file throws toml::parse_error", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("nofile", fixture("does_not_exist.toml")); + REQUIRE_THROWS_AS(a.init(false, false), toml::parse_error); +} + +TEST_CASE("init() from a malformed TOML file throws toml::parse_error", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("badcfg", fixture("malformed.toml")); + REQUIRE_THROWS_AS(a.init(false, false), toml::parse_error); +} + +TEST_CASE("init() throws AgentError when no section matches the agent name", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("missingsection", fixture("missing_section.toml")); + bool threw = false; + try { + a.init(false, false); + } catch (const Mads::AgentError &e) { + threw = true; + std::string msg = e.what(); + REQUIRE(msg.find("missing") != std::string::npos); + REQUIRE(msg.find("missingsection") != std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("save_settings() throws AgentError when settings are local", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("settingstest", fixture("valid.toml")); + a.init(false, false); + + auto tmp = std::filesystem::temp_directory_path() / + "mads_test_settings_save_local.ini"; + REQUIRE_THROWS_AS(a.save_settings(tmp.string()), Mads::AgentError); +} + +// --------------------------------------------------------------------------- +// Timeout accessors: guarded by _init_done (set_settings_timeout) or not +// (set_receive_timeout), verbatim from src/agent.cpp. +// --------------------------------------------------------------------------- + +TEST_CASE("settings/receive timeout accessors: defaults and guards", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("settingstest", fixture("valid.toml")); + + // Defaults, before init(). + REQUIRE(a.settings_timeout() == 0); + REQUIRE(a.receive_timeout() == Mads::DEFAULT_RECEIVE_TIMEOUT_MS); + + // set_settings_timeout() is allowed before init()... + a.set_settings_timeout(1234); + REQUIRE(a.settings_timeout() == 1234); + a.set_settings_timeout(std::chrono::milliseconds(50)); + REQUIRE(a.settings_timeout() == 50); + + // set_receive_timeout() has no _init_done guard in the source: it is legal + // to call at any time, and immediately reflects in the getter. + a.set_receive_timeout(777); + REQUIRE(a.receive_timeout() == 777); + a.set_receive_timeout(std::chrono::milliseconds(321)); + REQUIRE(a.receive_timeout() == 321); + + a.init(false, false); + + // ...but throws once initialized. + REQUIRE_THROWS_AS(a.set_settings_timeout(999), Mads::AgentError); + REQUIRE_THROWS_AS(a.set_settings_timeout(std::chrono::milliseconds(999)), + Mads::AgentError); + + // set_receive_timeout() still has no guard post-init(). + a.set_receive_timeout(444); + REQUIRE(a.receive_timeout() == 444); +} + +// --------------------------------------------------------------------------- +// "none" settings URI (documented test mode, src/agent.hpp:232): compare +// against a local-file equivalent to pin down the synthesized defaults +// (src/agent.cpp:365-371). +// --------------------------------------------------------------------------- + +TEST_CASE("settings_uri \"none\" synthesizes minimal defaults", + "[agent_settings]") { + mads_test::RunningGuard guard; + Mads::Agent a("noneagent", "none"); + a.init(false, false); + + REQUIRE(a.settings_are_local()); + REQUIRE(a.settings_uri() == "none"); + REQUIRE(a.pub_topic() == "noneagent"); + REQUIRE(a.sub_topic() == std::vector{""}); + REQUIRE(a.get_settings().at("pub_topic") == "noneagent"); +} From fcd2b9b343b17cd64d496511458d559993700a6c Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:09:27 +0000 Subject: [PATCH 06/14] ci: fail coverage workflow on Codecov upload errors now that the token is set --- .github/workflows/coverage.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 21f9a99..104dd14 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -54,9 +54,9 @@ jobs: gcovr --config gcovr.cfg --object-directory build-cov \ --gcov-executable "llvm-cov gcov" - - name: Upload to Codecov + - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v5 with: files: coverage.xml token: ${{ secrets.CODECOV_TOKEN }} - fail_ci_if_error: false # tolerate the token not being set yet + fail_ci_if_error: true From 2cfa434c318f8510031ca25a4d757da480936a3b Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:26:13 +0000 Subject: [PATCH 07/14] test: add unit tests for the C ABI wrapper (agent_c) and CURVE crypto helper tests/test_agent_c.cpp (23 cases, 137 assertions): free functions (mads_version/mads_default_settings_uri/mads_free), full create/init/destroy lifecycle and accessor sweep (id, pub/sub topics incl. malloc-on-demand convention, receive timeout, high watermark, wire format, compression, settings getters against tests/fixtures/settings/valid.toml), agent_init error paths (missing file, malformed TOML, missing section, double-init), agent_connect/agent_disconnect guarded states, agent_register_event before/after connect, discover_broker_settings parameter guards (no network touched), CURVE setup via the C ABI (string-based and file-based key paths), a full C-ABI pub/sub round trip over tcp://127.0.0.1:42303 (endpoint/cross wiring done via the underlying Mads::Agent*, since agent_c.h has no equivalent), a mixed C++-publisher/C-API-subscriber blob-receive check, and message_type_t/event_type_t <-> Mads::message_type/event_type enum parity. tests/test_curve.cpp (13 cases, 35 assertions): CurveAuth::fetch_public_keys, set_key_dir, key getters/setters, setup_curve_server and both setup_curve_client overloads (missing-file and happy paths, all keys generated at runtime via zmqpp::curve::generate_keypair() into a temp dir), setup_auth, and a full CURVE-encrypted REQ/REP round trip with ZAP authentication over tcp://127.0.0.1:42350. Full suite: 138/138 ctest passing (102 pre-existing + 36 new). Coverage: src/agent_c.cpp 78% lines (0% before), src/curve.hpp 96% lines. Source bug found (not fixed, per work-package scope): in src/agent_c.cpp, agent_set_client_public_key/secret_key/server_public_key guard against "CurveAuth not initialized" with `if (!ag->curve_auth())`, but Agent::curve_auth() returns `&_curve_auth` (address of a member), which is never null. Calling any of these three setters before agent_setup_crypto() dereferences a null CurveAuth* and segfaults instead of returning -1; the "before setup" precondition is therefore untestable without triggering a crash and is not exercised. Remaining uncovered lines in agent_c.cpp are: discover_broker_settings's real-discovery body (network access via UDP broadcast, out of scope per the no-network-in-tests rule), the six lines of that dead branch above, several catch(...) "Unexpected" fallbacks that require throwing a non-std::exception type, and a malloc-failure branch -- all judged not worth a hostile setup per the work-package guidance. --- tests/test_agent_c.cpp | 661 +++++++++++++++++++++++++++++++++++++++++ tests/test_curve.cpp | 344 +++++++++++++++++++++ 2 files changed, 1005 insertions(+) create mode 100644 tests/test_agent_c.cpp create mode 100644 tests/test_curve.cpp diff --git a/tests/test_agent_c.cpp b/tests/test_agent_c.cpp new file mode 100644 index 0000000..7bcaed1 --- /dev/null +++ b/tests/test_agent_c.cpp @@ -0,0 +1,661 @@ +// Unit tests for the pure-C ABI wrapper around Mads::Agent (src/agent_c.h, +// src/agent_c.cpp). Exercised from a C++ translation unit (agent_c.h wraps +// its declarations in `extern "C"`), which lets us also reach into +// agent.hpp/mads.hpp for two purposes: +// - assembling settings fixtures / verifying results against the same +// enums the C ABI mirrors (message_type, WireFormat, Compression, +// event_type all have fixed numeric values matched by the C header). +// - reaching Mads::Agent methods that have no C ABI equivalent (set_cross, +// set_sub_endpoint, set_pub_endpoint) to pair up two C-created agents for +// a loopback pub/sub round trip, exactly like test_agent_pubsub.cpp does +// for the C++ API. agent_t is a type-erased Agent*, so +// reinterpret_cast(handle) refers to the very same object +// the C API is operating on. +// +// Port range for this file: 42300-42399 (WP-D). +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "agent.hpp" +#include "agent_c.h" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +namespace { + +std::string fixture(const std::string &rel) { + return std::string(MADS_TEST_FIXTURES_DIR) + "/" + rel; +} + +// Repeatedly (re-)publishes until check_received becomes true, absorbing the +// ZMQ "slow joiner" propagation delay. Mirrors the helper used in +// test_agent_pubsub.cpp, kept local since we may not edit that file. +template +bool retry_until(PublishFn publish_once, CheckFn check_received, + std::chrono::milliseconds total = 3000ms, + std::chrono::milliseconds settle = 150ms) { + auto deadline = std::chrono::steady_clock::now() + total; + while (std::chrono::steady_clock::now() < deadline) { + publish_once(); + if (mads_test::wait_for(check_received, settle, 10ms)) + return true; + } + return false; +} + +} // namespace + +// --------------------------------------------------------------------------- +// Free functions +// --------------------------------------------------------------------------- + +TEST_CASE("mads_version/mads_default_settings_uri return stable, non-empty " + "strings", + "[agent_c]") { + const char *v = mads_version(); + REQUIRE(v != nullptr); + REQUIRE(std::string(v).size() > 0); + + const char *uri = mads_default_settings_uri(); + REQUIRE(uri != nullptr); + // Just check it is a stable pointer to a sane string, content is whatever + // was compiled in. + REQUIRE(std::string(uri) == std::string(mads_default_settings_uri())); +} + +TEST_CASE("mads_free tolerates a NULL pointer", "[agent_c]") { + mads_free(nullptr); // free(NULL) is well-defined; must not crash + SUCCEED(); +} + +// --------------------------------------------------------------------------- +// create / init / destroy lifecycle, accessor sweep +// --------------------------------------------------------------------------- + +TEST_CASE("agent_create -> agent_init(\"none\") -> full accessor sweep -> " + "agent_destroy", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabsweep", "none"); + REQUIRE(a != nullptr); + REQUIRE(agent_init(a, false) == 0); + + // id + agent_set_id(a, "the-id-1"); + REQUIRE(std::string(agent_id(a)) == "the-id-1"); + + // pub topic + agent_set_pub_topic(a, "mytopic"); + REQUIRE(std::string(agent_pub_topic(a)) == "mytopic"); + + // sub topics: set two, read back via the malloc-on-demand convention. + const char *topics_in[] = {"topicA", "topicB"}; + agent_set_sub_topics(a, topics_in, 2); + char *topics_out = nullptr; + size_t n_topics = 0; + int rc = agent_sub_topics(a, &topics_out, &n_topics); + REQUIRE(rc == 2); + REQUIRE(n_topics == 2); + REQUIRE(std::string(&topics_out[0 * 256]) == "topicA"); + REQUIRE(std::string(&topics_out[1 * 256]) == "topicB"); + mads_free(topics_out); + + // agent_topics() JSON dump + char *topics_json = agent_topics(a, 0); + REQUIRE(topics_json != nullptr); + nlohmann::json tj = nlohmann::json::parse(std::string(topics_json)); + REQUIRE(tj.at("publish") == "mytopic"); + REQUIRE(tj.at("subscribe") == nlohmann::json::array({"topicA", "topicB"})); + + // receive timeout + agent_set_receive_timeout(a, 321); + REQUIRE(agent_receive_timeout(a) == 321); + + // high watermark + REQUIRE(agent_set_high_watermark(a, 55) == 0); + REQUIRE(agent_high_watermark(a) == 55); + + // wire format + REQUIRE(agent_wire_format(a) == 0); // default Json + REQUIRE(agent_set_wire_format(a, 1) == 0); + REQUIRE(agent_wire_format(a) == 1); + REQUIRE(agent_set_wire_format(a, 42) == -1); // invalid + REQUIRE(std::string(agent_last_error()).size() > 0); + + // compression + REQUIRE(agent_compression(a) == 2); // default Auto + REQUIRE(agent_set_compression(a, 0) == 0); + REQUIRE(agent_compression(a) == 0); + REQUIRE(agent_set_compression(a, 99) == -1); // invalid + REQUIRE(std::string(agent_last_error()).size() > 0); + + // settings uri / settings dump / print + REQUIRE(std::string(agent_settings_uri(a)) == "none"); + const char *settings_json = agent_get_settings(a, 2); + REQUIRE(settings_json != nullptr); + nlohmann::json sj = nlohmann::json::parse(std::string(settings_json)); + REQUIRE(sj.contains("pub_topic")); + agent_print_settings(a, 0); // just must not crash; output goes to stdout + + // loop watchdog: install and let destroy() join it cleanly. + agent_install_loop_watchdog(a); + + agent_destroy(a); + SUCCEED("agent_destroy completed without hanging or crashing"); +} + +// --------------------------------------------------------------------------- +// agent_set_sub_topics with zero topics (used to build a pure publisher). +// --------------------------------------------------------------------------- + +TEST_CASE("agent_set_sub_topics(0 topics) clears the subscribe list", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabnosub", "none"); + REQUIRE(agent_init(a, false) == 0); + agent_set_sub_topics(a, nullptr, 0); + char *topics_out = nullptr; + size_t n_topics = 0; + int rc = agent_sub_topics(a, &topics_out, &n_topics); + // sub_topic() defaults to [""] from init("none"); an explicit 0-count set + // replaces it with an actually-empty vector. + REQUIRE(rc == 0); + REQUIRE(n_topics == 0); + // malloc(0) is implementation-defined (NULL or a free()-able unique + // pointer); mads_free() tolerates either. + mads_free(topics_out); + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// settings timeout: succeeds before init(), guarded after init() +// --------------------------------------------------------------------------- + +TEST_CASE("agent_set_settings_timeout succeeds pre-init, fails post-init", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabtimeout", "none"); + REQUIRE(agent_set_settings_timeout(a, 500) == 0); + REQUIRE(agent_settings_timeout(a) == 500); + + REQUIRE(agent_init(a, false) == 0); + REQUIRE(agent_set_settings_timeout(a, 999) == -1); + REQUIRE(std::string(agent_last_error()).size() > 0); + // Value is unchanged since the setter threw before mutating state. + REQUIRE(agent_settings_timeout(a) == 500); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// Settings getters against a TOML fixture (read-only, from WP-C). +// --------------------------------------------------------------------------- + +TEST_CASE("agent_setting_bool/int/dbl/str read values from a loaded TOML " + "file, defaulting on missing keys", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("settingstest", fixture("settings/valid.toml").c_str()); + REQUIRE(agent_init(a, false) == 0); + + REQUIRE(agent_setting_bool(a, "custom_bool") == true); + REQUIRE(agent_setting_int(a, "custom_int") == 42); + REQUIRE(agent_setting_dbl(a, "custom_double") == Catch::Approx(3.14)); + REQUIRE(std::string(agent_setting_str(a, "custom_string")) == + "custom_value"); + + // Missing keys default per documented behavior. + REQUIRE(agent_setting_bool(a, "does_not_exist") == false); + REQUIRE(agent_setting_int(a, "does_not_exist") == 0); + REQUIRE(agent_setting_dbl(a, "does_not_exist") == Catch::Approx(0.0)); + REQUIRE(std::string(agent_setting_str(a, "does_not_exist")) == ""); + + REQUIRE(std::string(agent_settings_uri(a)) == fixture("settings/valid.toml")); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// Error paths: agent_init failures and agent_last_error() +// --------------------------------------------------------------------------- + +TEST_CASE("agent_init fails against a nonexistent settings file and sets " + "agent_last_error()", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabmissing", "/no/such/path/settings.toml"); + int rc = agent_init(a, false); + REQUIRE(rc == -1); + std::string err = agent_last_error(); + REQUIRE(err.find("Error initializing agent") != std::string::npos); + agent_destroy(a); +} + +TEST_CASE("agent_init fails against a malformed TOML file", "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabmalformed", fixture("settings/malformed.toml").c_str()); + REQUIRE(agent_init(a, false) == -1); + REQUIRE(std::string(agent_last_error()).size() > 0); + agent_destroy(a); +} + +TEST_CASE("agent_init fails when the settings file lacks the agent's " + "section", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = + agent_create("cabnosection", fixture("settings/missing_section.toml").c_str()); + REQUIRE(agent_init(a, false) == -1); + REQUIRE(std::string(agent_last_error()).find("Error initializing agent") != + std::string::npos); + agent_destroy(a); +} + +TEST_CASE("agent_init fails when the agent is already connected", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42300; + agent_t a = agent_create("cabreinit", "none"); + REQUIRE(agent_init(a, false) == 0); + Mads::Agent *ag = reinterpret_cast(a); + ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(a, ""); + const char *reinit_topics[] = {""}; + agent_set_sub_topics(a, reinit_topics, 1); // non-empty -> connect_sub() runs + REQUIRE(agent_connect(a, 0) == 0); + + REQUIRE(agent_init(a, false) == -1); + REQUIRE(std::string(agent_last_error()).find("already connected") != + std::string::npos); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// connect / disconnect / register_event guarded states +// --------------------------------------------------------------------------- + +TEST_CASE("agent_connect fails on double-connect; agent_disconnect is " + "idempotent", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42301; + agent_t a = agent_create("cabconn", "none"); + REQUIRE(agent_init(a, false) == 0); + Mads::Agent *ag = reinterpret_cast(a); + ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(a, ""); + const char *conn_topics[] = {""}; + agent_set_sub_topics(a, conn_topics, 1); // non-empty -> connect_sub() runs + + REQUIRE(agent_connect(a, 0) == 0); + REQUIRE(agent_connect(a, 0) == -1); + REQUIRE(std::string(agent_last_error()).find("already connected") != + std::string::npos); + + REQUIRE(agent_disconnect(a) == 0); + REQUIRE(agent_disconnect(a) == 0); // no-op the second time + + agent_destroy(a); +} + +TEST_CASE("agent_connect reports the underlying AgentError when the agent " + "was never initialized", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabuninit", "none"); // agent_init() never called + REQUIRE(agent_connect(a, 0) == -1); + REQUIRE(std::string(agent_last_error()).find("Agent not initialized") != + std::string::npos); + agent_destroy(a); +} + +TEST_CASE("agent_register_event fails before connect, succeeds after", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42302; + agent_t a = agent_create("cabevent", "none"); + REQUIRE(agent_init(a, false) == 0); + + // Not connected yet: guarded failure. Uses mads_marker (synchronous + // publish, src/agent.cpp:708-711) rather than mads_startup/mads_shutdown, + // which spawn a detached/joined worker thread carrying a raw `this` -- + // safe only if the Agent outlives that thread's delay, which a short-lived + // test case cannot guarantee. + REQUIRE(agent_register_event(a, mads_marker, nullptr) == -1); + REQUIRE(std::string(agent_last_error()).find("not connected") != + std::string::npos); + + Mads::Agent *ag = reinterpret_cast(a); + ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(a, ""); + const char *event_topics[] = {""}; + agent_set_sub_topics(a, event_topics, 1); // non-empty -> connect_sub() runs + REQUIRE(agent_connect(a, 0) == 0); + + REQUIRE(agent_register_event(a, mads_marker, nullptr) == 0); + REQUIRE(agent_register_event(a, mads_marker, "{\"note\":\"hi\"}") == 0); + // Malformed JSON payload -> parse error caught and reported. + REQUIRE(agent_register_event(a, mads_marker, "{not json") == -1); + REQUIRE(std::string(agent_last_error()).size() > 0); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// discover_broker_settings: parameter guards only (no real network activity) +// --------------------------------------------------------------------------- + +TEST_CASE("discover_broker_settings rejects invalid output buffer " + "parameters without touching the network", + "[agent_c]") { + REQUIRE(discover_broker_settings("room", nullptr, 0) == -1); + REQUIRE(std::string(agent_last_error()).find("Invalid output buffer") != + std::string::npos); + + char fixed[4]; + char *fixed_ptr = fixed; + REQUIRE(discover_broker_settings("room", &fixed_ptr, 0) == -1); + REQUIRE(std::string(agent_last_error()).find("Invalid output buffer") != + std::string::npos); +} + +// --------------------------------------------------------------------------- +// Crypto setup via the C ABI, string-based keys (no key files needed). +// --------------------------------------------------------------------------- + +TEST_CASE("agent_set_client_public_key/secret_key/server_public_key succeed " + "once agent_setup_crypto has run", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabcrypto1", "none"); + + // ZMQ validates CURVE key length/encoding (32 raw bytes or 40-char Z85) as + // soon as the socket option is set, so real keypairs are needed even + // though this test never opens a network connection. + zmqpp::curve::keypair client_kp = zmqpp::curve::generate_keypair(); + zmqpp::curve::keypair server_kp = zmqpp::curve::generate_keypair(); + + // NOTE (source bug, not fixed per work-package scope): the "before + // agent_setup_crypto" guard in agent_set_client_public_key/secret_key/ + // server_public_key (src/agent_c.cpp:117-145) checks `if (!ag->curve_auth())`, + // but Agent::curve_auth() returns `&_curve_auth` (the address of a member + // variable), which is never null -- the check should instead test whether + // the pointed-to unique_ptr itself is empty (`!*ag->curve_auth()` or + // `!ag->curve_auth()->get()`). As written, calling any of these three + // setters before agent_setup_crypto() dereferences a null CurveAuth* and + // segfaults instead of returning -1. Exercising that path is therefore + // skipped here; only the documented post-setup_crypto usage is covered. + + REQUIRE(agent_setup_crypto(a, false) == 0); + + REQUIRE(agent_set_client_public_key(a, client_kp.public_key.c_str()) == 0); + REQUIRE(agent_set_client_secret_key(a, client_kp.secret_key.c_str()) == 0); + REQUIRE(agent_set_server_public_key(a, server_kp.public_key.c_str()) == 0); + + // init(crypto=true) with keys already set via the string path (no files + // touched): Agent::init() takes the "curve_auth already exists" branch. + REQUIRE(agent_init(a, true) == 0); + + agent_destroy(a); +} + +TEST_CASE("agent_init(crypto=true) with an incomplete key setup fails and " + "reports an error", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabcrypto2", "none"); + REQUIRE(agent_setup_crypto(a, false) == 0); + // Only the client public key is set; secret/server keys are missing. + zmqpp::curve::keypair client_kp = zmqpp::curve::generate_keypair(); + REQUIRE(agent_set_client_public_key(a, client_kp.public_key.c_str()) == 0); + REQUIRE(agent_init(a, true) == -1); + REQUIRE(std::string(agent_last_error()).find("Error initializing agent") != + std::string::npos); + agent_destroy(a); +} + +TEST_CASE("agent_set_key_dir/agent_set_client_key_name/" + "agent_set_server_key_name/agent_set_auth_verbose do not throw", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabkeydir", "none"); + agent_set_key_dir(a, "/some/dir/that/need/not/exist/yet"); + agent_set_client_key_name(a, "myclient"); + agent_set_server_key_name(a, "myserver"); + agent_set_auth_verbose(a, true); + agent_set_auth_verbose(a, false); + SUCCEED("setters completed without throwing"); + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// Pub/sub round trip entirely through the C ABI's publish/receive/last_* +// functions. Endpoint/cross wiring (not exposed in the C header) is done by +// reaching through to the underlying Mads::Agent, exactly as init() requires +// it be done: after agent_init() and before agent_connect(). +// --------------------------------------------------------------------------- + +TEST_CASE("C ABI pub/sub round trip: publish(JSON) -> receive -> " + "agent_last_message", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42303; + + agent_t pub = agent_create("cpubA", "none"); + REQUIRE(agent_init(pub, false) == 0); + Mads::Agent *pub_ag = reinterpret_cast(pub); + pub_ag->set_cross(true); + pub_ag->set_sub_endpoint(mads_test::loopback(port)); + // Leave pub_topic at its non-empty default (the agent name) so that + // connect() actually runs connect_pub() (src/agent.cpp:641-644 gates on + // !_pub_topic.empty()); clear sub_topic to skip connect_sub() for this + // pure publisher, mirroring make_pub() in test_agent_pubsub.cpp. + agent_set_sub_topics(pub, nullptr, 0); + REQUIRE(agent_connect(pub, 0) == 0); + + agent_t sub = agent_create("csubA", "none"); + REQUIRE(agent_init(sub, false) == 0); + Mads::Agent *sub_ag = reinterpret_cast(sub); + sub_ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(sub, ""); + const char *sub_topics[] = {""}; + agent_set_sub_topics(sub, sub_topics, 1); + REQUIRE(agent_connect(sub, 0) == 0); + + bool got = retry_until( + [&] { REQUIRE(agent_publish(pub, "{\"hello\":\"world\",\"n\":7}", + "topicX") == 0); }, + [&] { return agent_receive(sub, true) == mads_json; }); + REQUIRE(got); + + char *topic = nullptr; + char *message = nullptr; + agent_last_message(sub, &topic, &message); + REQUIRE(std::string(topic) == "topicX"); + nlohmann::json doc = nlohmann::json::parse(std::string(message)); + REQUIRE(doc.at("hello") == "world"); + REQUIRE(doc.at("n") == 7); + + // Publishing invalid JSON is rejected before it ever reaches the wire. + REQUIRE(agent_publish(pub, "{not json", "topicX") == -1); + REQUIRE(std::string(agent_last_error()).find("Invalid JSON") != + std::string::npos); + + agent_disconnect(pub); + agent_disconnect(sub); + agent_destroy(pub); + agent_destroy(sub); +} + +TEST_CASE("agent_publish and agent_receive fail while not connected", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabnotconn", "none"); + REQUIRE(agent_init(a, false) == 0); + + REQUIRE(agent_publish(a, "{}", "t") == -1); + REQUIRE(std::string(agent_last_error()).find("not connected") != + std::string::npos); + + REQUIRE(agent_receive(a, true) == mads_none); + REQUIRE(std::string(agent_last_error()).find("not connected") != + std::string::npos); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// agent_sub_topics: output-buffer parameter guards and the "too small" +// branch (src/agent_c.cpp:409-449). +// --------------------------------------------------------------------------- + +TEST_CASE("agent_sub_topics validates output parameters, honors a " + "caller-supplied buffer, and detects an oversized topic name", + "[agent_c]") { + mads_test::RunningGuard guard; + agent_t a = agent_create("cabsubtopics", "none"); + REQUIRE(agent_init(a, false) == 0); + const char *topics_in[] = {"short"}; + agent_set_sub_topics(a, topics_in, 1); + + // Both output pointers NULL. + size_t n = 0; + REQUIRE(agent_sub_topics(a, nullptr, &n) == -1); + REQUIRE(std::string(agent_last_error()) + .find("Invalid output buffer pointer") != std::string::npos); + char *buf = nullptr; + REQUIRE(agent_sub_topics(a, &buf, nullptr) == -1); + + // Caller-supplied (non-null) buffer with a declared size of zero. + char fixed[8] = {0}; + char *fixed_ptr = fixed; + size_t zero_n = 0; + REQUIRE(agent_sub_topics(a, &fixed_ptr, &zero_n) == -1); + REQUIRE(std::string(agent_last_error()).find("Invalid output buffer size") != + std::string::npos); + + // Caller-supplied, correctly-sized buffer: pre-existing content is + // cleared and then refilled. + char caller_buf[256]; + std::memset(caller_buf, 'X', sizeof(caller_buf)); + char *caller_ptr = caller_buf; + size_t caller_n = 1; + int rc = agent_sub_topics(a, &caller_ptr, &caller_n); + REQUIRE(rc == 1); + REQUIRE(caller_n == 1); + REQUIRE(std::string(caller_buf) == "short"); + + // A topic name at/above the fixed 256-byte-per-topic slot size overflows + // it; snprintf() reports the untruncated length, which the wrapper uses + // to detect and report the overflow instead of returning a corrupt topic. + std::string long_topic(300, 'a'); + const char *long_topics[] = {long_topic.c_str()}; + agent_set_sub_topics(a, long_topics, 1); + char *auto_buf = nullptr; + size_t auto_n = 0; + REQUIRE(agent_sub_topics(a, &auto_buf, &auto_n) == -1); + REQUIRE(std::string(agent_last_error()).find("Output buffer too small") != + std::string::npos); + mads_free(auto_buf); + + agent_destroy(a); +} + +// --------------------------------------------------------------------------- +// agent_receive: the exception path (mads_error), triggered the same way +// test_agent_pubsub.cpp triggers Agent::receive()'s AgentError -- threaded +// remote control owns the subscriber socket. enable_remote_control() has no +// C ABI equivalent, so it is called through the underlying Mads::Agent*. +// --------------------------------------------------------------------------- + +TEST_CASE("agent_receive reports mads_error when the underlying receive() " + "throws", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42305; + agent_t a = agent_create("cabrecverr", "none"); + REQUIRE(agent_init(a, false) == 0); + Mads::Agent *ag = reinterpret_cast(a); + ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(a, ""); + // Must be called before connect(): pushes "control" onto _sub_topic + // (non-empty -> connect_sub() runs) and hands the subscriber socket to a + // background thread. + ag->enable_remote_control(true); + REQUIRE(agent_connect(a, 0) == 0); + + REQUIRE(agent_receive(a, true) == mads_error); + REQUIRE(std::string(agent_last_error()).find("Error receiving message") != + std::string::npos); + + agent_destroy(a); +} + +// A raw C++ Agent (blob publisher) paired with a C-ABI-created subscriber: +// there is no blob-publish entry point in the C ABI, but agent_receive() +// must still report mads_blob correctly when a blob frame arrives, since the +// message_type_t <-> Mads::message_type mapping is part of this wrapper. +TEST_CASE("agent_receive reports mads_blob for a blob frame published by a " + "C++ peer", + "[agent_c]") { + mads_test::RunningGuard guard; + const uint16_t port = 42304; + + Mads::Agent pub_ag("cppblobpub", "none"); + pub_ag.init(false, false); + pub_ag.set_cross(true); + pub_ag.set_sub_endpoint(mads_test::loopback(port)); + pub_ag.set_sub_topic({}); + pub_ag.connect(std::chrono::milliseconds(0)); + + agent_t sub = agent_create("cblobsub", "none"); + REQUIRE(agent_init(sub, false) == 0); + Mads::Agent *sub_ag = reinterpret_cast(sub); + sub_ag->set_sub_endpoint(mads_test::loopback(port)); + agent_set_pub_topic(sub, ""); + const char *sub_topics[] = {""}; + agent_set_sub_topics(sub, sub_topics, 1); + REQUIRE(agent_connect(sub, 0) == 0); + + std::string data = "raw-bytes"; + nlohmann::json meta = {{"format", "raw"}}; + bool got = retry_until( + [&] { pub_ag.publish(data.data(), data.size(), meta, "blobtopic"); }, + [&] { return agent_receive(sub, true) == mads_blob; }); + REQUIRE(got); + + agent_destroy(sub); +} + +// --------------------------------------------------------------------------- +// message_type_t / event_type_t numeric values match Mads::message_type / +// Mads::event_type (the C ABI depends on the two enumerations staying in +// lock-step; this pins that contract). +// --------------------------------------------------------------------------- + +TEST_CASE("message_type_t and event_type_t values mirror the C++ enums", + "[agent_c]") { + REQUIRE(static_cast(mads_none) == static_cast(Mads::message_type::none)); + REQUIRE(static_cast(mads_json) == static_cast(Mads::message_type::json)); + REQUIRE(static_cast(mads_blob) == static_cast(Mads::message_type::blob)); + REQUIRE(static_cast(mads_error) == static_cast(Mads::message_type::error)); + + REQUIRE(static_cast(mads_marker) == static_cast(Mads::event_type::marker)); + REQUIRE(static_cast(mads_marker_in) == static_cast(Mads::event_type::marker_in)); + REQUIRE(static_cast(mads_marker_out) == static_cast(Mads::event_type::marker_out)); + REQUIRE(static_cast(mads_startup) == static_cast(Mads::event_type::startup)); + REQUIRE(static_cast(mads_shutdown) == static_cast(Mads::event_type::shutdown)); + REQUIRE(static_cast(mads_message) == static_cast(Mads::event_type::message)); +} diff --git a/tests/test_curve.cpp b/tests/test_curve.cpp new file mode 100644 index 0000000..67cffa6 --- /dev/null +++ b/tests/test_curve.cpp @@ -0,0 +1,344 @@ +// Unit tests for Mads::CurveAuth (src/curve.hpp), the ZAP/CURVE helper used +// by Mads::Agent and by the C ABI's crypto entry points (agent_c.cpp). +// +// Keys are generated at runtime with zmqpp::curve::generate_keypair() and +// written as .pub / .key files into a fresh subdirectory of +// std::filesystem::temp_directory_path(), removed at the end of each test +// (RAII helper below) -- no fixture files are checked in. +// +// Port range for this file: 42300-42399 (WP-D), disjoint from test_agent_c.cpp +// (42300-42304 used there); this file uses 42350+. +#include + +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "curve.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; +namespace fs = std::filesystem; + +namespace { + +// Creates a unique temp directory for one test case and removes it (and its +// contents) on scope exit. +struct TempKeyDir { + fs::path path; + TempKeyDir() { + path = fs::temp_directory_path() / + ("mads_test_curve_" + + std::to_string(std::chrono::steady_clock::now() + .time_since_epoch() + .count())); + fs::create_directories(path); + } + ~TempKeyDir() { + std::error_code ec; + fs::remove_all(path, ec); + } +}; + +void write_keypair(const fs::path &dir, const std::string &name, + const zmqpp::curve::keypair &kp) { + { + std::ofstream pub(dir / (name + ".pub")); + pub << kp.public_key << "\n"; + } + { + std::ofstream key(dir / (name + ".key")); + key << kp.secret_key << "\n"; + } +} + +} // namespace + +// --------------------------------------------------------------------------- +// fetch_public_keys() +// --------------------------------------------------------------------------- + +TEST_CASE("fetch_public_keys throws when the directory does not exist", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + bool threw = false; + try { + auth.fetch_public_keys("/no/such/curve/key/dir/at/all"); + } catch (const std::exception &e) { + threw = true; + REQUIRE(std::string(e.what()).find("does not exist") != std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("fetch_public_keys throws when the directory has no .pub files", + "[curve]") { + TempKeyDir dir; + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + bool threw = false; + try { + auth.fetch_public_keys(dir.path); + } catch (const std::exception &e) { + threw = true; + REQUIRE(std::string(e.what()).find("No client public keys found") != + std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("fetch_public_keys succeeds when .pub files are present", + "[curve]") { + TempKeyDir dir; + auto kp1 = zmqpp::curve::generate_keypair(); + auto kp2 = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "clientA", kp1); + write_keypair(dir.path, "clientB", kp2); + // A stray non-.pub file must be ignored, not mistaken for a key. + { std::ofstream junk(dir.path / "notes.txt"); junk << "hello"; } + + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + REQUIRE_NOTHROW(auth.fetch_public_keys(dir.path)); +} + +// --------------------------------------------------------------------------- +// set_key_dir() / getters +// --------------------------------------------------------------------------- + +TEST_CASE("set_key_dir throws for a nonexistent directory and succeeds for " + "a valid one", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + REQUIRE_THROWS_AS(auth.set_key_dir("/still/not/a/real/dir"), + std::runtime_error); + + TempKeyDir dir; + REQUIRE_NOTHROW(auth.set_key_dir(dir.path)); +} + +TEST_CASE("client/server key getters and setters round-trip and default to " + "empty", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + REQUIRE(auth.client_public_key().empty()); + REQUIRE(auth.client_secret_key().empty()); + REQUIRE(auth.server_public_key().empty()); + + auth.set_client_public_key("pubkeyvalue"); + auth.set_client_secret_key("seckeyvalue"); + auth.set_server_public_key("srvkeyvalue"); + + REQUIRE(auth.client_public_key() == "pubkeyvalue"); + REQUIRE(auth.client_secret_key() == "seckeyvalue"); + REQUIRE(auth.server_public_key() == "srvkeyvalue"); +} + +// --------------------------------------------------------------------------- +// setup_curve_server() +// --------------------------------------------------------------------------- + +TEST_CASE("setup_curve_server throws when fetch_public_keys was never " + "called (key dir unset)", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + zmqpp::socket sock(ctx, zmqpp::socket_type::rep); + bool threw = false; + try { + auth.setup_curve_server(sock, "server"); + } catch (const std::exception &e) { + threw = true; + REQUIRE(std::string(e.what()).find("Key directory not set") != + std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("setup_curve_server throws when the server key files are missing", + "[curve]") { + TempKeyDir dir; + // Only a client key is present; no "server.pub"/"server.key". + auto client_kp = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "client", client_kp); + + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + auth.fetch_public_keys(dir.path); + zmqpp::socket sock(ctx, zmqpp::socket_type::rep); + bool threw = false; + try { + auth.setup_curve_server(sock, "server"); + } catch (const std::exception &e) { + threw = true; + REQUIRE(std::string(e.what()).find("server public key file") != + std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("setup_curve_server succeeds and configures the socket when key " + "files are present", + "[curve]") { + TempKeyDir dir; + auto server_kp = zmqpp::curve::generate_keypair(); + auto client_kp = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "server", server_kp); + write_keypair(dir.path, "client", client_kp); + + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + auth.fetch_public_keys(dir.path); // picks up both server.pub and client.pub + zmqpp::socket sock(ctx, zmqpp::socket_type::rep); + REQUIRE_NOTHROW(auth.setup_curve_server(sock, "server")); +} + +// --------------------------------------------------------------------------- +// setup_curve_client() -- file-based overload +// --------------------------------------------------------------------------- + +TEST_CASE("setup_curve_client(file-based) throws when key files are " + "missing", + "[curve]") { + TempKeyDir dir; + auto client_kp = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "client", client_kp); + // No "server.pub" present. + + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + REQUIRE_NOTHROW(auth.set_key_dir(dir.path)); + zmqpp::socket sock(ctx, zmqpp::socket_type::req); + bool threw = false; + try { + auth.setup_curve_client(sock, "client", "server"); + } catch (const std::exception &e) { + threw = true; + REQUIRE(std::string(e.what()).find("server public key file") != + std::string::npos); + } + REQUIRE(threw); +} + +TEST_CASE("setup_curve_client(file-based) succeeds when all key files are " + "present", + "[curve]") { + TempKeyDir dir; + auto client_kp = zmqpp::curve::generate_keypair(); + auto server_kp = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "client", client_kp); + write_keypair(dir.path, "server", server_kp); + + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + auth.set_key_dir(dir.path); + zmqpp::socket sock(ctx, zmqpp::socket_type::req); + REQUIRE_NOTHROW(auth.setup_curve_client(sock, "client", "server")); + REQUIRE(auth.client_public_key() == client_kp.public_key); + REQUIRE(auth.client_secret_key() == client_kp.secret_key); + REQUIRE(auth.server_public_key() == server_kp.public_key); +} + +// --------------------------------------------------------------------------- +// setup_curve_client() -- raw-key overload +// --------------------------------------------------------------------------- + +TEST_CASE("setup_curve_client(no-arg) throws until all three keys are set, " + "then succeeds", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + zmqpp::socket sock(ctx, zmqpp::socket_type::req); + + REQUIRE_THROWS_AS(auth.setup_curve_client(sock), std::runtime_error); + + auto client_kp = zmqpp::curve::generate_keypair(); + auto server_kp = zmqpp::curve::generate_keypair(); + auth.set_client_public_key(client_kp.public_key); + REQUIRE_THROWS_AS(auth.setup_curve_client(sock), std::runtime_error); + auth.set_client_secret_key(client_kp.secret_key); + REQUIRE_THROWS_AS(auth.setup_curve_client(sock), std::runtime_error); + auth.set_server_public_key(server_kp.public_key); + + REQUIRE_NOTHROW(auth.setup_curve_client(sock)); +} + +// --------------------------------------------------------------------------- +// setup_auth(): whitelist configuration must not throw with a real context. +// --------------------------------------------------------------------------- + +TEST_CASE("setup_auth configures the ZAP authenticator without throwing", + "[curve]") { + zmqpp::context ctx; + Mads::CurveAuth auth(ctx); + auth.allowed_ips.push_back("127.0.0.1"); + REQUIRE_NOTHROW(auth.setup_auth(Mads::auth_verbose::off)); +} + +// --------------------------------------------------------------------------- +// Stretch: end-to-end CURVE-encrypted REQ/REP round trip including the ZAP +// authenticator, over tcp://127.0.0.1:. +// --------------------------------------------------------------------------- + +TEST_CASE("full CURVE-encrypted REQ/REP round trip with ZAP authentication", + "[curve]") { + const uint16_t port = 42350; + TempKeyDir dir; + auto server_kp = zmqpp::curve::generate_keypair(); + auto client_kp = zmqpp::curve::generate_keypair(); + write_keypair(dir.path, "server", server_kp); + write_keypair(dir.path, "client", client_kp); + + // Server side: the ZAP authenticator must live in the same context as the + // server socket it protects. + zmqpp::context server_ctx; + Mads::CurveAuth server_auth(server_ctx); + server_auth.allowed_ips.push_back("127.0.0.1"); + server_auth.setup_auth(Mads::auth_verbose::off); + server_auth.fetch_public_keys(dir.path); // registers client.pub (and server.pub) + + zmqpp::socket server_sock(server_ctx, zmqpp::socket_type::rep); + server_auth.setup_curve_server(server_sock, "server"); + server_sock.set(zmqpp::socket_option::receive_timeout, 200); + server_sock.bind(mads_test::loopback(port)); + + // Client side: separate context (as a different process would have). + zmqpp::context client_ctx; + Mads::CurveAuth client_auth(client_ctx); + client_auth.set_key_dir(dir.path); + zmqpp::socket client_sock(client_ctx, zmqpp::socket_type::req); + client_auth.setup_curve_client(client_sock, "client", "server"); + client_sock.set(zmqpp::socket_option::receive_timeout, 3000); + client_sock.connect(mads_test::loopback(port)); + + zmqpp::message request; + request << "ping"; + REQUIRE(client_sock.send(request)); + + bool server_got = mads_test::wait_for( + [&] { + zmqpp::message in; + if (!server_sock.receive(in)) return false; + REQUIRE(in.get(0) == "ping"); + zmqpp::message reply; + reply << "pong"; + server_sock.send(reply); + return true; + }, + 3000ms, 50ms); + REQUIRE(server_got); + + zmqpp::message response; + bool client_got = client_sock.receive(response); + REQUIRE(client_got); + REQUIRE(response.get(0) == "pong"); +} From 1ee7acae0863c41abe25bd4cccd24be75e8e88d2 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:28:24 +0000 Subject: [PATCH 08/14] fix: correct CurveAuth null guard in C API key setters agent_set_client_public_key/secret_key/server_public_key guarded with !ag->curve_auth(), but curve_auth() returns the address of the member unique_ptr, which is never null. Calling any of these setters before agent_setup_crypto() dereferenced a null CurveAuth and crashed instead of returning -1 as documented. Check the pointed-to unique_ptr instead; re-enable the unit-test assertions for the precondition. --- src/agent_c.cpp | 6 +++--- tests/test_agent_c.cpp | 15 +++++---------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/agent_c.cpp b/src/agent_c.cpp index e782687..447f07f 100644 --- a/src/agent_c.cpp +++ b/src/agent_c.cpp @@ -116,7 +116,7 @@ void agent_set_auth_verbose(agent_t agent, bool verbose) { int agent_set_client_public_key(agent_t agent, const char *key) { Agent *ag = reinterpret_cast(agent); - if (!ag->curve_auth()) { + if (!*ag->curve_auth()) { snprintf(_err_msg, ERR_MSG_SIZE, "Error setting client public key: CurveAuth not initialized"); return -1; } @@ -126,7 +126,7 @@ int agent_set_client_public_key(agent_t agent, const char *key) { int agent_set_client_secret_key(agent_t agent, const char *key) { Agent *ag = reinterpret_cast(agent); - if (!ag->curve_auth()) { + if (!*ag->curve_auth()) { snprintf(_err_msg, ERR_MSG_SIZE, "Error setting client secret key: CurveAuth not initialized"); return -1; } @@ -136,7 +136,7 @@ int agent_set_client_secret_key(agent_t agent, const char *key) { int agent_set_server_public_key(agent_t agent, const char *key) { Agent *ag = reinterpret_cast(agent); - if (!ag->curve_auth()) { + if (!*ag->curve_auth()) { snprintf(_err_msg, ERR_MSG_SIZE, "Error setting server public key: CurveAuth not initialized"); return -1; } diff --git a/tests/test_agent_c.cpp b/tests/test_agent_c.cpp index 7bcaed1..4e7121f 100644 --- a/tests/test_agent_c.cpp +++ b/tests/test_agent_c.cpp @@ -387,16 +387,11 @@ TEST_CASE("agent_set_client_public_key/secret_key/server_public_key succeed " zmqpp::curve::keypair client_kp = zmqpp::curve::generate_keypair(); zmqpp::curve::keypair server_kp = zmqpp::curve::generate_keypair(); - // NOTE (source bug, not fixed per work-package scope): the "before - // agent_setup_crypto" guard in agent_set_client_public_key/secret_key/ - // server_public_key (src/agent_c.cpp:117-145) checks `if (!ag->curve_auth())`, - // but Agent::curve_auth() returns `&_curve_auth` (the address of a member - // variable), which is never null -- the check should instead test whether - // the pointed-to unique_ptr itself is empty (`!*ag->curve_auth()` or - // `!ag->curve_auth()->get()`). As written, calling any of these three - // setters before agent_setup_crypto() dereferences a null CurveAuth* and - // segfaults instead of returning -1. Exercising that path is therefore - // skipped here; only the documented post-setup_crypto usage is covered. + // Before agent_setup_crypto() the CurveAuth is not initialized yet, so the + // key setters must fail cleanly with -1 and report an error. + REQUIRE(agent_set_client_public_key(a, client_kp.public_key.c_str()) == -1); + REQUIRE(agent_set_client_secret_key(a, client_kp.secret_key.c_str()) == -1); + REQUIRE(agent_set_server_public_key(a, server_kp.public_key.c_str()) == -1); REQUIRE(agent_setup_crypto(a, false) == 0); From 0e5cbf8400b4fe64cbe8bd188cf28b68a372d0a0 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:31:57 +0000 Subject: [PATCH 09/14] test: fix CI-only failures: track test fixtures, derive version in test The repo-wide *.toml gitignore rule silently kept tests/fixtures out of the WP-C commit (tests passed locally against untracked files, failed on CI with 'File could not be opened for reading'); negate the rule for tests/fixtures/**. Also derive the same-minor version string in the check_version test from Mads::version() instead of hardcoding v0.0.999, which only matched on tag-less clones. --- .gitignore | 1 + tests/fixtures/settings/malformed.toml | 10 ++++++++++ tests/fixtures/settings/missing_section.toml | 10 ++++++++++ tests/fixtures/settings/no_subtopic.toml | 8 ++++++++ tests/fixtures/settings/string_subtopic.toml | 9 +++++++++ tests/fixtures/settings/valid.toml | 20 ++++++++++++++++++++ tests/test_mads_core.cpp | 6 ++++-- 7 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 tests/fixtures/settings/malformed.toml create mode 100644 tests/fixtures/settings/missing_section.toml create mode 100644 tests/fixtures/settings/no_subtopic.toml create mode 100644 tests/fixtures/settings/string_subtopic.toml create mode 100644 tests/fixtures/settings/valid.toml diff --git a/.gitignore b/.gitignore index 3b55d90..d8af58b 100644 --- a/.gitignore +++ b/.gitignore @@ -32,6 +32,7 @@ TODO.md !mads.ini !rust/**/Cargo.toml !share/templates/rust_Cargo.toml +!tests/fixtures/** _*.md diff --git a/tests/fixtures/settings/malformed.toml b/tests/fixtures/settings/malformed.toml new file mode 100644 index 0000000..1192ab7 --- /dev/null +++ b/tests/fixtures/settings/malformed.toml @@ -0,0 +1,10 @@ +# Fixture for test_agent_settings.cpp: intentionally malformed TOML (an +# unterminated string value) to exercise the toml::parse_error path in +# toml::parse_file() (src/agent.cpp:374). + +[agents] +timecode_fps = 25 + +[badcfg] +pub_topic = "no closing quote +sub_topic = ["a", "b"] diff --git a/tests/fixtures/settings/missing_section.toml b/tests/fixtures/settings/missing_section.toml new file mode 100644 index 0000000..3e0a763 --- /dev/null +++ b/tests/fixtures/settings/missing_section.toml @@ -0,0 +1,10 @@ +# Fixture for test_agent_settings.cpp: valid TOML, but with no table matching +# the agent name that will be requested ("missingsection") -> Agent::init() +# must throw AgentError("Invalid settings file: missing '...' section") +# (src/agent.cpp:387-389). + +[agents] +timecode_fps = 25 + +[othername] +pub_topic = "other" diff --git a/tests/fixtures/settings/no_subtopic.toml b/tests/fixtures/settings/no_subtopic.toml new file mode 100644 index 0000000..f7adeb0 --- /dev/null +++ b/tests/fixtures/settings/no_subtopic.toml @@ -0,0 +1,8 @@ +# Fixture for test_agent_settings.cpp: no sub_topic key at all -> the agent's +# _sub_topic vector must stay empty (src/agent.cpp:408-410). + +[agents] +timecode_fps = 25 + +[nosub] +pub_topic = "nosub_pub" diff --git a/tests/fixtures/settings/string_subtopic.toml b/tests/fixtures/settings/string_subtopic.toml new file mode 100644 index 0000000..bb40b27 --- /dev/null +++ b/tests/fixtures/settings/string_subtopic.toml @@ -0,0 +1,9 @@ +# Fixture for test_agent_settings.cpp: sub_topic given as a single string +# instead of an array (Agent::init() supports both, src/agent.cpp:403-413). + +[agents] +timecode_fps = 25 + +[strsub] +pub_topic = "strsub_pub" +sub_topic = "onlytopic" diff --git a/tests/fixtures/settings/valid.toml b/tests/fixtures/settings/valid.toml new file mode 100644 index 0000000..daac5c2 --- /dev/null +++ b/tests/fixtures/settings/valid.toml @@ -0,0 +1,20 @@ +# Fixture for test_agent_settings.cpp: a full settings file mirroring the +# shape of the repo-root mads.ini, with a fleet-wide [agents] section and one +# agent section ("settingstest") that exercises string/array/scalar types. + +[agents] +frontend_address = "tcp://localhost:42210" +backend_address = "tcp://localhost:42211" +timecode_fps = 25 +wire_format = "json" +compression = "auto" + +[settingstest] +pub_topic = "settingstest_pub" +sub_topic = ["topicA", "topicB"] +time_step = 100 +queue_size = 5 +custom_string = "custom_value" +custom_int = 42 +custom_bool = true +custom_double = 3.14 diff --git a/tests/test_mads_core.cpp b/tests/test_mads_core.cpp index 351223e..2e0681b 100644 --- a/tests/test_mads_core.cpp +++ b/tests/test_mads_core.cpp @@ -21,8 +21,10 @@ TEST_CASE("check_version matches on identical minor version", "[mads_core]") { // The library's own version always matches its own check. REQUIRE(Mads::check_version(Mads::version())); // Same major.minor, different patch also matches, since only the part - // before the last dot is compared. - REQUIRE(Mads::check_version("v0.0.999")); + // before the last dot is compared. Derive the string from the library's + // own version, which depends on the git tags visible at configure time. + std::string v = Mads::version(); + REQUIRE(Mads::check_version(v.substr(0, v.find_last_of('.')) + ".999")); } TEST_CASE("check_version rejects a non-matching minor version", From 7860d6943f8deb85ae8602f063d9ac591ca1869d Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 16:40:01 +0000 Subject: [PATCH 10/14] test: cover the plugin migration engine (plugin_migrate.hpp) 47 Catch2 test cases / 189 assertions across helpers (line_of_offset, is_ident_char, find_matching_paren, read_file/write_file, run_command), detect_protocol/bump_git_tag, apply_literal/apply_regex, load_migrations against the real share/plugin_migrations chain, SpanRewriter/MethodTransform on tricky snippets (multi-line signatures, parens in comments/strings, call vs. declaration disambiguation), and run() end to end against copies of a new fixture plugin project (tests/fixtures/plugin_p6/): dry-run, full P6->P8 migration with backups, already-at-target, missing CMakeLists.txt, from/to overrides, missing chain step, undeterminable target, no-op run, and undetectable protocol. Full suite: 185/185 ctest passed (138 pre-existing + 47 new). src/main/plugin_migrate.hpp coverage: 87% lines (445/507), 88.8% functions. Uncovered: the Tier B compile-check block (invokes cmake/network, out of scope for a no-network unit suite), the Windows _popen branch and a popen() failure branch, and a few lexer edge branches (escaped chars, char literals) already exercised indirectly via find_matching_paren's own test cases. --- tests/fixtures/plugin_p6/CMakeLists.txt | 53 ++ tests/fixtures/plugin_p6/src/p6demo.cpp | 47 + .../fixtures/plugin_p6/src/p6demo_helper.cpp | 12 + tests/test_plugin_migrate.cpp | 807 ++++++++++++++++++ 4 files changed, 919 insertions(+) create mode 100644 tests/fixtures/plugin_p6/CMakeLists.txt create mode 100644 tests/fixtures/plugin_p6/src/p6demo.cpp create mode 100644 tests/fixtures/plugin_p6/src/p6demo_helper.cpp create mode 100644 tests/test_plugin_migrate.cpp diff --git a/tests/fixtures/plugin_p6/CMakeLists.txt b/tests/fixtures/plugin_p6/CMakeLists.txt new file mode 100644 index 0000000..6ab7e73 --- /dev/null +++ b/tests/fixtures/plugin_p6/CMakeLists.txt @@ -0,0 +1,53 @@ +# ____ _ _ +# | _ \| |_ _ __ _(_)_ __ +# | |_) | | | | |/ _` | | '_ \ +# | __/| | |_| | (_| | | | | | +# |_| |_|\__,_|\__, |_|_| |_| +# |___/ +# Fake plugin project (fixture) used by tests/test_plugin_migrate.cpp to drive +# Mads::PluginMigrate::run() end to end. Modeled after share/templates/CMakeLists.txt +# but pinned at protocol P6, i.e. pre-P6->P7 migration. +cmake_minimum_required(VERSION 3.28) +project(p6demo VERSION 0.1.0 LANGUAGES CXX) + +set(CMAKE_CXX_STANDARD 17) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(SRC_DIR ${CMAKE_CURRENT_LIST_DIR}/src) + +include(FetchContent) +# pugg is for the plugin system +FetchContent_Declare(pugg + GIT_REPOSITORY https://github.com/pbosetti/pugg.git + GIT_TAG 1.0.0 + GIT_SHALLOW TRUE +) + +FetchContent_Declare(json + GIT_REPOSITORY https://github.com/nlohmann/json.git + GIT_TAG v3.11.3 + GIT_SHALLOW TRUE +) + +FetchContent_MakeAvailable(pugg json) + +# NOTE: keep this block's only GIT_TAG-with-protocol-suffix token being the +# real pin below; detect_protocol() is a plain (non-comment-aware) regex scan, +# so a second such token anywhere in the block -- even inside a "#" comment -- +# would be picked up first. (bump_git_tag()'s comment-awareness is exercised +# with synthetic strings in test_plugin_migrate.cpp instead.) +FetchContent_Populate(plugin + # pinned to the mads_plugin repo at the P6 protocol tag + GIT_REPOSITORY https://github.com/pbosetti/mads_plugin.git + GIT_TAG v2.3-P6 + GIT_SHALLOW TRUE + SUBBUILD_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps/plugin-subbuild + SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps/plugin-src + BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps/plugin-build +) + +include_directories(${plugin_SOURCE_DIR}/src) +include_directories(${json_SOURCE_DIR}/include) + +add_library(p6demo SHARED ${SRC_DIR}/p6demo.cpp ${SRC_DIR}/p6demo_helper.cpp) +target_link_libraries(p6demo PRIVATE pugg) +set_target_properties(p6demo PROPERTIES PREFIX "" SUFFIX ".plugin") diff --git a/tests/fixtures/plugin_p6/src/p6demo.cpp b/tests/fixtures/plugin_p6/src/p6demo.cpp new file mode 100644 index 0000000..8080343 --- /dev/null +++ b/tests/fixtures/plugin_p6/src/p6demo.cpp @@ -0,0 +1,47 @@ +// Fake P6-protocol plugin source used as migration input by +// tests/test_plugin_migrate.cpp. The method signatures below deliberately +// mirror the *pre*-P6->P7 shapes described in +// share/plugin_migrations/P6-P7.json, plus a couple of tricky lexical cases +// (a comment and a string literal that contain '(' right next to a real +// signature, and a signature split across several lines). +#include +#include +#include + +using json = nlohmann::json; +using namespace std; + +class P6demoPlugin { +public: + const char *kind() override { return "source"; } + + // set_params(...) used to take an opaque pointer (pre-P7 ABI) + std::string debug_msg = "about to call set_params(...) with a raw pointer"; + void set_params(void *params) override { + _params.merge_patch(*(json *)params); + } + + // load_data(...) reads the next sample (raw pointer form used pre-P7) + bool load_data( + json const &input + ) override { + _last = input; + return true; + } + + bool process() override { + /* process() takes no arguments yet (pre-P7 signature) */ + return true; + } + + bool get_output(json &out) override { + out = _last; + return true; + } + +private: + json _params; + json _last; +}; + +INSTALL_SOURCE_DRIVER(P6demoPlugin, json) diff --git a/tests/fixtures/plugin_p6/src/p6demo_helper.cpp b/tests/fixtures/plugin_p6/src/p6demo_helper.cpp new file mode 100644 index 0000000..fbb1f32 --- /dev/null +++ b/tests/fixtures/plugin_p6/src/p6demo_helper.cpp @@ -0,0 +1,12 @@ +// Helper translation unit for the plugin_p6 fixture: exercises a +// set_params call site passing a raw pointer (pre-P7 calling convention), +// which the P6->P7 migration step rewrites to pass by reference instead. +#include + +using json = nlohmann::json; + +class P6demoPlugin; + +void configure(P6demoPlugin &instance, json ¶ms) { + instance.set_params(¶ms); +} diff --git a/tests/test_plugin_migrate.cpp b/tests/test_plugin_migrate.cpp new file mode 100644 index 0000000..290ec7b --- /dev/null +++ b/tests/test_plugin_migrate.cpp @@ -0,0 +1,807 @@ +#include + +#include +#include +#include +#include +#include +#include +#include + +#include "main/plugin_migrate.hpp" + +namespace fs = std::filesystem; +using namespace Mads::PluginMigrate; + +// --------------------------------------------------------------------------- +// Test-local helpers: every test operates on copies under +// std::filesystem::temp_directory_path(); the repo fixture at +// tests/fixtures/plugin_p6/ is only ever read, never mutated. +// --------------------------------------------------------------------------- +namespace { + +fs::path unique_temp_path(const std::string &tag) { + static std::atomic counter{0}; + return fs::temp_directory_path() / + ("mads_ptm_" + tag + "_" + std::to_string(::getpid()) + "_" + + std::to_string(counter++)); +} + +// RAII: removes the temp path (file or directory tree) on scope exit. +struct ScopedPath { + fs::path p; + explicit ScopedPath(fs::path p_) : p(std::move(p_)) {} + ~ScopedPath() { + std::error_code ec; + fs::remove_all(p, ec); + } +}; + +fs::path fixture_dir() { return fs::path(MADS_TEST_FIXTURES_DIR) / "plugin_p6"; } + +fs::path migrations_dir() { + return fs::path(MADS_PROJECT_SOURCE_DIR) / "share" / "plugin_migrations"; +} + +fs::path deps_manifest() { + return fs::path(MADS_PROJECT_SOURCE_DIR) / "share" / "plugin_deps.json"; +} + +// Copies the fixture plugin project into a fresh temp directory and returns +// its path. Never touches the repo fixture. +fs::path copy_fixture(const std::string &tag) { + fs::path dst = unique_temp_path(tag); + fs::copy(fixture_dir(), dst, fs::copy_options::recursive); + return dst; +} + +} // namespace + +// --------------------------------------------------------------------------- +// 1. Pure helpers +// --------------------------------------------------------------------------- + +TEST_CASE("is_ident_char classifies identifier characters", + "[plugin_migrate][helpers]") { + REQUIRE(is_ident_char('a')); + REQUIRE(is_ident_char('Z')); + REQUIRE(is_ident_char('9')); + REQUIRE(is_ident_char('_')); + REQUIRE_FALSE(is_ident_char(' ')); + REQUIRE_FALSE(is_ident_char('(')); + REQUIRE_FALSE(is_ident_char('.')); +} + +TEST_CASE("line_of_offset counts 1-based lines", "[plugin_migrate][helpers]") { + std::string s = "aaa\nbbb\nccc\n"; + REQUIRE(line_of_offset(s, 0) == 1); + REQUIRE(line_of_offset(s, 3) == 1); // still before the first '\n' + REQUIRE(line_of_offset(s, 4) == 2); // just after the first '\n' + REQUIRE(line_of_offset(s, 8) == 3); + // Offset beyond the string is clamped to size(), i.e. counts every '\n'. + REQUIRE(line_of_offset(s, 1000) == 4); +} + +TEST_CASE("read_file / write_file round-trip", "[plugin_migrate][helpers]") { + ScopedPath dir(unique_temp_path("rw")); + fs::create_directories(dir.p); + fs::path f = dir.p / "roundtrip.txt"; + std::string content = "line1\nline2\nwith \"quotes\" and (parens)\n"; + + REQUIRE(write_file(f, content)); + std::string out; + REQUIRE(read_file(f, out)); + REQUIRE(out == content); +} + +TEST_CASE("read_file fails on a missing file", "[plugin_migrate][helpers]") { + std::string out = "unchanged"; + REQUIRE_FALSE(read_file(fs::temp_directory_path() / "mads_ptm_does_not_exist.txt", out)); +} + +TEST_CASE("write_file fails when the parent directory does not exist", + "[plugin_migrate][helpers]") { + fs::path bogus = unique_temp_path("nowrite") / "sub" / "file.txt"; + REQUIRE_FALSE(write_file(bogus, "content")); +} + +TEST_CASE("run_command captures output and a zero exit code", + "[plugin_migrate][helpers]") { + std::string out; + int rc = run_command("echo hi", out); + REQUIRE(rc == 0); + REQUIRE(out.find("hi") != std::string::npos); +} + +TEST_CASE("run_command captures stderr too (2>&1 redirection)", + "[plugin_migrate][helpers]") { + std::string out; + int rc = run_command("sh -c \"echo err-marker 1>&2\"", out); + REQUIRE(rc == 0); + REQUIRE(out.find("err-marker") != std::string::npos); +} + +TEST_CASE("run_command returns a non-zero exit code on failure", + "[plugin_migrate][helpers]") { + std::string out; + int rc = run_command("false", out); + REQUIRE(rc != 0); +} + +TEST_CASE("find_matching_paren balances simple and nested parens", + "[plugin_migrate][helpers]") { + std::string s1 = "foo()"; + REQUIRE(find_matching_paren(s1, 3) == 4); + + std::string s2 = "foo(a, (b, c), d)"; + REQUIRE(find_matching_paren(s2, 3) == 16); + + std::string s3 = "f((((x))))"; + REQUIRE(find_matching_paren(s3, 1) == 9); +} + +TEST_CASE("find_matching_paren ignores parens inside string/char literals", + "[plugin_migrate][helpers]") { + // The '(' right after the opening paren, inside a string literal, must not + // affect depth tracking; nor must the escaped quote inside it. + std::string s = "f(\"a (b\\\" (c\" , 'x' , y)"; + std::size_t open = 1; + std::size_t close = find_matching_paren(s, open); + REQUIRE(close == s.size() - 1); +} + +TEST_CASE("find_matching_paren ignores parens inside comments", + "[plugin_migrate][helpers]") { + std::string line_comment = "f(a // ( still open\n, b)"; + REQUIRE(find_matching_paren(line_comment, 1) == line_comment.size() - 1); + + std::string block_comment = "f(a /* ( nested ( parens */, b)"; + REQUIRE(find_matching_paren(block_comment, 1) == block_comment.size() - 1); +} + +TEST_CASE("find_matching_paren returns npos on unbalanced input", + "[plugin_migrate][helpers]") { + std::string s = "f(a, (b, c)"; + REQUIRE(find_matching_paren(s, 1) == std::string::npos); +} + +// --------------------------------------------------------------------------- +// 2. detect_protocol / bump_git_tag +// --------------------------------------------------------------------------- + +TEST_CASE("detect_protocol parses the -P suffix from GIT_TAG", + "[plugin_migrate][protocol]") { + std::string cmake = R"cmake( +FetchContent_Populate(plugin + GIT_REPOSITORY https://github.com/pbosetti/mads_plugin.git + GIT_TAG v2.3-P6 +) +)cmake"; + REQUIRE(detect_protocol(cmake) == 6); +} + +TEST_CASE("detect_protocol returns -1 sentinel when absent", + "[plugin_migrate][protocol]") { + REQUIRE(detect_protocol("project(x)\n") == -1); + // No protocol suffix on the tag. + std::string cmake = R"cmake( +FetchContent_Populate(plugin + GIT_TAG v2.3 +) +)cmake"; + REQUIRE(detect_protocol(cmake) == -1); +} + +TEST_CASE("detect_protocol requires FetchContent_Populate specifically", + "[plugin_migrate][protocol]") { + // A Declare-only block (no Populate) does not match: this documents the + // engine's actual (narrower) detection scope rather than assuming it. + std::string cmake = R"cmake( +FetchContent_Declare(plugin + GIT_TAG v2.3-P6 +) +)cmake"; + REQUIRE(detect_protocol(cmake) == -1); +} + +TEST_CASE("bump_git_tag replaces a bare GIT_TAG token", "[plugin_migrate][protocol]") { + std::string cmake = R"cmake( +FetchContent_Declare(pugg + GIT_REPOSITORY https://github.com/pbosetti/pugg.git + GIT_TAG 1.0.0 + GIT_SHALLOW TRUE +) +)cmake"; + std::vector changes; + int n = bump_git_tag(cmake, "pugg", "1.1.0", changes); + REQUIRE(n == 1); + REQUIRE(changes.size() == 1); + REQUIRE(cmake.find("1.1.0") != std::string::npos); + REQUIRE(cmake.find("GIT_TAG 1.0.0") == std::string::npos); +} + +TEST_CASE("bump_git_tag replaces a quoted GIT_TAG value", "[plugin_migrate][protocol]") { + std::string cmake = R"cmake( +FetchContent_Declare(plugin + GIT_TAG "v2.3-P6" +) +)cmake"; + std::vector changes; + int n = bump_git_tag(cmake, "plugin", "v2.3-P7", changes); + REQUIRE(n == 1); + REQUIRE(cmake.find("\"v2.3-P7\"") != std::string::npos); +} + +TEST_CASE("bump_git_tag is comment-aware: a fake GIT_TAG in a # comment is skipped", + "[plugin_migrate][protocol]") { + std::string cmake = R"cmake( +FetchContent_Populate(plugin + # legacy pin, superseded below: GIT_TAG v1.0-P5 (deprecated) + GIT_REPOSITORY https://github.com/pbosetti/mads_plugin.git + GIT_TAG v2.3-P6 +) +)cmake"; + std::vector changes; + int n = bump_git_tag(cmake, "plugin", "v2.3-P7", changes); + REQUIRE(n == 1); + REQUIRE(cmake.find("v1.0-P5") != std::string::npos); // comment untouched + REQUIRE(cmake.find("GIT_TAG v2.3-P7") != std::string::npos); + REQUIRE(cmake.find("GIT_TAG v2.3-P6") == std::string::npos); +} + +TEST_CASE("bump_git_tag is a no-op when the block is absent", + "[plugin_migrate][protocol]") { + std::string cmake = "project(x)\n"; + std::string original = cmake; + std::vector changes; + int n = bump_git_tag(cmake, "plugin", "v2.3-P7", changes); + REQUIRE(n == 0); + REQUIRE(changes.empty()); + REQUIRE(cmake == original); +} + +TEST_CASE("bump_git_tag is idempotent when already at the target tag", + "[plugin_migrate][protocol]") { + std::string cmake = R"cmake( +FetchContent_Declare(plugin + GIT_TAG v2.3-P7 +) +)cmake"; + std::string original = cmake; + std::vector changes; + int n = bump_git_tag(cmake, "plugin", "v2.3-P7", changes); + REQUIRE(n == 0); + REQUIRE(changes.empty()); + REQUIRE(cmake == original); +} + +TEST_CASE("bump_git_tag handles a new_tag starting with a digit", + "[plugin_migrate][protocol]") { + // Regression guard for the exact hazard documented in the header: naive + // std::regex_replace with "$1" + new_tag breaks when new_tag starts with a + // digit (e.g. "$11.2.0" is read as capture group 11). + std::string cmake = R"cmake( +FetchContent_Declare(pugg + GIT_TAG 1.1.0 +) +)cmake"; + std::vector changes; + int n = bump_git_tag(cmake, "pugg", "1.2.0", changes); + REQUIRE(n == 1); + REQUIRE(cmake.find("1.2.0") != std::string::npos); +} + +// --------------------------------------------------------------------------- +// 3. apply_literal / apply_regex +// --------------------------------------------------------------------------- + +TEST_CASE("apply_literal replaces every occurrence", "[plugin_migrate][transforms]") { + std::string src = "a(x); a(x); a(x);"; + std::vector changes; + int n = apply_literal(src, "a(x)", "b(y)", "rename call", changes); + REQUIRE(n == 3); + REQUIRE(changes.size() == 3); + REQUIRE(src == "b(y); b(y); b(y);"); +} + +TEST_CASE("apply_literal returns 0 and leaves the buffer untouched on no match", + "[plugin_migrate][transforms]") { + std::string src = "unrelated content"; + std::string original = src; + std::vector changes; + int n = apply_literal(src, "needle", "replacement", "desc", changes); + REQUIRE(n == 0); + REQUIRE(changes.empty()); + REQUIRE(src == original); +} + +TEST_CASE("apply_regex replaces all matches but logs a single Change", + "[plugin_migrate][transforms]") { + std::string src = "INSTALL_SOURCE_DRIVER(Foo, json)\nINSTALL_SOURCE_DRIVER(Bar, json)\n"; + std::vector changes; + int n = apply_regex(src, R"(INSTALL_SOURCE_DRIVER\s*\(\s*([A-Za-z_]\w*)\s*,[^)]*\))", + "MADS_REGISTER_PLUGINS($1)", "", "driver macro", changes); + REQUIRE(n == 2); + REQUIRE(changes.size() == 1); // one Change entry regardless of match count + REQUIRE(src == "MADS_REGISTER_PLUGINS(Foo)\nMADS_REGISTER_PLUGINS(Bar)\n"); +} + +TEST_CASE("apply_regex returns 0 on no match and leaves the buffer untouched", + "[plugin_migrate][transforms]") { + std::string src = "nothing to see here"; + std::string original = src; + std::vector changes; + int n = apply_regex(src, "NOPE_(\\w+)", "$1", "", "desc", changes); + REQUIRE(n == 0); + REQUIRE(changes.empty()); + REQUIRE(src == original); +} + +TEST_CASE("apply_regex honours the case-insensitive flag", "[plugin_migrate][transforms]") { + std::string src = "Hello WORLD"; + std::vector changes; + int n = apply_regex(src, "world", "there", "i", "desc", changes); + REQUIRE(n == 1); + REQUIRE(src == "Hello there"); +} + +// --------------------------------------------------------------------------- +// 4. load_migrations against the real share/plugin_migrations directory +// --------------------------------------------------------------------------- + +TEST_CASE("load_migrations chains the real P6->P7 and P7->P8 steps", + "[plugin_migrate][migrations]") { + auto steps = load_migrations(migrations_dir()); + REQUIRE(steps.size() == 2); + REQUIRE(steps.count(6) == 1); + REQUIRE(steps.count(7) == 1); + REQUIRE(steps.at(6).from == 6); + REQUIRE(steps.at(6).to == 7); + REQUIRE(steps.at(6).lang == "cpp"); + REQUIRE(steps.at(7).from == 7); + REQUIRE(steps.at(7).to == 8); + + // Spot-check the method transforms present in the P6->P7 document. + const json &p6p7 = steps.at(6).doc; + REQUIRE(p6p7.contains("source")); + std::vector method_names; + for (const auto &t : p6p7["source"]) + if (t.value("kind", "") == "method") + method_names.push_back(t.value("name", "")); + REQUIRE(method_names.size() == 4); + REQUIRE(std::find(method_names.begin(), method_names.end(), "set_params") != + method_names.end()); + REQUIRE(std::find(method_names.begin(), method_names.end(), "load_data") != + method_names.end()); + REQUIRE(std::find(method_names.begin(), method_names.end(), "process") != + method_names.end()); + REQUIRE(std::find(method_names.begin(), method_names.end(), "get_output") != + method_names.end()); + + REQUIRE(p6p7["cmake"]["plugin_git_tag"] == "v2.3-P7"); + REQUIRE(p6p7["cmake"]["pugg_git_tag"] == "1.1.0"); + + const json &p7p8 = steps.at(7).doc; + REQUIRE(p7p8["cmake"]["plugin_git_tag"] == "v2.4-P8"); + REQUIRE(p7p8["cmake"]["pugg_git_tag"] == "1.2.0"); +} + +TEST_CASE("load_migrations returns an empty map for a non-existent directory", + "[plugin_migrate][migrations]") { + auto steps = load_migrations(unique_temp_path("no_such_dir")); + REQUIRE(steps.empty()); +} + +TEST_CASE("load_migrations skips malformed JSON and non-cpp/non-chaining entries", + "[plugin_migrate][migrations]") { + ScopedPath dir(unique_temp_path("migsyn")); + fs::create_directories(dir.p); + + REQUIRE(write_file(dir.p / "ok.json", + R"({"from": 6, "to": 7, "lang": "cpp"})")); + REQUIRE(write_file(dir.p / "broken.json", "{ this is not json")); + REQUIRE(write_file(dir.p / "wrong_lang.json", + R"({"from": 1, "to": 2, "lang": "rust"})")); + REQUIRE(write_file(dir.p / "non_chaining.json", + R"({"from": 2, "to": 9, "lang": "cpp"})")); + REQUIRE(write_file(dir.p / "not_json.txt", "irrelevant")); + + auto steps = load_migrations(dir.p); + REQUIRE(steps.size() == 1); + REQUIRE(steps.count(6) == 1); +} + +// --------------------------------------------------------------------------- +// 5. SpanRewriter / MethodTransform on realistic, tricky snippets +// --------------------------------------------------------------------------- + +TEST_CASE("SpanRewriter rewrites a simple single-line signature", + "[plugin_migrate][rewriter]") { + std::string src = " void set_params(void *params) override {\n _p = params;\n }\n"; + MethodTransform t; + t.name = "set_params"; + t.params = "(const json ¶ms)"; + t.require_qualifier = "override"; + t.description = "set_params now takes a json&"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); + REQUIRE(changes.size() == 1); + REQUIRE(changes[0].line == 1); + REQUIRE(src.find("set_params(const json ¶ms) override") != std::string::npos); +} + +TEST_CASE("SpanRewriter handles a signature split across multiple lines", + "[plugin_migrate][rewriter]") { + std::string src = + " bool load_data(\n" + " json const &input\n" + " ) override {\n" + " return true;\n" + " }\n"; + MethodTransform t; + t.name = "load_data"; + t.params = "(json const &input, string topic = \"\", vector const *blob = nullptr)"; + t.require_qualifier = "override"; + t.description = "load_data gains topic/blob"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); + REQUIRE(src.find("load_data(json const &input, string topic = \"\", " + "vector const *blob = nullptr) override") != + std::string::npos); + // The multi-line whitespace that used to separate '(' and the parameter is + // gone: the whole span from '(' to ')' was replaced verbatim. + REQUIRE(src.find("json const &input\n") == std::string::npos); +} + +TEST_CASE("SpanRewriter ignores '(' inside a line comment right before the signature", + "[plugin_migrate][rewriter]") { + std::string src = + " // load_data(...) used a raw pointer pre-P7\n" + " bool load_data(json const &input) override { return true; }\n"; + MethodTransform t; + t.name = "load_data"; + t.params = "(json const &input, string topic)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); + REQUIRE(changes[0].line == 2); + REQUIRE(src.find("// load_data(...) used a raw pointer pre-P7") != std::string::npos); +} + +TEST_CASE("SpanRewriter ignores '(' inside a block comment and a string literal", + "[plugin_migrate][rewriter]") { + std::string src = + " /* process(...) takes no args (yet) */\n" + " std::string msg = \"about to call process(...) now\";\n" + " bool process() override { return true; }\n"; + MethodTransform t; + t.name = "process"; + t.params = "(json &out, vector *blob = nullptr)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); + REQUIRE(src.find("process(json &out, vector *blob = nullptr) override") != + std::string::npos); + REQUIRE(src.find("\"about to call process(...) now\"") != std::string::npos); +} + +TEST_CASE("SpanRewriter skips call sites (member/scope-qualified) and only " + "rewrites declarations", + "[plugin_migrate][rewriter]") { + std::string src = + " instance.set_params(¶ms);\n" + " Ns::set_params(¶ms);\n" + " void set_params(void *params) override { }\n"; + MethodTransform t; + t.name = "set_params"; + t.params = "(const json ¶ms)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); + REQUIRE(src.find("instance.set_params(¶ms)") != std::string::npos); + REQUIRE(src.find("Ns::set_params(¶ms)") != std::string::npos); + REQUIRE(src.find("void set_params(const json ¶ms) override") != std::string::npos); +} + +TEST_CASE("SpanRewriter requires the qualifier when one is configured", + "[plugin_migrate][rewriter]") { + std::string src = " void set_params(void *params) { /* no override */ }\n"; + MethodTransform t; + t.name = "set_params"; + t.params = "(const json ¶ms)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 0); + REQUIRE(changes.empty()); + REQUIRE(src.find("void set_params(void *params)") != std::string::npos); +} + +TEST_CASE("SpanRewriter rewrites multiple declarations in one buffer, last-to-first", + "[plugin_migrate][rewriter]") { + std::string src = + " bool process() override { return true; }\n" + " // a second overload further down\n" + " bool process() override { return false; }\n"; + MethodTransform t; + t.name = "process"; + t.params = "(json &out)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 2); + REQUIRE(changes.size() == 2); + std::size_t first = src.find("process(json &out) override"); + std::size_t second = src.find("process(json &out) override", first + 1); + REQUIRE(first != std::string::npos); + REQUIRE(second != std::string::npos); +} + +TEST_CASE("SpanRewriter returns 0 when the method name does not occur", + "[plugin_migrate][rewriter]") { + std::string src = " void unrelated() override { }\n"; + MethodTransform t; + t.name = "set_params"; + t.params = "(const json ¶ms)"; + t.require_qualifier = "override"; + + auto rw = make_rewriter(t); + std::vector changes; + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 0); + REQUIRE(src == " void unrelated() override { }\n"); +} + +TEST_CASE("make_rewriter falls back to SpanRewriter when no ts_query is set", + "[plugin_migrate][rewriter]") { + MethodTransform t; + t.name = "foo"; + auto rw = make_rewriter(t); + REQUIRE(rw != nullptr); + std::string src = "void foo() override {}\n"; + std::vector changes; + // Just confirm the returned object is a working SpanRewriter (Tier A), + // since MADS_ENABLE_TREE_SITTER is not built in this configuration. + int n = rw->rewrite_method(src, t, changes); + REQUIRE(n == 1); +} + +// --------------------------------------------------------------------------- +// 6. run() end-to-end against a temp copy of tests/fixtures/plugin_p6 +// --------------------------------------------------------------------------- + +TEST_CASE("run() dry-run leaves every file untouched", "[plugin_migrate][run]") { + fs::path project = copy_fixture("dryrun"); + ScopedPath cleanup(project); + + std::string cmake_before, src_before, helper_before; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_before)); + REQUIRE(read_file(project / "src" / "p6demo.cpp", src_before)); + REQUIRE(read_file(project / "src" / "p6demo_helper.cpp", helper_before)); + + Options opts; + opts.dry_run = true; + opts.check = false; + int rc = run(project, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 0); + + std::string cmake_after, src_after, helper_after; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_after)); + REQUIRE(read_file(project / "src" / "p6demo.cpp", src_after)); + REQUIRE(read_file(project / "src" / "p6demo_helper.cpp", helper_after)); + + REQUIRE(cmake_after == cmake_before); + REQUIRE(src_after == src_before); + REQUIRE(helper_after == helper_before); + REQUIRE_FALSE(fs::exists(project / "CMakeLists.txt.bak")); + REQUIRE_FALSE(fs::exists(project / "src" / "p6demo.cpp.bak")); +} + +TEST_CASE("run() migrates P6 -> P8 end to end with the compile check disabled", + "[plugin_migrate][run]") { + fs::path project = copy_fixture("full"); + ScopedPath cleanup(project); + + Options opts; + opts.dry_run = false; + opts.check = false; // never invoke cmake/network from the test suite + int rc = run(project, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 0); + + std::string cmake_after; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_after)); + // Both migration steps' cmake transforms applied, in order: final tag is + // the P7->P8 target, not the intermediate P6->P7 one. + REQUIRE(cmake_after.find("v2.4-P8") != std::string::npos); + REQUIRE(cmake_after.find("v2.3-P6") == std::string::npos); + REQUIRE(cmake_after.find("1.2.0") != std::string::npos); + // The nlohmann/json FetchContent block was modernized to the tarball form. + REQUIRE(cmake_after.find("json.tar.xz") != std::string::npos); + REQUIRE(cmake_after.find("GIT_TAG v3.11.3") == std::string::npos); + + std::string src_after; + REQUIRE(read_file(project / "src" / "p6demo.cpp", src_after)); + REQUIRE(src_after.find("set_params(const json ¶ms) override") != std::string::npos); + REQUIRE(src_after.find("_params.merge_patch(params)") != std::string::npos); + REQUIRE(src_after.find("*(json *)params") == std::string::npos); + REQUIRE(src_after.find("load_data(json const &input, string topic = \"\", " + "vector const *blob = nullptr) override") != + std::string::npos); + REQUIRE(src_after.find("process(json &out, vector *blob = nullptr) override") != + std::string::npos); + REQUIRE(src_after.find("get_output(json &out, vector *blob = nullptr) override") != + std::string::npos); + // P7->P8: the INSTALL_SOURCE_DRIVER macro is replaced. + REQUIRE(src_after.find("MADS_REGISTER_PLUGINS(P6demoPlugin)") != std::string::npos); + REQUIRE(src_after.find("INSTALL_SOURCE_DRIVER") == std::string::npos); + // Untouched declarations/comments/strings survive. + REQUIRE(src_after.find("about to call set_params(...)") != std::string::npos); + REQUIRE(src_after.find("const char *kind() override") != std::string::npos); + + std::string helper_after; + REQUIRE(read_file(project / "src" / "p6demo_helper.cpp", helper_after)); + REQUIRE(helper_after.find(".set_params(params)") != std::string::npos); + REQUIRE(helper_after.find(".set_params(¶ms)") == std::string::npos); + + // Backups of the pre-migration originals were written. + REQUIRE(fs::exists(project / "CMakeLists.txt.bak")); + REQUIRE(fs::exists(project / "src" / "p6demo.cpp.bak")); + REQUIRE(fs::exists(project / "src" / "p6demo_helper.cpp.bak")); + std::string cmake_bak; + REQUIRE(read_file(project / "CMakeLists.txt.bak", cmake_bak)); + REQUIRE(cmake_bak.find("v2.3-P6") != std::string::npos); +} + +TEST_CASE("run() reports nothing-to-do when already at/above the target protocol", + "[plugin_migrate][run]") { + fs::path project = copy_fixture("attarget"); + ScopedPath cleanup(project); + + std::string cmake_before; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_before)); + + Options opts; + opts.check = false; + opts.to_override = 6; // == the auto-detected current protocol + int rc = run(project, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 0); + + std::string cmake_after; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_after)); + REQUIRE(cmake_after == cmake_before); + REQUIRE_FALSE(fs::exists(project / "CMakeLists.txt.bak")); +} + +TEST_CASE("run() fails with an error code when CMakeLists.txt is missing", + "[plugin_migrate][run]") { + ScopedPath project(unique_temp_path("nocmake")); + fs::create_directories(project.p); + + Options opts; + opts.check = false; + int rc = run(project.p, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 1); +} + +TEST_CASE("run() fails with an error code when the protocol cannot be auto-detected", + "[plugin_migrate][run]") { + ScopedPath project(unique_temp_path("noproto")); + fs::create_directories(project.p); + REQUIRE(write_file(project.p / "CMakeLists.txt", "project(noproto)\n")); + + Options opts; + opts.check = false; // no --from override, and no GIT_TAG v*-P to detect + int rc = run(project.p, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 1); +} + +TEST_CASE("run() honours --from/--to overrides and stops short of the full chain", + "[plugin_migrate][run]") { + fs::path project = copy_fixture("override"); + ScopedPath cleanup(project); + + // Strip the -P6 suffix so auto-detection would fail without --from. + std::string cmake; + REQUIRE(read_file(project / "CMakeLists.txt", cmake)); + std::string stripped = cmake; + auto pos = stripped.find("v2.3-P6"); + REQUIRE(pos != std::string::npos); + stripped.replace(pos, std::string("v2.3-P6").size(), "v2.3"); + REQUIRE(detect_protocol(stripped) == -1); // confirms auto-detection would fail + REQUIRE(write_file(project / "CMakeLists.txt", stripped)); + + Options opts; + opts.check = false; + opts.from_override = 6; + opts.to_override = 7; // stop after the P6->P7 step only + int rc = run(project, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 0); + + std::string cmake_after; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_after)); + REQUIRE(cmake_after.find("v2.3-P7") != std::string::npos); + REQUIRE(cmake_after.find("v2.4-P8") == std::string::npos); + + std::string src_after; + REQUIRE(read_file(project / "src" / "p6demo.cpp", src_after)); + // P6->P7 source transforms applied... + REQUIRE(src_after.find("set_params(const json ¶ms) override") != std::string::npos); + // ...but P7->P8 did not run. + REQUIRE(src_after.find("INSTALL_SOURCE_DRIVER") != std::string::npos); + REQUIRE(src_after.find("MADS_REGISTER_PLUGINS") == std::string::npos); +} + +TEST_CASE("run() fails when a migration step is missing from the chain", + "[plugin_migrate][run]") { + fs::path project = copy_fixture("gap"); + ScopedPath cleanup(project); + + std::string cmake_before; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_before)); + + Options opts; + opts.check = false; + opts.from_override = 2; // no P2->P3 step exists in share/plugin_migrations + int rc = run(project, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 1); + + // The chain is verified before any file is touched. + std::string cmake_after; + REQUIRE(read_file(project / "CMakeLists.txt", cmake_after)); + REQUIRE(cmake_after == cmake_before); +} + +TEST_CASE("run() fails when no target protocol can be determined", + "[plugin_migrate][run]") { + fs::path project = copy_fixture("notarget"); + ScopedPath cleanup(project); + + ScopedPath empty_migrations(unique_temp_path("empty_migrations")); + fs::create_directories(empty_migrations.p); + fs::path missing_manifest = unique_temp_path("missing_manifest.json"); + + Options opts; + opts.check = false; + int rc = run(project, empty_migrations.p, missing_manifest, opts); + REQUIRE(rc == 1); +} + +TEST_CASE("run() reports no matching code when the chain applies cleanly but nothing matches", + "[plugin_migrate][run]") { + ScopedPath project(unique_temp_path("nomatch")); + fs::create_directories(project.p); + // No FetchContent(plugin/pugg) blocks and no src/ directory: the P6->P7 + // step has nothing to touch, so the run should still succeed with 0 edits. + REQUIRE(write_file(project.p / "CMakeLists.txt", "project(nomatch)\n")); + + Options opts; + opts.check = false; + opts.from_override = 6; + opts.to_override = 7; + int rc = run(project.p, migrations_dir(), deps_manifest(), opts); + REQUIRE(rc == 0); + + std::string cmake_after; + REQUIRE(read_file(project.p / "CMakeLists.txt", cmake_after)); + REQUIRE(cmake_after == "project(nomatch)\n"); + REQUIRE_FALSE(fs::exists(project.p / "CMakeLists.txt.bak")); +} From dd107b0c0d3a32c56239d39e1f8f254e680e5701 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 17:14:43 +0000 Subject: [PATCH 11/14] ci: make the 80% coverage gate blocking; add Codecov badge to README --- README.md | 2 +- codecov.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8292d0d..8847e82 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ [Documentation](https://mads-net.github.io) · [Compiling](COMPILE.md) · [Changelog](CHANGES.md) · [License](#license) -![C++20](https://img.shields.io/badge/C%2B%2B-20-blue) ![ZeroMQ](https://img.shields.io/badge/transport-ZeroMQ-orange) [![License: Apache 2.0](https://img.shields.io/badge/license-Apache%202-green)](http://www.apache.org/licenses/LICENSE-2.0) +![C++20](https://img.shields.io/badge/C%2B%2B-20-blue) ![ZeroMQ](https://img.shields.io/badge/transport-ZeroMQ-orange) [![codecov](https://codecov.io/gh/pbosetti/MADS/graph/badge.svg)](https://codecov.io/gh/pbosetti/MADS) [![License: Apache 2.0](https://img.shields.io/badge/license-Apache%202-green)](http://www.apache.org/licenses/LICENSE-2.0) diff --git a/codecov.yml b/codecov.yml index f03ace2..1e14e11 100644 --- a/codecov.yml +++ b/codecov.yml @@ -7,7 +7,7 @@ coverage: default: target: 80% threshold: 2% - informational: true # flip to false once the suite stabilizes + informational: false patch: default: target: 70% From fecd31db968c6e1266b72d8e815235c6d1eee61e Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 20:42:40 +0000 Subject: [PATCH 12/14] feat: introduce Mads::Runtime, deprecate the process-global Mads::running flag Agent run state is now owned by a Mads::Runtime object. Each agent gets its own Runtime by default, so shutting down or destroying one agent no longer stops the loops, LKV drain threads, or watchdogs of other agents in the same process, and a disconnected agent can reconnect and loop again. Agents that should stop together can share a Runtime via Agent::set_runtime(); signal handlers and the remote shutdown/restart commands request a process-wide stop (Runtime::stop_process()) that overrides every Runtime. Mads::running remains as a [[deprecated]] alias of the process-wide flag, so existing code keeps compiling and behaving as before, with a compile-time warning pointing at the replacement API. New tests: Runtime semantics, agent/runtime wiring, regression tests for the historical cross-agent stop and reconnect hazards, and the deprecated shim. Full suite: 193/193; coverage 82.4% lines. --- src/agent.cpp | 52 ++++++---- src/agent.hpp | 44 +++++++- src/agent_c.cpp | 1 - src/bridge.hpp | 2 +- src/mads.hpp.in | 51 +++++++++- src/main/broker.cpp | 18 ++-- src/main/federate.cpp | 9 +- src/main/logger.cpp | 4 +- src/main/plugin_loader.cpp | 8 +- src/main/rust_plugin_loader.cpp | 8 +- src/main/worker.cpp | 2 +- src/service_discovery.cpp | 6 +- tests/mads_test_helpers.hpp | 9 +- tests/test_agent_loop.cpp | 61 +++++------ tests/test_runtime.cpp | 170 +++++++++++++++++++++++++++++++ tools/service_discovery_demo.cpp | 4 +- 16 files changed, 362 insertions(+), 87 deletions(-) create mode 100644 tests/test_runtime.cpp diff --git a/src/agent.cpp b/src/agent.cpp index 03ba178..ed1d797 100644 --- a/src/agent.cpp +++ b/src/agent.cpp @@ -484,13 +484,14 @@ void Agent::shutdown() { if (_shutdown_done) return; _shutdown_done = true; - // 0. Stop the watchdog first so that setting Mads::running=false below does + // 0. Stop the watchdog first so that raising the stop request below does // not trip its force-exit countdown during an orderly shutdown. _watchdog_stop = true; if (_watchdog_thread.joinable()) _watchdog_thread.join(); - // 1. Signal all threads to stop - Mads::running = false; + // 1. Signal this agent's threads to stop (other agents sharing the + // Runtime are unaffected) + _stopping = true; // 2. Unblock any cv.wait() in receive_raw() LKV mode { @@ -524,8 +525,9 @@ void Agent::shutdown() { } void Agent::install_loop_watchdog(uint8_t max_count) { - // Cooperative failsafe (REFACTOR.md §1.5): when Mads::running goes false the - // main thread is expected to leave loop() and run shutdown(), which sets + // Cooperative failsafe (REFACTOR.md §1.5): when the agent is asked to stop + // (Runtime stopped or shutdown requested) the main thread is expected to + // leave loop() and run shutdown(), which sets // _watchdog_stop and joins this thread promptly. Only if the orderly path // does NOT complete within the bounded grace period do we force-exit, and we // use quick_exit() to skip static destructors that might themselves hang. @@ -535,7 +537,7 @@ void Agent::install_loop_watchdog(uint8_t max_count) { bool counting = false; steady_clock::time_point deadline{}; while (!_watchdog_stop) { - if (!Mads::running) { + if (!keep_running()) { if (!counting) { counting = true; std::signal(SIGINT, SIG_DFL); @@ -630,6 +632,9 @@ void Agent::info(ostream &out) { void Agent::connect(chrono::milliseconds delay) { if (!_init_done) throw AgentError("Agent not initialized"); + // A previous disconnect()/shutdown() must not keep a reconnected agent's + // loops from running (a process-wide stop still does). + _stopping = false; if (_connected) { try { _publisher.disconnect(_pub_endpoint); @@ -654,7 +659,8 @@ void Agent::disconnect() { if (!_connected) return; - Mads::running = false; + // Stop only this agent; other agents sharing the Runtime are unaffected. + _stopping = true; // Unblock LKV cv.wait() { @@ -836,7 +842,7 @@ inline bool Agent::receive_raw(message &message, bool dont_block) { // return true; // }); _latest_message.cv.wait(lock, [&] { - return _latest_message.value.has_value() || !Mads::running || dont_block; + return _latest_message.value.has_value() || !keep_running() || dont_block; }); if (_latest_message.value.has_value()) { message = _latest_message.value.value().copy(); @@ -964,11 +970,11 @@ void Agent::install_signal_handlers() { std::call_once(g_signal_once, []() { std::signal(SIGINT, [](int signum) { UNUSED(signum); - Mads::running = false; + Mads::Runtime::stop_process(); }); std::signal(SIGTERM, [](int signum) { UNUSED(signum); - Mads::running = false; + Mads::Runtime::stop_process(); }); }); } @@ -1002,13 +1008,13 @@ void Agent::loop(loop_fun_t const &lambda, chrono::nanoseconds duration) { throw AgentError("Agent not initialized"); install_signal_handlers(); chrono::nanoseconds nld(0); // next loop duration - while (Mads::running) { + while (keep_running()) { if (duration > 0ns || nld > 0ns) { thread t([&]() { this_thread::sleep_for(nld == 0ns ? duration : nld); }); try { nld = lambda(); } catch (...) { - Mads::running = false; + _stopping = true; } t.join(); } else { @@ -1016,7 +1022,7 @@ void Agent::loop(loop_fun_t const &lambda, chrono::nanoseconds duration) { nld = lambda(); } catch (std::exception &e) { cerr << "Exception in loop: " << e.what() << endl; - Mads::running = false; + _stopping = true; } } } @@ -1027,14 +1033,14 @@ void Agent::loop(loop_fun_t const &lambda, chrono::nanoseconds duration) { throw AgentError("Agent not initialized"); install_signal_handlers(); chrono::nanoseconds nld(0); // next loop duration - while (Mads::running) { + while (keep_running()) { chrono::nanoseconds sleep_duration = nld == 0ns ? duration : nld; auto start = chrono::steady_clock::now(); try { nld = lambda(); } catch (std::exception &e) { cerr << "Exception in loop: " << e.what() << endl; - Mads::running = false; + _stopping = true; } if (sleep_duration > 0ns) { wait_until(start + sleep_duration, _high_res_loop, _spin_margin); @@ -1069,7 +1075,7 @@ void Agent::enable_remote_control(bool threaded) { _rc_thread = thread([this]() { _subscriber.set(zmqpp::socket_option::receive_timeout, 500); message msg; - while (Mads::running) { + while (keep_running()) { if (!_subscriber.receive(msg, false)) continue; const size_t parts = msg.parts(); @@ -1113,9 +1119,9 @@ void Agent::remote_control(string payload_str) { command = payload["cmd"]; if (command == "restart") { _restart = true; - Mads::running = false; + Mads::Runtime::stop_process(); } else if (command == "shutdown") { - Mads::running = false; + Mads::Runtime::stop_process(); } else if (command == "info") { nlohmann::json response = get_settings(); response["agent"] = _name; @@ -1166,7 +1172,7 @@ void Agent::connect_sub() { if (_last_value_only) { _drain_thread = thread([this]() { zmqpp::message_t msg; - while(Mads::running && _connected) { + while(keep_running() && _connected) { try { if (!_subscriber.receive(msg, false)) continue; std::lock_guard lock(_latest_message.mtx); @@ -1265,6 +1271,14 @@ void Agent::set_receive_timeout(std::chrono::milliseconds to) { bool Agent::restart() { return _restart; } +void Agent::set_runtime(std::shared_ptr runtime) { + if (!runtime) + throw AgentError("Runtime cannot be null"); + if (_connected) + throw AgentError("Cannot change the runtime of a connected agent"); + _runtime = std::move(runtime); +} + filesystem::path Agent::attachment_path() { return _attachment_path; } diff --git a/src/agent.hpp b/src/agent.hpp index 5a9a9ca..69731cc 100644 --- a/src/agent.hpp +++ b/src/agent.hpp @@ -259,8 +259,9 @@ class Agent { /** * @brief Install a watch thread to ensure exit from loops * - * This starts a low-frequency thread that forces and exit when - * `Mads::running` remains false for more than 3 seconds + * This starts a low-frequency thread that forces an exit when the agent + * keeps looping for more than 3 seconds after it was asked to stop (its + * Runtime was stopped or shutdown/disconnect was requested) */ void install_loop_watchdog(uint8_t max_count = 3); @@ -777,6 +778,30 @@ class Agent { */ bool restart(); + /** + * @brief The Runtime that owns this agent's run state. + * + * loop(), the delivery drain thread, and the threaded remote control all + * keep going while runtime()->running() is true and the agent has not been + * individually stopped (shutdown()/disconnect()). Stopping the Runtime + * stops every agent that shares it. + * + * @return The agent's Runtime (never null). + */ + std::shared_ptr runtime() const { return _runtime; } + + /** + * @brief Attach the agent to a different Runtime. + * + * Each agent owns its own Runtime by default; attach several agents to a + * shared Runtime (e.g. another agent's runtime()) to stop them together + * without affecting the rest of the process. + * + * @param runtime The Runtime to attach to. + * @throws AgentError if runtime is null or the agent is connected. + */ + void set_runtime(std::shared_ptr runtime); + /** * @brief Returns the path to the attachment file. @@ -919,6 +944,16 @@ class Agent { */ protected: + /** + * @brief True while this agent's loops should keep going. + * + * Combines the Runtime state (group + process level) with the per-agent + * stop request raised by shutdown()/disconnect(). + */ + bool keep_running() const { + return _runtime->running() && !_stopping.load(); + } + /** * @brief Connects the agent to the publish endpoint. * @@ -971,6 +1006,11 @@ class Agent { int _settings_timeout = 0; bool _init_done = false; bool _restart = false; + std::shared_ptr _runtime = std::make_shared(); + // Per-agent stop request: set by shutdown()/disconnect(), cleared by + // connect(). Keeps an individual agent's teardown from stopping the other + // agents that share its Runtime. + std::atomic _stopping{false}; bool _remote_controlled = false; std::chrono::nanoseconds _time_step = std::chrono::nanoseconds(0); bool _high_res_loop = false; diff --git a/src/agent_c.cpp b/src/agent_c.cpp index 447f07f..9c52dc2 100644 --- a/src/agent_c.cpp +++ b/src/agent_c.cpp @@ -204,7 +204,6 @@ int agent_register_event(agent_t agent, event_type_t event, int agent_disconnect(agent_t agent) { Agent *ag = reinterpret_cast(agent); if (!ag->is_connected()) return 0; - Mads::running = false; try { ag->disconnect(); } catch (const std::exception &e) { diff --git a/src/bridge.hpp b/src/bridge.hpp index 497f5f8..ecdacc7 100644 --- a/src/bridge.hpp +++ b/src/bridge.hpp @@ -64,7 +64,7 @@ class Bridge : public Agent { json j; while (getline(cin, payload)) { if (payload == "exit") { - Mads::running = false; + Mads::Runtime::stop_process(); break; } if (regex_search(payload, match, re) && match.size() > 1) { diff --git a/src/mads.hpp.in b/src/mads.hpp.in index 140136b..1e6eaa6 100644 --- a/src/mads.hpp.in +++ b/src/mads.hpp.in @@ -24,6 +24,7 @@ Author(s): Paolo Bosetti #include #include #include +#include /* @@ -186,11 +187,59 @@ inline bool check_version(const std::string &version) { } } +/** + * @brief Run-state owner for agent loops. + * + * A Runtime holds the "keep going" state consulted by Agent::loop(), the + * delivery drain thread, and similar long-running constructs. Every Agent + * owns its own Runtime by default, so stopping or destroying one agent never + * affects the others hosted in the same process; agents that should stop + * together can be grouped by sharing one Runtime (Agent::set_runtime()), and + * the whole group is then stopped with a single Runtime::stop(). + * + * Two levels of stopping exist: + * - Runtime::stop() stops the agents attached to that Runtime only; + * - Runtime::stop_process() (used by signal handlers and the remote + * "shutdown"/"restart" commands) stops every Runtime in the process. + * + * running() reports true only if neither level has been stopped. + */ +class Runtime { +public: + /// True until this Runtime is stopped or a process-wide stop occurred. + bool running() const noexcept { + return _running.load() && process_running().load(); + } + /// Stop the loops of the agents attached to this Runtime. + void stop() noexcept { _running.store(false); } + /// Re-arm this Runtime (does not undo a process-wide stop). + void reset() noexcept { _running.store(true); } + + /// Process-wide run flag, shared by all Runtimes (async-signal-safe). + static std::atomic &process_running() noexcept { + static std::atomic flag{true}; + return flag; + } + /// Request a process-wide stop: every Runtime's running() becomes false. + static void stop_process() noexcept { process_running().store(false); } + +private: + std::atomic _running{true}; +}; + /** * @brief Global flag for stopping the main loop in Mads agents. * + * @deprecated Superseded by Mads::Runtime. This remains a working alias of + * the process-wide run flag: writing false still stops every agent in the + * process, and reading it reports the process-wide state. Prefer + * agent.runtime()->stop() to stop a single agent, or + * Mads::Runtime::stop_process() for a process-wide stop. */ -inline std::atomic running{true}; +[[deprecated("use Mads::Runtime (agent.runtime()->stop() or " + "Mads::Runtime::stop_process()) instead of the process-global " + "Mads::running flag")]] +inline std::atomic &running = Runtime::process_running(); /** * @brief Enumeration of event types for agents. diff --git a/src/main/broker.cpp b/src/main/broker.cpp index 622244b..081e4cf 100644 --- a/src/main/broker.cpp +++ b/src/main/broker.cpp @@ -214,13 +214,13 @@ void proxy(zmqpp::socket &frontend, zmqpp::socket &backend, zmqpp::proxy_steerable(frontend, backend, ctrl); } -// Install SIGINT/SIGTERM handlers that request a clean shutdown by clearing -// Mads::running. Used in daemon mode so a `kill`/`systemctl stop` (or CTRL-C) +// Install SIGINT/SIGTERM handlers that request a clean shutdown by stopping +// the process-wide run flag. Used in daemon mode so a `kill`/`systemctl stop` (or CTRL-C) // unwinds the proxy and stops advertising instead of killing the process // abruptly. Only async-signal-safe work is done here (a store to an atomic). void install_signal_handlers() { - std::signal(SIGINT, [](int) { Mads::running = false; }); - std::signal(SIGTERM, [](int) { Mads::running = false; }); + std::signal(SIGINT, [](int) { Mads::Runtime::stop_process(); }); + std::signal(SIGTERM, [](int) { Mads::Runtime::stop_process(); }); } std::string &read_settings_file(std::string const &settings_path) { @@ -543,14 +543,14 @@ int main(int argc, char **argv) { << discovery_retry_interval.count() << "s in the background" << style::reset << fg::reset << endl; discovery_retry_thread = thread([&, discovery_retry_interval]() { - while (Mads::running && !discovery_service.is_advertising()) { + while (Mads::Runtime::process_running() && !discovery_service.is_advertising()) { // Interruptible sleep so shutdown is not delayed by up to 5s. for (auto waited = std::chrono::milliseconds::zero(); - waited < discovery_retry_interval && Mads::running; + waited < discovery_retry_interval && Mads::Runtime::process_running(); waited += std::chrono::milliseconds(100)) { std::this_thread::sleep_for(std::chrono::milliseconds(100)); } - if (!Mads::running) { + if (!Mads::Runtime::process_running()) { break; } if (try_start_discovery(true)) { @@ -603,7 +603,7 @@ int main(int argc, char **argv) { << fg::reset << endl; // Graceful shutdown: run the proxy in steerable mode so that SIGINT/SIGTERM - // (clearing Mads::running, see install_signal_handlers above) can unwind it + // (stopping the process-wide run flag, see install_signal_handlers above) can unwind it // cleanly. The blocking zmqpp::proxy() cannot be interrupted by a signal, so // we drive a steerable proxy from a control socket and send TERMINATE once a // shutdown is requested. @@ -614,7 +614,7 @@ int main(int argc, char **argv) { thread proxy_thread(proxy, ref(frontend), ref(backend), ref(controlled)); - while (Mads::running) { + while (Mads::Runtime::process_running()) { this_thread::sleep_for(200ms); } diff --git a/src/main/federate.cpp b/src/main/federate.cpp index 2e3647d..a2ba7f4 100644 --- a/src/main/federate.cpp +++ b/src/main/federate.cpp @@ -127,8 +127,7 @@ int main(int argc, char *argv[]) { try { // No CURVE/watchdog on the relay's two legs: each side is a plain agent // connection, and a per-leg watchdog force-exiting the process would - // fight over the single process-wide Mads::running flag with the other - // leg (see Mads::Agent's loop()/shutdown() implementation). + // force-exit the whole relay because of a single slow leg. side_a.init(/*crypto=*/false, /*install_watchdog=*/false); side_b.init(/*crypto=*/false, /*install_watchdog=*/false); } catch (const std::exception &e) { @@ -140,8 +139,8 @@ int main(int argc, char *argv[]) { side_a.connect(); side_b.connect(); - signal(SIGINT, [](int) { Mads::running = false; }); - signal(SIGTERM, [](int) { Mads::running = false; }); + signal(SIGINT, [](int) { Mads::Runtime::stop_process(); }); + signal(SIGTERM, [](int) { Mads::Runtime::stop_process(); }); cerr << style::bold << "mads-federate" << style::reset << " relaying between two networks:" << endl; @@ -151,7 +150,7 @@ int main(int argc, char *argv[]) { << endl; cerr << fg::green << "Federation started" << fg::reset << endl; - while (Mads::running) { + while (Mads::Runtime::process_running()) { bool busy = false; if (relay_one(side_a, side_b, relay_id)) { count_a_to_b++; diff --git a/src/main/logger.cpp b/src/main/logger.cpp index 69c055c..f3a53df 100644 --- a/src/main/logger.cpp +++ b/src/main/logger.cpp @@ -118,7 +118,7 @@ int main(int argc, char *argv[]) { // Create and start the thread std::thread logger_status_thread([&logger] { json j; - while (Mads::running) { + while (logger.runtime()->running()) { j["logger_paused"] = logger.paused;; logger.publish(j, LOGGER_STATUS_TOPIC); std::this_thread::sleep_for(std::chrono::seconds(1)); @@ -186,7 +186,7 @@ int main(int argc, char *argv[]) { cout << fg::green << "Logger process stopped" << fg::reset << endl; // Cleanup - // Mads::running = false; + logger_status_thread.join(); logger.register_event(Mads::event_type::shutdown); logger.disconnect(); // Not necessary, called by destructor diff --git a/src/main/plugin_loader.cpp b/src/main/plugin_loader.cpp index 6dc4ebd..a5a5ec7 100644 --- a/src/main/plugin_loader.cpp +++ b/src/main/plugin_loader.cpp @@ -401,7 +401,7 @@ int main(int argc, char *argv[]) { // cerr << fg::red << "Critical error getting data: " << plugin->error() // << fg::reset << endl; count_err++; - Mads::running = false; + agent.runtime()->stop(); throw std::runtime_error(string("Critical error in getting data: ") + plugin->error()); return 0ms; } @@ -484,7 +484,7 @@ int main(int argc, char *argv[]) { << fg::reset << endl; err["error"]["load_data"] = plugin->error(); agent.register_event(event_type::message, err); - Mads::running = false; + agent.runtime()->stop(); return 0ms; } // processing data in the plugin @@ -513,7 +513,7 @@ int main(int argc, char *argv[]) { << fg::reset << endl; err["error"]["process"] = plugin->error(); agent.register_event(event_type::message, err); - Mads::running = false; + agent.runtime()->stop(); return 0ms; } // publishing data @@ -595,7 +595,7 @@ int main(int argc, char *argv[]) { json e_msg = {{"error", {"load_data", plugin->error()}}}; agent.register_event(event_type::message, e_msg); count_err++; - Mads::running = false; + agent.runtime()->stop(); return 0ms; } diff --git a/src/main/rust_plugin_loader.cpp b/src/main/rust_plugin_loader.cpp index 8306c59..84b8d08 100644 --- a/src/main/rust_plugin_loader.cpp +++ b/src/main/rust_plugin_loader.cpp @@ -394,7 +394,7 @@ int main(int argc, char *argv[]) { break; case return_type::critical: count_err++; - Mads::running = false; + agent.runtime()->stop(); throw runtime_error("Critical error in get_output: " + plugin.error()); } if (!silent) @@ -448,7 +448,7 @@ int main(int argc, char *argv[]) { case return_type::critical: err["error"]["load_data"] = plugin.error(); agent.register_event(event_type::message, err); - Mads::running = false; + agent.runtime()->stop(); return 0ms; } process_output: @@ -472,7 +472,7 @@ int main(int argc, char *argv[]) { case return_type::critical: err["error"]["process"] = plugin.error(); agent.register_event(event_type::message, err); - Mads::running = false; + agent.runtime()->stop(); return 0ms; } if (!blob.empty()) { @@ -533,7 +533,7 @@ int main(int argc, char *argv[]) { case return_type::critical: err = {{"error", {{"load_data", plugin.error()}}}}; agent.register_event(event_type::message, err); - Mads::running = false; + agent.runtime()->stop(); return 0ms; } if (!silent) diff --git a/src/main/worker.cpp b/src/main/worker.cpp index e522bfd..2274afb 100644 --- a/src/main/worker.cpp +++ b/src/main/worker.cpp @@ -170,7 +170,7 @@ int main(int argc, char *argv[]) { agent.loop([&]() -> chrono::milliseconds { json payload = agent.pull(); return_type rt; - if (payload.empty() && !Mads::running) return 0ms; + if (payload.empty() && !agent.runtime()->running()) return 0ms; // TODO: verify if we need to check return type // message_type type = agent.receive(); agent.receive(); diff --git a/src/service_discovery.cpp b/src/service_discovery.cpp index 681bc17..46146c2 100644 --- a/src/service_discovery.cpp +++ b/src/service_discovery.cpp @@ -956,7 +956,7 @@ void ServiceDiscovery::advertising_loop() { std::chrono::milliseconds interval{0}; { std::scoped_lock lock(_mutex); - if (_stop_requested || !Mads::running) { + if (_stop_requested || !Mads::Runtime::process_running()) { _advertising = false; break; } @@ -973,8 +973,8 @@ void ServiceDiscovery::advertising_loop() { std::unique_lock lock(_mutex); _cv.wait_for(lock, interval, - [this]() { return _stop_requested || !Mads::running; }); - if (_stop_requested || !Mads::running) { + [this]() { return _stop_requested || !Mads::Runtime::process_running(); }); + if (_stop_requested || !Mads::Runtime::process_running()) { _advertising = false; break; } diff --git a/tests/mads_test_helpers.hpp b/tests/mads_test_helpers.hpp index d29d5cf..b55f0a9 100644 --- a/tests/mads_test_helpers.hpp +++ b/tests/mads_test_helpers.hpp @@ -33,10 +33,13 @@ inline std::string loopback(uint16_t port) { } // Restores the global run flag on scope exit so a test that stops the loop -// cannot poison later tests in the same binary. +// cannot poison later tests in the same binary. Since the Runtime refactor +// only process-level stops (signal handlers, remote_control shutdown/restart, +// Runtime::stop_process()) touch shared state; this guard resets that flag. +// Agent shutdown()/disconnect() are per-agent and need no guard. struct RunningGuard { - RunningGuard() { Mads::running = true; } - ~RunningGuard() { Mads::running = true; } + RunningGuard() { Mads::Runtime::process_running() = true; } + ~RunningGuard() { Mads::Runtime::process_running() = true; } }; // Polls a predicate until it returns true or the timeout expires. Use this diff --git a/tests/test_agent_loop.cpp b/tests/test_agent_loop.cpp index cceaf1f..780a45c 100644 --- a/tests/test_agent_loop.cpp +++ b/tests/test_agent_loop.cpp @@ -1,13 +1,15 @@ // Unit tests for Mads::Agent's main loop machinery and remote_control() -// (src/agent.cpp ~995-1120). Every TEST_CASE that touches Agent::loop() or -// remote_control()'s effect on Mads::running instantiates -// mads_test::RunningGuard, since Mads::running is a process-global atomic -// (src/mads.hpp.in:193). +// (src/agent.cpp ~995-1120). Each agent owns its own Mads::Runtime, so loop +// tests stop agents through agent.runtime()->stop() without touching shared +// state. remote_control("restart"/"shutdown") is process-level by design +// (it stops Mads::Runtime::process_running()), so those TEST_CASEs +// instantiate mads_test::RunningGuard to restore the process flag. // // The watchdog *firing* path (install_loop_watchdog()'s force-exit branch) // is intentionally not exercised: it calls std::_Exit() and would kill the // test binary. install_loop_watchdog() itself is exercised once, in a way -// that never lets the countdown start (Mads::running stays true throughout). +// that never lets the countdown start (the agent is never asked to stop +// while looping). #include #include @@ -25,10 +27,9 @@ using namespace std::chrono_literals; // loop() // --------------------------------------------------------------------------- -TEST_CASE("loop() invokes the lambda repeatedly until Mads::running is " - "cleared", +TEST_CASE("loop() invokes the lambda repeatedly until the agent's Runtime " + "is stopped", "[agent_loop]") { - mads_test::RunningGuard guard; Mads::Agent a("loopA", "none"); a.init(false, false); @@ -38,7 +39,7 @@ TEST_CASE("loop() invokes the lambda repeatedly until Mads::running is " [&]() -> std::chrono::nanoseconds { ++count; if (count >= target) - Mads::running = false; + a.runtime()->stop(); return std::chrono::nanoseconds(0); }, std::chrono::nanoseconds(0)); @@ -48,7 +49,6 @@ TEST_CASE("loop() invokes the lambda repeatedly until Mads::running is " TEST_CASE("loop() paces iterations by the requested duration", "[agent_loop]") { - mads_test::RunningGuard guard; Mads::Agent a("loopB", "none"); a.init(false, false); @@ -60,7 +60,7 @@ TEST_CASE("loop() paces iterations by the requested duration", [&]() -> std::chrono::nanoseconds { ++count; if (count >= target) - Mads::running = false; + a.runtime()->stop(); return std::chrono::nanoseconds(0); // keep using the fixed `period` }, period); @@ -77,7 +77,6 @@ TEST_CASE("loop() paces iterations by the requested duration", TEST_CASE("loop()'s lambda-returned duration overrides the fixed duration " "on subsequent iterations", "[agent_loop]") { - mads_test::RunningGuard guard; Mads::Agent a("loopE", "none"); a.init(false, false); @@ -92,7 +91,7 @@ TEST_CASE("loop()'s lambda-returned duration overrides the fixed duration " [&]() -> std::chrono::nanoseconds { ++count; if (count >= 5) - Mads::running = false; + a.runtime()->stop(); return std::chrono::microseconds(500); }, std::chrono::seconds(1)); @@ -104,9 +103,8 @@ TEST_CASE("loop()'s lambda-returned duration overrides the fixed duration " } TEST_CASE("loop() catches an exception thrown by the lambda and stops " - "Mads::running", + "only that agent's loop", "[agent_loop]") { - mads_test::RunningGuard guard; Mads::Agent a("loopF", "none"); a.init(false, false); @@ -119,7 +117,10 @@ TEST_CASE("loop() catches an exception thrown by the lambda and stops " std::chrono::nanoseconds(0)); REQUIRE(count == 1); // the loop exits after the first, throwing iteration - REQUIRE_FALSE(Mads::running.load()); + // The stop is agent-local: neither the agent's Runtime nor the process-wide + // flag is touched, so other agents in the process keep running. + REQUIRE(a.runtime()->running()); + REQUIRE(Mads::Runtime::process_running().load()); } // --------------------------------------------------------------------------- @@ -127,7 +128,6 @@ TEST_CASE("loop() catches an exception thrown by the lambda and stops " // --------------------------------------------------------------------------- TEST_CASE("enable_high_res_loop toggles high_res_loop()", "[agent_loop]") { - mads_test::RunningGuard guard; Mads::Agent a("loopC", "none"); a.init(false, false); @@ -147,9 +147,9 @@ TEST_CASE("install_loop_watchdog() starts and joins cleanly on an orderly " "[agent_loop]") { mads_test::RunningGuard guard; Mads::Agent a("loopD", "none"); - // Default install_watchdog=true exercises install_loop_watchdog(). - // Mads::running stays true for the whole test (RunningGuard), so the - // watchdog's force-exit countdown never starts. + // Default install_watchdog=true exercises install_loop_watchdog(). The + // agent is never asked to stop while looping, so the watchdog's force-exit + // countdown never starts before shutdown() joins it. a.init(); a.shutdown(); // stops and joins the watchdog thread promptly SUCCEED("watchdog thread installed and joined without tripping"); @@ -159,8 +159,8 @@ TEST_CASE("install_loop_watchdog() starts and joins cleanly on an orderly " // remote_control() // --------------------------------------------------------------------------- -TEST_CASE("remote_control(\"restart\") sets restart() and clears " - "Mads::running", +TEST_CASE("remote_control(\"restart\") sets restart() and requests a " + "process-wide stop", "[agent_loop]") { mads_test::RunningGuard guard; Mads::Agent a("rcA", "none"); @@ -169,11 +169,11 @@ TEST_CASE("remote_control(\"restart\") sets restart() and clears " REQUIRE_FALSE(a.restart()); a.remote_control(R"({"cmd":"restart"})"); REQUIRE(a.restart()); - REQUIRE_FALSE(Mads::running.load()); + REQUIRE_FALSE(Mads::Runtime::process_running().load()); } -TEST_CASE("remote_control(\"shutdown\") clears Mads::running without " - "setting restart()", +TEST_CASE("remote_control(\"shutdown\") requests a process-wide stop " + "without setting restart()", "[agent_loop]") { mads_test::RunningGuard guard; Mads::Agent a("rcB", "none"); @@ -181,18 +181,18 @@ TEST_CASE("remote_control(\"shutdown\") clears Mads::running without " a.remote_control(R"({"cmd":"shutdown"})"); REQUIRE_FALSE(a.restart()); - REQUIRE_FALSE(Mads::running.load()); + REQUIRE_FALSE(Mads::Runtime::process_running().load()); } TEST_CASE("remote_control() with malformed JSON is a no-op that leaves " - "Mads::running set", + "the process-wide run flag set", "[agent_loop]") { mads_test::RunningGuard guard; Mads::Agent a("rcC", "none"); a.init(false, false); a.remote_control("not json at all"); - REQUIRE(Mads::running.load()); + REQUIRE(Mads::Runtime::process_running().load()); REQUIRE_FALSE(a.restart()); } @@ -203,7 +203,7 @@ TEST_CASE("remote_control() with an unrecognized cmd is a no-op", a.init(false, false); a.remote_control(R"({"cmd":"frobnicate"})"); - REQUIRE(Mads::running.load()); + REQUIRE(Mads::Runtime::process_running().load()); REQUIRE_FALSE(a.restart()); } @@ -248,5 +248,6 @@ TEST_CASE("remote_control(\"info\") publishes the agent's settings on the " auto [topic, doc] = sub->last_json(); REQUIRE(topic == "info"); REQUIRE(doc.at("agent") == "rcD"); - REQUIRE(Mads::running.load()); // "info" must not touch the running flag + // "info" must not touch the run state + REQUIRE(Mads::Runtime::process_running().load()); } diff --git a/tests/test_runtime.cpp b/tests/test_runtime.cpp new file mode 100644 index 0000000..d03406e --- /dev/null +++ b/tests/test_runtime.cpp @@ -0,0 +1,170 @@ +// Unit tests for Mads::Runtime and the per-agent run-state semantics +// introduced by it. The key regressions guarded here are the historical +// process-global Mads::running hazards: destroying one agent used to stop +// every other agent's loop (and LKV drain thread) in the same process, and a +// disconnected agent could never loop again. +#include + +#include +#include +#include +#include + +#include "agent.hpp" +#include "mads_test_helpers.hpp" + +using namespace std::chrono_literals; + +// --------------------------------------------------------------------------- +// Runtime semantics +// --------------------------------------------------------------------------- + +TEST_CASE("Runtime starts running and stops/resets independently", + "[runtime]") { + Mads::Runtime rt; + REQUIRE(rt.running()); + rt.stop(); + REQUIRE_FALSE(rt.running()); + rt.reset(); + REQUIRE(rt.running()); +} + +TEST_CASE("stop_process() stops every Runtime and reset() cannot override it", + "[runtime]") { + mads_test::RunningGuard guard; // restores the process flag afterwards + Mads::Runtime rt1, rt2; + REQUIRE(rt1.running()); + REQUIRE(rt2.running()); + + Mads::Runtime::stop_process(); + REQUIRE_FALSE(rt1.running()); + REQUIRE_FALSE(rt2.running()); + + rt1.reset(); // a group-level reset must not resurrect a stopping process + REQUIRE_FALSE(rt1.running()); +} + +// --------------------------------------------------------------------------- +// Agent/Runtime wiring +// --------------------------------------------------------------------------- + +TEST_CASE("each agent owns its own Runtime by default", "[runtime]") { + Mads::Agent a("rtA", "none"), b("rtB", "none"); + REQUIRE(a.runtime() != nullptr); + REQUIRE(b.runtime() != nullptr); + REQUIRE(a.runtime() != b.runtime()); + a.runtime()->stop(); + REQUIRE_FALSE(a.runtime()->running()); + REQUIRE(b.runtime()->running()); +} + +TEST_CASE("set_runtime() rejects null and connected agents", "[runtime]") { + Mads::Agent a("rtC", "none"); + a.init(false, false); + REQUIRE_THROWS_AS(a.set_runtime(nullptr), Mads::AgentError); + + a.set_pub_topic("t"); + a.connect(0ms); + REQUIRE_THROWS_AS(a.set_runtime(std::make_shared()), + Mads::AgentError); + a.disconnect(); +} + +TEST_CASE("agents sharing a Runtime are stopped together", "[runtime]") { + Mads::Agent a("rtD", "none"), b("rtE", "none"); + b.set_runtime(a.runtime()); + REQUIRE(a.runtime() == b.runtime()); + a.runtime()->stop(); + REQUIRE_FALSE(b.runtime()->running()); +} + +// --------------------------------------------------------------------------- +// Regressions for the historical global-flag hazards +// --------------------------------------------------------------------------- + +TEST_CASE("destroying one agent does not stop another agent's loop", + "[runtime]") { + auto victim = std::make_unique("rtF", "none"); + victim->init(false, false); + + Mads::Agent survivor("rtG", "none"); + survivor.init(false, false); + + std::atomic count{0}; + std::thread loop_thread([&] { + survivor.loop( + [&]() -> std::chrono::nanoseconds { + ++count; + return std::chrono::nanoseconds(0); + }, + 1ms); + }); + + REQUIRE(mads_test::wait_for([&] { return count.load() >= 3; })); + + // Historically this shutdown flipped the process-global run flag and the + // survivor's loop exited on its next iteration. + victim->shutdown(); + victim.reset(); + + int at_destruction = count.load(); + bool still_looping = mads_test::wait_for( + [&] { return count.load() >= at_destruction + 5; }); + + survivor.runtime()->stop(); + loop_thread.join(); + REQUIRE(still_looping); +} + +TEST_CASE("an agent can loop again after disconnect() and connect()", + "[runtime]") { + Mads::Agent a("rtH", "none"); + a.init(false, false); + a.set_pub_topic("t"); + a.connect(0ms); + a.disconnect(); // raises the per-agent stop request... + a.connect(0ms); // ...and reconnecting clears it + + int count = 0; + a.loop( + [&]() -> std::chrono::nanoseconds { + if (++count >= 3) + a.runtime()->stop(); + return std::chrono::nanoseconds(0); + }, + std::chrono::nanoseconds(0)); + REQUIRE(count == 3); + a.disconnect(); +} + +// --------------------------------------------------------------------------- +// Deprecated Mads::running shim +// --------------------------------------------------------------------------- + +// The shim must keep legacy code working: it aliases the process-wide flag, +// so writing false stops every agent and reading it reports process state. +#if defined(__GNUC__) || defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#elif defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4996) +#endif +TEST_CASE("deprecated Mads::running still aliases the process-wide flag", + "[runtime]") { + mads_test::RunningGuard guard; + Mads::Runtime rt; + + REQUIRE(Mads::running.load()); + Mads::running = false; // legacy process-wide stop + REQUIRE_FALSE(rt.running()); + REQUIRE_FALSE(Mads::Runtime::process_running().load()); + + Mads::running = true; + REQUIRE(rt.running()); +} +#if defined(__GNUC__) || defined(__clang__) +#pragma GCC diagnostic pop +#elif defined(_MSC_VER) +#pragma warning(pop) +#endif diff --git a/tools/service_discovery_demo.cpp b/tools/service_discovery_demo.cpp index ef3d8a5..9d084a3 100644 --- a/tools/service_discovery_demo.cpp +++ b/tools/service_discovery_demo.cpp @@ -19,7 +19,7 @@ namespace { void stop_demo(int) { // keep_running = false; - Mads::running = false; + Mads::Runtime::stop_process(); } pair parse_service_argument(const string &value) { @@ -119,7 +119,7 @@ int main(int argc, char **argv) { } cout << "Press Ctrl-C to stop." << endl; - while (Mads::running) { + while (Mads::Runtime::process_running()) { this_thread::sleep_for(200ms); } From ea238e739544afc7443a355960c0ea46b48b6203 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 21:01:58 +0000 Subject: [PATCH 13/14] docs: add 'Run state and agent lifecycle' page to the Doxygen docs New RUNTIME.md topical page (wired into doc/Doxyfile INPUT, alongside CRYPTO.md) documenting the Mads::Runtime model: the three stop levels (agent, Runtime group, process), usage examples, and a migration table from the deprecated Mads::running flag. The Runtime class doc and the deprecated flag doc now link to it via \ref. --- RUNTIME.md | 99 +++++++++++++++++++++++++++++++++++++++++++++++++ doc/Doxyfile | 1 + src/mads.hpp.in | 7 ++++ 3 files changed, 107 insertions(+) create mode 100644 RUNTIME.md diff --git a/RUNTIME.md b/RUNTIME.md new file mode 100644 index 0000000..08a8e32 --- /dev/null +++ b/RUNTIME.md @@ -0,0 +1,99 @@ +# Run state and agent lifecycle + +## Context + +Since MADS v2.5, the "keep going" state that drives `Mads::Agent::loop()` is +owned by a dedicated object, `Mads::Runtime`, instead of a single +process-global flag. This page explains the model, how to control agent +lifecycles with it, and how the deprecated `Mads::running` flag maps onto it. + +Historically, all loops in a process watched one global `Mads::running` +atomic, and `Agent::shutdown()`/`Agent::disconnect()` cleared it. That made +instance-level teardown mutate process-level state, with surprising effects +when a process hosted more than one agent: destroying one agent silently +ended every other agent's `loop()`, froze their last-known-value drain +threads on stale data, could arm another agent's watchdog into a forced +process exit, and a disconnected agent could never loop again. + +## The model + +There are three levels of run state, from narrowest to widest: + +1. **Agent** — `Agent::shutdown()` and `Agent::disconnect()` raise a stop + request that affects *only that agent*. `Agent::connect()` clears it, so + an agent can be disconnected, reconfigured, reconnected, and looped again. +2. **Runtime (group)** — every agent references a `Mads::Runtime`; by default + each agent owns its own. Calling `runtime->stop()` ends the loops of every + agent attached to that Runtime. Attach several agents to one Runtime with + `Agent::set_runtime()` (before connecting) to stop them as a group. +3. **Process** — `Mads::Runtime::stop_process()` makes *every* Runtime's + `running()` report false. This is what the SIGINT/SIGTERM handlers + installed by `Agent::loop()` use, and what the remote-control `shutdown` + and `restart` commands request. A process-wide stop cannot be undone by + `Runtime::reset()`. + +`Agent::loop()`, the last-known-value drain thread, and the threaded remote +control all keep going only while the agent's Runtime is running *and* no +per-agent stop was requested. + +## Typical usage + +A single-agent program needs no explicit Runtime management at all: + +```cpp +Mads::Agent agent("my_agent", settings_uri); +agent.init(); +agent.connect(); +agent.loop([&]() { + // ... publish/receive ... + return std::chrono::milliseconds(100); +}); +agent.shutdown(); // stops this agent only +``` + +To stop a loop from inside the lambda, stop the agent's own Runtime: + +```cpp +agent.loop([&]() -> std::chrono::nanoseconds { + if (done()) agent.runtime()->stop(); + return std::chrono::nanoseconds(0); +}); +``` + +Two independent agents in one process no longer interfere: each has its own +Runtime, so destroying or disconnecting one leaves the other's loop, drain +thread, and watchdog untouched. If they *should* stop together, group them: + +```cpp +Mads::Agent source("source", uri), sink("sink", uri); +sink.set_runtime(source.runtime()); // one shared group +// ... +source.runtime()->stop(); // stops both +``` + +For an orderly full-process shutdown from your own code (equivalent to +Ctrl-C): + +```cpp +Mads::Runtime::stop_process(); +``` + +## Migrating from `Mads::running` + +`Mads::running` still exists as a **deprecated alias of the process-wide run +flag**, so existing code compiles and behaves as before — writing `false` +stops every agent in the process, and reading it reports the process-wide +state. Using it raises a compile-time deprecation warning. Replace it as +follows: + +| Legacy | Replacement | +|---|---| +| `Mads::running = false;` (stop everything) | `Mads::Runtime::stop_process();` | +| `Mads::running = false;` (stop one agent's loop) | `agent.runtime()->stop();` | +| `while (Mads::running) { ... }` (app main loop) | `while (Mads::Runtime::process_running()) { ... }` | +| `while (Mads::running) { ... }` (per-agent thread) | `while (agent.runtime()->running()) { ... }` | + +One behavioral difference is intentional: agent teardown no longer clears the +process-wide flag. Code that relied on "destroying an agent ends my own +`while (Mads::running)` loop" must now stop the process (or a shared +Runtime) explicitly. diff --git a/doc/Doxyfile b/doc/Doxyfile index a324a75..91ecc2d 100644 --- a/doc/Doxyfile +++ b/doc/Doxyfile @@ -31,6 +31,7 @@ INPUT = README.md \ CHANGES.md \ COMPILE.md \ CRYPTO.md \ + RUNTIME.md \ src \ rust/README.md RECURSIVE = YES diff --git a/src/mads.hpp.in b/src/mads.hpp.in index 1e6eaa6..468e08f 100644 --- a/src/mads.hpp.in +++ b/src/mads.hpp.in @@ -203,6 +203,10 @@ inline bool check_version(const std::string &version) { * "shutdown"/"restart" commands) stops every Runtime in the process. * * running() reports true only if neither level has been stopped. + * + * @see \ref md_RUNTIME "Run state and agent lifecycle" (RUNTIME.md) for the + * full model, usage patterns, and the migration table from the deprecated + * Mads::running flag. */ class Runtime { public: @@ -235,6 +239,9 @@ private: * process, and reading it reports the process-wide state. Prefer * agent.runtime()->stop() to stop a single agent, or * Mads::Runtime::stop_process() for a process-wide stop. + * + * @see \ref md_RUNTIME "Run state and agent lifecycle" (RUNTIME.md) for the + * migration table. */ [[deprecated("use Mads::Runtime (agent.runtime()->stop() or " "Mads::Runtime::stop_process()) instead of the process-global " From f58a45c245b918def49a40c30f76eb482a698f18 Mon Sep 17 00:00:00 2001 From: Paolo Bosetti Date: Sat, 11 Jul 2026 21:05:05 +0000 Subject: [PATCH 14/14] docs: file this work under the unreleased v2.4.0, not v2.5 v2.4.0 hasn't shipped yet, so the test suite/coverage infrastructure and the Mads::Runtime refactor belong in its CHANGES.md entry rather than a hypothetical v2.5. Also fixes a stray 'v2.5' reference in RUNTIME.md. --- CHANGES.md | 7 +++++++ RUNTIME.md | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 82a8fb6..31b56b6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,8 @@ These changes summarize what was added or updated since `v2.3.1`. - **High-resolution loop pacing.** `Agent::loop()` now paces its internal timing in nanoseconds instead of milliseconds (existing millisecond-based code keeps compiling and behaving as before). Agents can set a `time_step_us` key in their settings section (wins over `time_step`) for microsecond-granularity periods, and opt into `enable_high_res_loop()` (or the `high_res_loop`/`spin_margin_us` settings keys) to busy-spin the tail of each interval instead of sleeping through it, trading CPU time for microsecond-accurate wake-up timing. - **`mads plugin --update` migration command.** C++ plugins can now be migrated in place to the current plugin protocol version instead of being rewritten by hand. It detects the plugin's current protocol from its `CMakeLists.txt`, chains the migration steps recorded in `share/plugin_migrations` up to the current protocol, bumps the pinned `GIT_TAG`, and rewrites affected method signatures (located structurally, so reformatted/multi-line declarations migrate correctly). Each modified file is saved as `*.bak`, and a change report plus manual follow-up checklist is printed; unless `--no-check`, the migrated plugin is then compiled so the compiler flags anything the rewrite could not handle. `--dry-run`, `--from`, and `--to` are also available. Rust plugins are unaffected (they track the `mads-plugin` crate version in `Cargo.toml`). See [share/man/mads-plugin.md](share/man/mads-plugin.md). - **`plugin_deps.json` dependency manifest.** Scaffolded C++ plugins now pin their dependency versions (plugin protocol, `mads_plugin`/`pugg` git tags, bundled `nlohmann::json` version) from a single `share/plugin_deps.json` file, which also supplies the default target protocol for `mads plugin --update`, instead of being hard-coded into the CMake template. +- **`Mads::Runtime`.** Agent run state (the "keep going" flag consulted by `Agent::loop()`, the last-known-value drain thread, and the threaded remote control) is now owned by a `Mads::Runtime` object instead of a single process-global flag. Each `Agent` gets its own `Runtime` by default, so stopping or destroying one agent no longer stops the loops of other agents hosted in the same process, and a disconnected agent can reconnect and loop again. Agents that should stop together can share a `Runtime` via `Agent::set_runtime()`; `Mads::Runtime::stop_process()` (used by the SIGINT/SIGTERM handlers and the remote `shutdown`/`restart` commands) still stops every agent in the process. The previous `Mads::running` flag remains available as a `[[deprecated]]` alias of the process-wide state, so existing code keeps compiling and behaving as before, with a compile-time warning pointing at the replacement API. See [RUNTIME.md](RUNTIME.md). +- **Unit test suite and code coverage.** MADS gained an in-tree Catch2-based unit test suite (`tests/`, built with `-DMADS_BUILD_TESTS=ON`) covering the core library — settings loading, the broker request protocol, pub/sub and the wire codec over loopback ZeroMQ, the C ABI, CURVE key handling, and the plugin migration engine — with no external broker, database, or network access required. A `-DMADS_COVERAGE=ON` CMake option instruments the build for `gcovr`/lcov-style reports, and a new GitHub Actions workflow (`.github/workflows/coverage.yml`) runs the suite and uploads results to [codecov.io](https://codecov.io/gh/pbosetti/MADS) on every push. ## Improvements @@ -21,6 +23,11 @@ These changes summarize what was added or updated since `v2.3.1`. - **More robust `GIT_TAG` rewriting during plugin migration.** The CMake `GIT_TAG` bump used by `mads plugin --update` no longer relies on a single fragile regex spanning the whole `FetchContent` block (which could match a tag mentioned in a comment, or mis-substitute a replacement value starting with a digit). It now locates the block header and then tokenizes it with a small CMake-aware scanner that honours `#` comments, quoted arguments, and nested parentheses, replacing exactly the `GIT_TAG` value token by position. - **gv2fsm updated to v2.0.0.** The bundled finite-state-machine generator is updated to its latest release. +## Bug fixes + +- **`Agent::query_broker()` could hang forever on shutdown.** The REQ socket used to fetch settings from a broker had no linger timeout; if the broker was unreachable, a queued but undelivered request kept the ZeroMQ context alive and `Agent::shutdown()` blocked indefinitely in context termination. The socket now sets `linger = 0`. +- **C API CURVE key setters could crash instead of returning an error.** `agent_set_client_public_key`/`agent_set_client_secret_key`/`agent_set_server_public_key` guarded against being called before `agent_setup_crypto()` by checking the address of the internal `CurveAuth` pointer, which is never null; calling any of them too early dereferenced a null `CurveAuth` and crashed the process instead of returning `-1` as documented. + --- # Release 2.3.1 diff --git a/RUNTIME.md b/RUNTIME.md index 08a8e32..538c298 100644 --- a/RUNTIME.md +++ b/RUNTIME.md @@ -2,7 +2,7 @@ ## Context -Since MADS v2.5, the "keep going" state that drives `Mads::Agent::loop()` is +Since MADS v2.4.0, the "keep going" state that drives `Mads::Agent::loop()` is owned by a dedicated object, `Mads::Runtime`, instead of a single process-global flag. This page explains the model, how to control agent lifecycles with it, and how the deprecated `Mads::running` flag maps onto it.