[#53] Review and Optimize

.github/workflows/ci.yml

@@ -36,3 +36,7 @@ jobs:
       - name: Build Test
         run: |
           make build-test
+      - name: Upload to Codecov
+        uses: codecov/codecov-action@v5
+        with:
+          verbose: true

CMakeLists.txt

@@ -2,6 +2,8 @@
 cmake_minimum_required(VERSION 3.25)
 project(Simulith VERSION 0.0 LANGUAGES C)
 
+option(BUILD_SIMULITH_TESTS "Build Simulith tests" OFF)
+
 # Enable compile_commands.json for IDEs and static analysis
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 
@@ -19,15 +21,31 @@ if(EXISTS "${COMP_FLAGS_FILE}")
     message(STATUS "Including component flags from ${COMP_FLAGS_FILE}")
     include("${COMP_FLAGS_FILE}")
 elseif(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
+    # Basic fallback flags when component flags aren't available
     add_compile_options(-Wall -Werror)
+    # If building tests, ensure we enable diagnostics and coverage flags
+    if(BUILD_SIMULITH_TESTS)
+        message(STATUS "CompFlags.cmake not found — adding test/coverage flags for GNU/Clang")
+        add_compile_options(-fdiagnostics-show-option -fprofile-arcs -ftest-coverage)
+        # Ensure linker emits coverage data too
+        if(POLICY CMP0068)
+            # add_link_options is available; attach --coverage for linking
+            add_link_options(--coverage)
+        else()
+            set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} --coverage")
+            set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} --coverage")
+        endif()
+    endif()
 endif()
 
-# Create Unity library with specific compiler flags and always build with -fPIC
-add_library(unity STATIC unity/unity.c)
-set_target_properties(unity PROPERTIES POSITION_INDEPENDENT_CODE ON)
-target_include_directories(unity PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/unity)
-if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
-    target_compile_options(unity PRIVATE -Wno-float-equal)
+if(BUILD_SIMULITH_TESTS)
+    # Create Unity library with specific compiler flags and always build with -fPIC
+    add_library(unity STATIC ./test/unity/unity.c)
+    set_target_properties(unity PROPERTIES POSITION_INDEPENDENT_CODE ON)
+    target_include_directories(unity PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/unity)
+    if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
+        target_compile_options(unity PRIVATE -Wno-float-equal)
+    endif()
 endif()
 
 # Simulith library sources
@@ -52,6 +70,11 @@ target_include_directories(simulith PUBLIC
 )
 target_link_libraries(simulith PUBLIC ${ZeroMQ_LIBRARIES})
 
