refactor(facade): migrate all facade APIs from throw to Result<T>#952
Conversation
Replace 30 throw statements across 5 facade files with Result<T> return values, unifying error handling with the ecosystem convention. Changes per facade (tcp, quic, udp, websocket, http): - validate_* return type: void -> VoidResult - create_client/create_server return type: shared_ptr -> Result<shared_ptr> - create_connection_pool return type: shared_ptr -> Result<shared_ptr> - throw std::invalid_argument -> return error_void(-1, msg, source) - throw std::runtime_error -> return error<T>(code, msg, source) - Success paths: return ptr -> return ok(ptr) Also fix broken markdown anchors in docs TOC files. Closes #951
Performance ComparisonBase Branch ResultsNo base results PR Branch ResultsNo PR results |
Migrate all facade test assertions from exception-based to Result-based: - EXPECT_THROW -> result.is_err() - EXPECT_NO_THROW(var = ...) -> ASSERT_TRUE(result.is_ok()); var = result.value() - Direct assignment -> result check + value extraction
Performance ComparisonBase Branch ResultsNo base results PR Branch ResultsNo PR results |
…opes Rename second 'auto result' to 'send_result' or 'stop_result' in tests where create_* result and interface method result share scope.
Performance ComparisonBase Branch ResultsNo base results PR Branch ResultsNo PR results |
facade create_client() and create_server() now return Result<shared_ptr<T>> instead of shared_ptr<T>. Update all call sites in the benchmark file to unwrap the Result with .value() before using the pointer.
CI Failure Analysis — PR #952SummaryAll CI failures are build errors caused by incomplete Error Category 1: Benchmark —
|
| Line | Code | Issue |
|---|---|---|
| 141 | auto client = facade.create_client(...) |
Returns Result<shared_ptr<...>>, then line 149 calls client->stop() on Result |
| 191 | auto server = facade.create_server(...) |
Returns Result<shared_ptr<...>>, then line 197 calls server->stop() on Result |
| 345 | server = facade.create_server(...) |
Assigns Result<shared_ptr<...>> to shared_ptr<i_protocol_server> |
| 360 | client = facade.create_client(...) |
Assigns Result<shared_ptr<...>> to shared_ptr<i_protocol_client> |
| 641 | server = facade.create_server(...) |
Same type mismatch |
| 667 | auto client = facade.create_client(...) |
Returns Result, line 674 calls client->start() on Result |
Proposed fix: Unwrap Result values before use. For benchmarks, use .value() after checking .is_ok(), or skip the benchmark iteration on error.
Error Category 2: Unit test — Result<T> vs nullptr comparison
Workflow: Integration Tests (unit test target)
Jobs: ubuntu-latest Debug/Release, macos-latest Debug/Release
Step: Build
File: tests/unit/facade_id_generation_test.cpp
Root Cause: create_client(), create_server(), and create_connection_pool() now return Result<shared_ptr<T>> instead of shared_ptr<T>. GTest's EXPECT_NE(result, nullptr) cannot compare Result<T> with nullptr.
Affected lines:
| Line | Code | Issue |
|---|---|---|
| 128-129 | auto pool = ...; EXPECT_NE(pool, nullptr) |
pool is Result<shared_ptr<...>> |
| 138-139 | Same pattern | Result<T> vs nullptr |
| 148-149 | Same pattern | Result<T> vs nullptr |
| 378-379 | auto client = ...; EXPECT_NE(client, nullptr) |
http_facade Result<T> |
| 387-388 | Same pattern | http_facade |
| 397-398 | Same pattern | http_facade |
| 479-480 | Same pattern | websocket_facade |
| 488-489 | Same pattern | websocket_facade |
Additional runtime issue (compiles but will fail at test time):
| Lines | Code | Issue |
|---|---|---|
| 118, 158, 292, 318, 424, 538, 578, 613 | EXPECT_THROW(..., std::invalid_argument) |
Facades no longer throw; they return Result with error state |
Proposed fix: Check result.is_ok() via EXPECT_TRUE(result.is_ok()), then verify result.value() != nullptr. For EXPECT_THROW tests, change to EXPECT_TRUE(result.is_err()) to verify error Result is returned.
Error Category 3: E2E test — [[nodiscard]] VoidResult ignored
Workflow: Integration Tests
Jobs: ubuntu-latest Debug/Release, macos-latest Debug, windows-latest Debug
Step: Build
File: tests/integration/test_e2e.cpp
Root Cause: messaging_client::start_client(), stop_client(), send_packet() and messaging_server::start_server(), stop_server() now return [[nodiscard]] VoidResult. The E2E test ignores all return values. With -Werror, these discarded-value warnings become compilation errors.
Affected lines (29 occurrences):
start_server(): lines 107, 160, 233, 279, 306, 353start_client(): lines 113, 173, 238, 288, 311, 362send_packet(): lines 120, 182, 249, 294, 316, 366stop_client(): lines 126, 187, 255, 298, 319, 370stop_server(): lines 127, 204, 256, 302, 320, 376
Proposed fix: Either cast to (void) to explicitly discard, or check the result and handle errors (preferred for E2E test reliability).
Failed CI Runs Summary
| Workflow | Run ID | Failing Jobs | Failing Step |
|---|---|---|---|
| Integration Tests | 24324461004 | ubuntu-latest Debug/Release, macos-latest Debug/Release | Build |
| Integration Tests | 24324461012 | macos-latest Debug, windows-latest Debug, ubuntu-latest Debug/Release | Build |
| Benchmarks | 24324461008 | ubuntu-24.04 clang Release, macos-15 clang Release | Build benchmarks |
Facade create methods now return Result<shared_ptr<T>> instead of throwing exceptions or returning raw pointers. Update tests to: - Use .is_ok() and .value() to unwrap Result before nullptr checks - Replace EXPECT_THROW(invalid_argument) with is_err() assertions for port validation tests across TCP, UDP, HTTP, WebSocket, and QUIC facades
start_server(), stop_server(), start_client(), stop_client(), and send_packet() now return [[nodiscard]] VoidResult. Cast all call sites to (void) to suppress -Werror=unused-result warnings.
Update all facade code examples and API signatures to reflect the migration from throw-based to Result<T> return types in create_client(), create_server(), and create_connection_pool() methods.
Performance ComparisonBase Branch ResultsNo base results PR Branch ResultsNo PR results |
Coverage Report
Coverage DetailsFull HTML report is available as a build artifact. |
…gger OpenSSL) (#1167) * fix(ci): disable samples in Network Load Tests build The Network Load Tests workflow (Ubuntu / macOS / Windows) configured without -DBUILD_SAMPLES=OFF. BUILD_SAMPLES defaults ON in the root CMakeLists, so the build compiled samples/, which were not migrated to the facade Result<T> API (#952) and fail to compile (e.g. basic_usage.cpp treats the Result return of create_server/create_client as a pointer). ninja stops before the actual load-test binaries (tcp/udp/websocket_load_test) are built, so all three jobs fail. Add -DBUILD_SAMPLES=OFF to the three Configure CMake steps, matching the sibling workflows (ci.yml, integration-tests.yml, coverage.yml, sanitizers.yml) which already pass it. Samples are unrelated to load testing. The samples' Result<T> migration and a concepts_example MSVC header omission remain as separate real-code follow-ups, out of scope for this CI fix. * fix(ci): restore network build CI (vcpkg baseline, X509 100y, logger OpenSSL) Three develop CI build failures resolved alongside the Network Load Tests samples fix: 1. CI (ci.yml) vcpkg install failed on every platform. The kcenon/vcpkg-registry baseline in vcpkg-configuration.json was pinned to a3c5d7c8 (2026-04-09), before the registry corrected common_system's v0.2.0 SHA512 (#88, 2026-05-03); vcpkg fetched a portfile with the stale hash and rejected the tarball ("unexpected hash"). Bump the baseline to 1be52cbd (registry HEAD), which carries the correct hash. 2. Integration Tests (windows) failed to compile. mock_tls_socket.cpp computed a 100-year validity as `60L*60*24*365*100` (~3.15e9 s), overflowing a 32-bit long on LLP64/Windows (constexpr overflow error). Use X509_time_adj_ex (day-based) which sidesteps the long-seconds limit and is cross-platform. 3. Integration Tests (ubuntu Release) failed to link network_messaging_server_lifecycle_test: logger_system's HMAC integrity policy uses OpenSSL EVP_MAC, but the network_add_messaging_test helper did not link libcrypto, producing undefined references. Add OpenSSL::Crypto (guarded by TARGET_NAME_IF_EXISTS) to that helper, matching the explicit OpenSSL links already present on the TLS test targets. * fix(ci): force-find OpenSSL and link libcrypto on messaging tests The previous $<TARGET_NAME_IF_EXISTS:OpenSSL::Crypto> guard linked nothing because network's find_package(OpenSSL) is gated behind BUILD_TLS_SUPPORT, leaving OpenSSL::Crypto undefined for the messaging tests' generate step. Call find_package(OpenSSL 3.0.0 REQUIRED) inside network_add_messaging_test and link OpenSSL::Crypto unconditionally so logger_system's EVP_MAC symbols (rotating_file_writer compute_hmac) resolve at link time. * fix(ci): preserve logger libcrypto link ordering
Summary
Result<T>returnsvalidate_*method signatures:void->VoidResultcreate_*method signatures:shared_ptr<T>->Result<shared_ptr<T>>Related Issues
Closes #951
Files Changed
include/kcenon/network/facade/src/facade/docs/Test Plan
throw std::in facade sources