refactor(facade): migrate all facade APIs from throw to Result<T>#952
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. |
1 participant
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