+if(BUILD_SIMULITH_TESTS)
+    # When building tests, expose internal symbols for test-only helpers
+    target_compile_definitions(simulith PRIVATE SIMULITH_TESTING)
+endif()
+
 # Build 42 as a library
 set(FORTYTWO_SOURCES
     42/Source/42exec.c
@@ -162,9 +185,7 @@ set_target_properties(simulith_server_standalone PROPERTIES
     INSTALL_RPATH "$ORIGIN"
 )
 
-
 # Optionally add tests subdirectory
-option(BUILD_SIMULITH_TESTS "Build Simulith tests" OFF)
 if(BUILD_SIMULITH_TESTS)
     enable_testing()
     add_subdirectory(test)

Makefile

@@ -27,40 +27,57 @@ build-server:
 	mkdir -p $(BUILDDIR)
 	cd $(BUILDDIR) && cmake ..
 	$(MAKE) --no-print-directory -C $(BUILDDIR) simulith_server_standalone
-	# Create components directory and copy shared libraries
-	mkdir -p $(BUILDDIR)/components
-	@for dir in $(TOPDIR)/comp/*/sim/build ; do \
-		if [ -d "$$dir" ] && [ -f "$$dir/Makefile" ]; then \
-			cp $$dir/*.so $(BUILDDIR)/components/ 2>/dev/null || true; \
-		fi; \
-	done
-	# Copy 42 configuration to build directory for runtime
-	cp -r 42/InOut $(BUILDDIR)/ 2>/dev/null || true
-	cp -r 42/Model $(BUILDDIR)/ 2>/dev/null || true
 
 build-sim:
 	$(MAKE) build-director
 	$(MAKE) build-server
 
 build-test:
 	mkdir -p $(BUILDDIR)
-	cd $(BUILDDIR) && cmake .. -DBUILD_SIMULITH_TESTS=ON
+	cd $(BUILDDIR) && cmake .. -DBUILD_SIMULITH_TESTS=ON -DENABLE_UNIT_TESTS=true
 	$(MAKE) --no-print-directory -C $(BUILDDIR)
 	cd $(BUILDDIR) && ctest --output-on-failure -O ctest.log
+	lcov -c --directory . --output-file $(BUILDDIR)/coverage.info
+	lcov --remove $(BUILDDIR)/coverage.info '*/test/*' '*/tests/*' --output-file $(BUILDDIR)/coverage.filtered.info
+	genhtml $(BUILDDIR)/coverage.filtered.info --output-directory $(BUILDDIR)/report
+	@echo ""
+	@echo "Review coverage report: "
+	@echo "  firefox $(BUILDDIR)/report/index.html"
+	@echo ""
+
+copy-sims:
+	@if [ ! -d "$(BUILDDIR)" ]; then \
+		echo "Error: Build directory $(BUILDDIR) does not exist. Please run 'make build' first."; \
+		exit 1; \
+	fi
+	mkdir -p $(BUILDDIR)/components
+	@for dir in $(TOPDIR)/comp/*/sim/build ; do \
+		if [ -d "$$dir" ] && [ -f "$$dir/Makefile" ]; then \
+			cp $$dir/*.so $(BUILDDIR)/components/ 2>/dev/null || true; \
+		fi; \
+	done
+	cp -r 42/InOut $(BUILDDIR)/ 2>/dev/null || true
+	cp -r 42/Model $(BUILDDIR)/ 2>/dev/null || true
 
 clean:
 	rm -rf $(BUILDDIR)
 
 debug:
 	docker run --rm -it -v $(TOPDIR):$(TOPDIR) --user $(shell id -u):$(shell id -g) --name $(CONTAINER_NAME) -w $(CURDIR) $(BUILD_IMAGE) /bin/bash
 
-director:
-	$(MAKE) clean build
-	docker build -t $(RUNTIME_DIRECTOR_NAME) -f test/Dockerfile.director .
+director: copy-sims
+	@if [ ! -d "$(BUILDDIR)" ]; then \
+		echo "Error: Build directory $(BUILDDIR) does not exist. Please run 'make build' first."; \
+		exit 1; \
+	fi
+	docker build -t $(RUNTIME_DIRECTOR_NAME) -f docker/Dockerfile.director .
 
 server:
-	$(MAKE) clean build
-	docker build -t $(RUNTIME_SERVER_NAME) -f test/Dockerfile.server .
+	@if [ ! -d "$(BUILDDIR)" ]; then \
+		echo "Error: Build directory $(BUILDDIR) does not exist. Please run 'make build' first."; \
+		exit 1; \
+	fi
+	docker build -t $(RUNTIME_SERVER_NAME) -f docker/Dockerfile.server .
 
 stop:
 	docker ps --filter name=tryspace-* | xargs docker stop

include/simulith.h

@@ -41,6 +41,12 @@ extern "C"
     // Logging function
     void simulith_log(const char *fmt, ...);
 
+#ifdef SIMULITH_TESTING
+    /* Test-only helper to reset logging state between tests. Only available
+     * when building tests (SIMULITH_TESTING). */
+    void simulith_log_reset_for_tests(void);
+#endif
+
     // ---------- Server API ----------
 
     /**

src/simulith_common.c

@@ -2,6 +2,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <stdarg.h>
 
 typedef enum {
     LOG_MODE_STDOUT,
@@ -10,9 +11,18 @@ typedef enum {
     LOG_MODE_NONE
 } simulith_log_mode_t;
 
+/* When building tests we make these globals non-static so test code can
+ * inspect/reset them. In production builds they remain translation-unit
+ * static to preserve encapsulation. */
+#ifdef SIMULITH_TESTING
+simulith_log_mode_t simulith_log_mode = LOG_MODE_STDOUT;
+int simulith_log_mode_initialized = 0;
+FILE *simulith_log_file = NULL;
+#else
 static simulith_log_mode_t simulith_log_mode = LOG_MODE_STDOUT;
 static int simulith_log_mode_initialized = 0;
 static FILE *simulith_log_file = NULL;
+#endif
 
 static void simulith_log_init_mode(void) {
     const char *env = getenv("SIMULITH_LOG_MODE");
@@ -72,4 +82,19 @@ void simulith_log(const char *fmt, ...)
     } else {
         va_end(args);
     }
-}
+}
+
+#ifdef SIMULITH_TESTING
+/* Test-only helper: reset logging to uninitialized state and close any open
+ * log file. Tests should call this between cases to avoid cross-test
+ * interference. */
+void simulith_log_reset_for_tests(void)
+{
+    if (simulith_log_file) {
+        fclose(simulith_log_file);
+        simulith_log_file = NULL;
+    }
+    simulith_log_mode_initialized = 0;
+    simulith_log_mode = LOG_MODE_STDOUT;
+}
+#endif

src/simulith_server.c

@@ -1,5 +1,6 @@
 #include "simulith.h"
 #include <sched.h>
+#include <signal.h>
 
 #define MAX_CLIENTS 32
 
@@ -17,6 +18,9 @@ static uint64_t    tick_interval_ns           = 0;
 static int         expected_clients           = 0;
 static ClientState client_states[MAX_CLIENTS] = {0};
 
+/* Test/debug helper: request server shutdown from other threads. */
+static volatile sig_atomic_t simulith_server_stop_requested = 0;
+
 static int is_client_id_taken(const char *id)
 {
     for (int i = 0; i < MAX_CLIENTS; ++i)
@@ -32,6 +36,9 @@ static int is_client_id_taken(const char *id)
 
 int simulith_server_init(const char *pub_bind, const char *rep_bind, int client_count, uint64_t interval_ns)
 {
+    /* Clear any previous stop request so a fresh server run isn't short-circuited. */
+    simulith_server_stop_requested = 0;
+
     // Validate parameters
     if (client_count <= 0 || client_count > MAX_CLIENTS)
     {
@@ -75,6 +82,12 @@ int simulith_server_init(const char *pub_bind, const char *rep_bind, int client_
         return -1;
     }
 
+    /* Set a short receive timeout on the responder so the server loop can
+     * periodically check for shutdown requests and avoid getting stuck in a
+     * blocking recv during tests. */
+    int recv_timeout_ms = 200;
+    zmq_setsockopt(responder, ZMQ_RCVTIMEO, &recv_timeout_ms, sizeof(recv_timeout_ms));
+
     // Optimize responder settings
     int rcvhwm = 1000;
     zmq_setsockopt(responder, ZMQ_RCVHWM, &rcvhwm, sizeof(rcvhwm));
@@ -108,9 +121,9 @@ static void broadcast_time(void)
         // Calculate actual speed (sim seconds per real second)
         struct timespec now_ts;
         clock_gettime(CLOCK_MONOTONIC, &now_ts);
-    uint64_t now_real_ns = (uint64_t)now_ts.tv_sec * 1000000000ULL + (uint64_t)now_ts.tv_nsec;
-    double sim_elapsed = (double)(current_time_ns - last_log_time) / 1e9;
-    double real_elapsed = (g_last_log_real_ns > 0) ? ((double)(now_real_ns - g_last_log_real_ns) / 1e9) : 0.0;
+        uint64_t now_real_ns = (uint64_t)now_ts.tv_sec * 1000000000ULL + (uint64_t)now_ts.tv_nsec;
+        double sim_elapsed = (double)(current_time_ns - last_log_time) / 1e9;
+        double real_elapsed = (g_last_log_real_ns > 0) ? ((double)(now_real_ns - g_last_log_real_ns) / 1e9) : 0.0;
         double actual_speed = (real_elapsed > 0.0) ? (sim_elapsed / real_elapsed) : 0.0;
 
         simulith_log("  Simulation time: %.3f seconds | Attempted speed: %.2fx | Actual: %.2fx\n",
@@ -161,6 +174,11 @@ void simulith_server_run(void)
     int ready_clients = 0;
     while (ready_clients < expected_clients)
     {
+        if (simulith_server_stop_requested) {
+            simulith_log("Server shutdown requested while waiting for READY\n");
+            return;
+        }
+
         char buffer[64] = {0};
         int  size       = zmq_recv(responder, buffer, sizeof(buffer) - 1, 0);
         if (size > 0)
@@ -220,6 +238,20 @@ void simulith_server_run(void)
             zmq_send(responder, "ACK", 3, 0);
             simulith_log("Registered client %s (%d/%d)\n", client_id, ready_clients, expected_clients);
         }
+        else
+        {
+            /* recv timed out or failed; check for shutdown and continue waiting */
+            if (errno == EAGAIN)
+            {
+                /* timeout - loop again so we can detect shutdown requests */
+                continue;
+            }
+            else
+            {
+                simulith_log("Error receiving handshake: %s\n", strerror(errno));
+                continue;
+            }
+        }
     }
 
     simulith_log("All clients ready. Starting time broadcast.\n");
@@ -403,6 +435,9 @@ void simulith_server_run(void)
 
 void simulith_server_shutdown(void)
 {
+    /* Request the server loop to stop, then proceed to close sockets. */
+    simulith_server_stop_requested = 1;
+
     if (publisher)
         zmq_close(publisher);
     if (responder)

test/CMakeLists.txt

@@ -1,6 +1,6 @@
 # Unity source and include path
-set(UNITY_SRC ../unity/unity.c)
-set(UNITY_INC ../unity)
+set(UNITY_SRC ./unity/unity.c)
+set(UNITY_INC ./unity)
 
 # Include paths for tests
 include_directories(
@@ -9,12 +9,27 @@ include_directories(
     ${ZeroMQ_INCLUDE_DIRS}
 )
 
-# Main simulith tests executable
+add_executable(test_42_command_api test_42.c ${UNITY_SRC})
+target_link_libraries(test_42_command_api simulith ${ZeroMQ_LIBRARIES} pthread)
+target_compile_definitions(test_42_command_api PRIVATE SIMULITH_TESTING)
+add_test(NAME 42Tests COMMAND test_42_command_api)
+
+add_executable(test_common test_common.c ${UNITY_SRC})
+target_link_libraries(test_common simulith ${ZeroMQ_LIBRARIES})
+target_compile_definitions(test_common PRIVATE SIMULITH_TESTING)
+add_test(NAME CommonTests COMMAND test_common)
+
 add_executable(test_simulith test_simulith.c ${UNITY_SRC})
 target_link_libraries(test_simulith simulith ${ZeroMQ_LIBRARIES} pthread)
-add_test(NAME SimulithTest COMMAND test_simulith)
+target_compile_definitions(test_simulith PRIVATE SIMULITH_TESTING)
+add_test(NAME SimulithTests COMMAND test_simulith)
+
+add_executable(test_time test_time.c ${UNITY_SRC})
+target_link_libraries(test_time simulith ${ZeroMQ_LIBRARIES} pthread)
+target_compile_definitions(test_time PRIVATE SIMULITH_TESTING)
+add_test(NAME TimeTests COMMAND test_time)
 
-# Transport tests executable
 add_executable(test_transport test_transport.c ${UNITY_SRC})
 target_link_libraries(test_transport simulith ${ZeroMQ_LIBRARIES})
-add_test(NAME TransportTest COMMAND test_transport)
+target_compile_definitions(test_transport PRIVATE SIMULITH_TESTING)
+add_test(NAME TransportTests COMMAND test_transport)