diff --git a/design.md b/design.md index b4897ed..ff6cba0 100644 --- a/design.md +++ b/design.md @@ -393,7 +393,7 @@ Only Server can create connections (using private constructor via friend access) #### Simple Synchronous Handler ```cpp -class HttpHandler : public ConnectionHandler { +class HttpHandler : ConnectionHandler { public: void on_data_arrived(std::string_view data, Connection& conn) override { // Parse HTTP request using connection's arena @@ -410,7 +410,7 @@ public: #### Async Handler with WeakRef ```cpp -class AsyncHandler : public ConnectionHandler { +class AsyncHandler : ConnectionHandler { public: void on_data_arrived(std::string_view data, Connection& conn) override { // Get weak reference for async processing @@ -429,7 +429,7 @@ public: #### Batching Handler with User Data ```cpp -class BatchingHandler : public ConnectionHandler { +class BatchingHandler : ConnectionHandler { public: void on_connection_established(Connection &conn) override { // Allocate some protocol-specific data and attach it to the connection @@ -468,7 +468,7 @@ private: #### Streaming "yes" Handler ```cpp -class YesHandler : public ConnectionHandler { +class YesHandler : ConnectionHandler { public: void on_connection_established(Connection &conn) override { // Write an initial "y\n" diff --git a/src/arena.cpp b/src/arena.cpp index b6d2461..2319061 100644 --- a/src/arena.cpp +++ b/src/arena.cpp @@ -1,4 +1,5 @@ #include "arena.hpp" + #include #include #include diff --git a/src/arena.hpp b/src/arena.hpp index 5acbf53..efe85dc 100644 --- a/src/arena.hpp +++ b/src/arena.hpp @@ -59,10 +59,9 @@ * * ### Safe Usage Patterns in WeaselDB: * - **Per-Connection Instances**: Each Connection owns its own Arena - * instance, accessed only by the thread that currently owns the connection - * - **Single Owner Principle**: Connection ownership transfers atomically - * between threads using unique_ptr, ensuring only one thread accesses the arena - * at a time + * instance, accessed by its io thread + * - **Server Ownership**: Server retains connection ownership, handlers access + * arenas through Connection& references with proper mutex protection * * ### Thread Ownership Model: * 1. **I/O Thread**: Server owns connections, processes socket I/O events diff --git a/src/connection.cpp b/src/connection.cpp index f1d49ac..a988503 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -45,8 +45,7 @@ Connection::Connection(struct sockaddr_storage addr, int fd, int64_t id, : fd_(fd), id_(id), epoll_index_(epoll_index), addr_(addr), handler_(handler), server_(std::move(server)) { auto server_ref = server_.lock(); - // This should only be called from a member of Server itself, so I should - // hope it's alive. + // Should only be called from the io thread assert(server_ref); server_ref->active_connections_.fetch_add(1, std::memory_order_relaxed); @@ -96,19 +95,23 @@ void Connection::append_message(std::span data_parts, bool was_empty = message_queue_.empty(); // Add message to queue + // TODO this allocates while holding the connection lock message_queue_.emplace_back( Message{std::move(arena), data_parts, close_after_send}); outgoing_bytes_queued_ += total_bytes; // If queue was empty, we need to add EPOLLOUT interest. - if (was_empty && fd_ >= 0) { + if (was_empty) { auto server = server_.lock(); - if (server) { + if (fd_ >= 0 && server) { // Add EPOLLOUT interest - pipeline thread manages epoll struct epoll_event event; event.data.fd = fd_; event.events = EPOLLIN | EPOLLOUT; tsan_release(); + // I think we have to call epoll_ctl while holding mutex_. Otherwise a + // call that clears the write interest could get reordered with one that + // sets it and we would hang. epoll_ctl(server->epoll_fds_[epoll_index_], EPOLL_CTL_MOD, fd_, &event); } } @@ -133,9 +136,8 @@ int Connection::readBytes(char *buf, size_t buffer_size) { } // Increment bytes read metric - if (r > 0) { - bytes_read.inc(r); - } + assert(r > 0); + bytes_read.inc(r); return r; } @@ -196,9 +198,7 @@ uint32_t Connection::write_bytes() { if (errno == EAGAIN) { // Increment EAGAIN failure metric write_eagain_failures.inc(); - if (total_bytes_written > 0) { - bytes_written.inc(total_bytes_written); - } + bytes_written.inc(total_bytes_written); return result; } perror("sendmsg"); @@ -221,7 +221,6 @@ uint32_t Connection::write_bytes() { while (bytes_remaining > 0 && !message_queue_.empty()) { auto &front_message = message_queue_.front(); - bool message_complete = true; for (auto &part : front_message.data_parts) { if (part.empty()) @@ -236,22 +235,17 @@ uint32_t Connection::write_bytes() { part = std::string_view(part.data() + bytes_remaining, part.size() - bytes_remaining); bytes_remaining = 0; - message_complete = false; break; } } - if (message_complete) { - if (front_message.close_after_send) { - result |= Close; - } - // Move arena to thread-local vector for deferred cleanup - g_arenas_to_free.emplace_back(std::move(front_message.arena)); - message_queue_.pop_front(); - if (result & Close) { - break; - } - } else { + if (front_message.close_after_send) { + result |= Close; + } + // Move arena to thread-local vector for deferred cleanup + g_arenas_to_free.emplace_back(std::move(front_message.arena)); + message_queue_.pop_front(); + if (result & Close) { break; } } @@ -269,18 +263,19 @@ uint32_t Connection::write_bytes() { event.data.fd = fd_; event.events = EPOLLIN; // Remove EPOLLOUT tsan_release(); + // I think we have to call epoll_ctl while holding mutex_. Otherwise a + // call that clears the write interest could get reordered with one that + // sets it and we would hang. epoll_ctl(server->epoll_fds_[epoll_index_], EPOLL_CTL_MOD, fd_, &event); } } } // Increment bytes written metric - if (total_bytes_written > 0) { - bytes_written.inc(total_bytes_written); - } + bytes_written.inc(total_bytes_written); // Clean up arenas after all mutex operations are complete - // This avoids holding the connection mutex while free() potentially contends + // This avoids holding the connection mutex while calling free() g_arenas_to_free.clear(); return result; diff --git a/src/connection.hpp b/src/connection.hpp index 22ce42c..669d5aa 100644 --- a/src/connection.hpp +++ b/src/connection.hpp @@ -34,7 +34,9 @@ struct MessageSender { * * Thread-safe method that can be called from any thread, including * pipeline processing threads. The arena is moved into the connection - * to maintain data lifetime until the message is sent. + * to maintain data lifetime until the message is sent. Messages appended + * concurrently may be written in either order, but they will not be + * interleaved. * * @param data_parts Span of string_view parts to send (arena-allocated) * @param arena Arena containing the memory for data_parts string_views @@ -55,20 +57,21 @@ struct MessageSender { }; /** - * Represents a single client connection with thread-safe concurrent access. + * Represents a single client connection - the full interface available to the + * io thread and connection handler. * * Connection ownership model: * - Server owns all connections * - Handlers receive Connection& references, and can keep a WeakRef to - * Connection for async responses. - * - Multiple pipeline threads can safely access connection concurrently + * MessageSender for async responses. + * - Multiple pipeline threads can safely access the MessageSender concurrently * - I/O thread has exclusive access to socket operations * * Threading model: - * - Single mutex protects all connection state + * - Single mutex protects state shared with pipeline threads * - Pipeline threads call Connection methods (append_message, etc.) * - I/O thread processes socket events and message queue - * - Pipeline threads manage epoll interests via Connection methods + * - Pipeline threads register epoll write interest via append_message * - Connection tracks closed state to prevent EBADF errors * * Arena allocator usage: @@ -258,7 +261,7 @@ struct Connection : MessageSender { * * Example usage: * ```cpp - * class HttpHandler : public ConnectionHandler { + * class HttpHandler : ConnectionHandler { * void on_connection_established(Connection& conn) override { * // Allocate HTTP state in connection's arena or heap * auto* state = conn.get_arena().construct(); @@ -272,8 +275,8 @@ struct Connection : MessageSender { * } * * void on_data_arrived(std::string_view data, - * Ref& conn_ptr) override { - * auto* state = static_cast(conn_ptr->user_data); + * Connection& conn) override { + * auto* state = static_cast(conn.user_data); * // Use state for protocol processing... * } * }; @@ -323,28 +326,25 @@ private: }; uint32_t write_bytes(); - // Direct access methods for Server (must hold mutex) - int getFd() const { return fd_; } - bool has_messages() const { return !message_queue_.empty(); } - size_t getEpollIndex() const { return epoll_index_; } void close(); // Immutable connection properties - int fd_; const int64_t id_; const size_t epoll_index_; // Index of the epoll instance this connection uses struct sockaddr_storage addr_; // sockaddr_storage handles IPv4/IPv6 ConnectionHandler *const handler_; - WeakRef server_; // Weak reference to server for safe cleanup + WeakRef server_; // Weak reference to server for safe epoll_ctl calls WeakRef self_ref_; // WeakRef to self for get_weak_ref() - // Thread-safe state (protected by mutex_) + // state shared with pipeline threads (protected by mutex_) mutable std::mutex mutex_; // Protects all mutable state std::deque - message_queue_; // Queue of messages to send. Protectec by + message_queue_; // Queue of messages to send. Protected by // mutex_, but if non-empty mutex_ can be // dropped while server accesses existing elements. int64_t outgoing_bytes_queued_{0}; // Counter of queued bytes + // Set to a negative number in `close` + int fd_; #if __has_feature(thread_sanitizer) void tsan_acquire() { tsan_sync.load(std::memory_order_acquire); } diff --git a/src/connection_handler.hpp b/src/connection_handler.hpp index 656aab9..4020e34 100644 --- a/src/connection_handler.hpp +++ b/src/connection_handler.hpp @@ -51,6 +51,7 @@ public: * @param conn Connection that made write progress - server retains ownership * @note Called from this connection's io thread. * @note Called during writes, not necessarily when buffer becomes empty + * TODO Add bytes written argument? */ virtual void on_write_progress(Connection &) {} diff --git a/src/connection_registry.cpp b/src/connection_registry.cpp index af98f8f..61f5b43 100644 --- a/src/connection_registry.cpp +++ b/src/connection_registry.cpp @@ -13,6 +13,7 @@ ConnectionRegistry::ConnectionRegistry() : connections_(nullptr), max_fds_(0) { } max_fds_ = rlim.rlim_cur; + // TODO re-enable "ondemand pages" behavior // // Calculate size rounded up to page boundary // size_t array_size = max_fds_ * sizeof(Connection *); // size_t page_size = getpagesize(); diff --git a/src/connection_registry.hpp b/src/connection_registry.hpp index 7fb1e29..9e7e1ad 100644 --- a/src/connection_registry.hpp +++ b/src/connection_registry.hpp @@ -34,10 +34,10 @@ public: /** * Store a connection in the registry, indexed by its file descriptor. - * Takes ownership of the connection via unique_ptr. + * Takes a reference to the connection for storage. * * @param fd File descriptor (must be valid and < max_fds_) - * @param connection unique_ptr to the connection (ownership transferred) + * @param connection Ref to store in the registry */ void store(int fd, Ref connection); diff --git a/src/http_handler.cpp b/src/http_handler.cpp index 2db1eab..5db0e51 100644 --- a/src/http_handler.cpp +++ b/src/http_handler.cpp @@ -69,14 +69,17 @@ void HttpHandler::on_connection_closed(Connection &conn) { conn.user_data = nullptr; } +// TODO there might be an issue if we get pipelined requests here + void HttpHandler::on_write_buffer_drained(Connection &conn) { - // Reset state after all messages have been written for the next request + // Reset state after entire reply messages have been written for the next + // request auto *state = static_cast(conn.user_data); if (state) { TRACE_EVENT("http", "reply", perfetto::Flow::Global(state->http_request_id)); } - // TODO we don't need this anymore. Look at removing it. + // TODO consider replacing with HttpConnectionState->reset() on_connection_closed(conn); // Note: Connection reset happens at server level, not connection level on_connection_established(conn); @@ -109,7 +112,8 @@ void HttpHandler::on_batch_complete(std::span batch) { } } - // Send requests to 4-stage pipeline in batch + // Send requests to 4-stage pipeline in batch. Batching here reduces + // contention on the way into the pipeline. if (pipeline_count > 0) { auto guard = commitPipeline.push(pipeline_count, true); auto out_iter = guard.batch.begin(); @@ -396,7 +400,7 @@ void HttpHandler::handle_get_metrics(Connection &conn, conn.append_message(result, std::move(state.arena)); } -void HttpHandler::handle_get_ok(Connection &, HttpConnectionState &state) { +void HttpHandler::handle_get_ok(Connection &, HttpConnectionState &) { ok_counter.inc(); TRACE_EVENT("http", "GET /ok", perfetto::Flow::Global(state.http_request_id)); diff --git a/src/http_handler.hpp b/src/http_handler.hpp index 03fd4c4..34b57f6 100644 --- a/src/http_handler.hpp +++ b/src/http_handler.hpp @@ -133,7 +133,7 @@ struct HttpHandler : ConnectionHandler { void on_connection_closed(Connection &conn) override; void on_data_arrived(std::string_view data, Connection &conn) override; void on_batch_complete(std::span batch) override; - void on_write_buffer_drained(Connection &conn_ptr) override; + void on_write_buffer_drained(Connection &conn) override; // llhttp callbacks (public for HttpConnectionState access) static int onUrl(llhttp_t *parser, const char *at, size_t length); diff --git a/src/metric.hpp b/src/metric.hpp index 457cd72..3b0c37e 100644 --- a/src/metric.hpp +++ b/src/metric.hpp @@ -45,7 +45,6 @@ #include #include -#include #include #include #include diff --git a/src/process_collector.hpp b/src/process_collector.hpp index ce1d1b7..2d3689f 100644 --- a/src/process_collector.hpp +++ b/src/process_collector.hpp @@ -8,7 +8,7 @@ * Gathers metrics like CPU usage, memory, and file descriptors by reading * files from the /proc filesystem. */ -struct ProcessCollector : public metric::Collector { +struct ProcessCollector : metric::Collector { /** * @brief Constructs the collector and initializes the process metrics. */ diff --git a/src/server.cpp b/src/server.cpp index 835ec2b..233da48 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -324,7 +324,7 @@ void Server::start_io_threads(std::vector &threads) { // Process existing connections in batch if (batch_count > 0) { process_connection_batch( - epollfd, std::span(batch).subspan(0, batch_count), + std::span(batch).subspan(0, batch_count), std::span(batch_events).subspan(0, batch_count)); } @@ -388,7 +388,7 @@ void Server::start_io_threads(std::vector &threads) { // Process batch if full if (batch_count == config_.server.event_batch_size) { process_connection_batch( - epollfd, {batch.data(), (size_t)batch_count}, + {batch.data(), (size_t)batch_count}, {batch_events.data(), (size_t)batch_count}); batch_count = 0; } @@ -398,7 +398,7 @@ void Server::start_io_threads(std::vector &threads) { // Process remaining accepted connections if (batch_count > 0) { process_connection_batch( - epollfd, std::span(batch).subspan(0, batch_count), + std::span(batch).subspan(0, batch_count), std::span(batch_events).subspan(0, batch_count)); batch_count = 0; } @@ -463,10 +463,9 @@ void Server::close_connection(Ref &conn) { conn.reset(); } -static thread_local std::vector conn_ptrs; +static thread_local std::vector batch_connections; -void Server::process_connection_batch(int epollfd, - std::span> batch, +void Server::process_connection_batch(std::span> batch, std::span events) { // First process writes for each connection @@ -484,19 +483,19 @@ void Server::process_connection_batch(int epollfd, } // Call batch complete handler with connection pointers - conn_ptrs.clear(); - for (auto &conn_ref : batch) { - if (conn_ref) { - conn_ptrs.push_back(conn_ref.get()); + batch_connections.clear(); + for (auto &conn : batch) { + if (conn) { + batch_connections.push_back(conn.get()); } } - handler_.on_batch_complete(conn_ptrs); + handler_.on_batch_complete(batch_connections); - // Transfer all remaining connections back to registry - for (auto &conn_ptr : batch) { - if (conn_ptr) { - int fd = conn_ptr->getFd(); - connection_registry_.store(fd, std::move(conn_ptr)); + // Return all connections to registry + for (auto &conn : batch) { + if (conn) { + const int fd = conn->fd_; + connection_registry_.store(fd, std::move(conn)); } } } diff --git a/src/server.hpp b/src/server.hpp index 581b1a4..7722507 100644 --- a/src/server.hpp +++ b/src/server.hpp @@ -1,7 +1,6 @@ #pragma once #include -#include #include #include #include @@ -101,20 +100,21 @@ private: * Private constructor - use create() factory method instead. * * @param config Server configuration (threads, ports, limits, etc.) - * @param handler Protocol handler for processing connection data + * @param handler Protocol handler for processing connection data. Must + * outlive the server. * @param listen_fds Vector of file descriptors to accept connections on. - * Server takes ownership and will close them on + * Server takes ownership and will close them on * destruction. Server will set these to non-blocking mode for safe epoll * usage. */ explicit Server(const weaseldb::Config &config, ConnectionHandler &handler, const std::vector &listen_fds); - friend Ref make_ref(const weaseldb::Config &config, - ConnectionHandler &handler, - const std::vector &listen_fds); + template + friend Ref make_ref(Args &&...args); + WeakRef self_; - const weaseldb::Config &config_; + weaseldb::Config config_; ConnectionHandler &handler_; // Connection registry @@ -145,25 +145,15 @@ private: int get_epoll_for_thread(int thread_id) const; // Helper for processing connection I/O - void process_connection_reads(Ref &conn_ptr, int events); - void process_connection_writes(Ref &conn_ptr, int events); + void process_connection_reads(Ref &conn, int events); + void process_connection_writes(Ref &conn, int events); void close_connection(Ref &conn); // Helper for processing a batch of connections with their events - void process_connection_batch(int epollfd, std::span> batch, + void process_connection_batch(std::span> batch, std::span events); - /** - * Called internally to return ownership to the server. - * - * This method is thread-safe and can be called from any thread. - * The connection will be re-added to the epoll for continued processing. - * - * @param connection Unique pointer to the connection being released back - */ - void receiveConnectionBack(Ref connection); - // Make non-copyable and non-movable Server(const Server &) = delete; Server &operator=(const Server &) = delete; diff --git a/style.md b/style.md index 6871587..f244e56 100644 --- a/style.md +++ b/style.md @@ -21,7 +21,6 @@ ______________________________________________________________________ - **C++20** is the target standard - Use modern C++ features: RAII, move semantics, constexpr, concepts where appropriate -- Prefer standard library containers and algorithms over custom implementations ### C Library Functions and Headers @@ -65,16 +64,16 @@ signal(SIGTERM, handler); - Interfacing with APIs that require unsigned types - Where defined unsigned overflow behavior (wraparound) is intentional and desired - **Almost always auto** - let the compiler deduce types except when: - - The type is not obvious from context (prefer explicit for clarity) + - The type is not obvious from context and the exact type is important (prefer explicit for clarity) - Specific type requirements matter (numeric conversions, template parameters) - Interface contracts need explicit types (public APIs, function signatures) - **Prefer uninitialized memory to default initialization** when using before initializing would be an error - Valgrind will catch uninitialized memory usage bugs - - Avoid hiding logic errors with unnecessary zero-initialization + - Avoid hiding logic errors that Valgrind would have caught with unnecessary zero-initialization - Default initialization can mask bugs and hurt performance - **Floating point is for metrics only** - avoid `float`/`double` in core data structures and algorithms - Use for performance measurements, statistics, and monitoring data - - Never use for counts, sizes, or business logic + - Avoid branching on the values of floats ### Type Casting @@ -106,7 +105,7 @@ auto addr = reinterpret_cast(ptr); // Pointer to integer conv - **Strive for 0% CPU usage when idle** - avoid polling, busy waiting, or unnecessary background activity - Use **inline functions** for performance-critical code (e.g., `allocate_raw`) - **String views** with `std::string_view` to minimize unnecessary copying -- **Arena allocation** for efficient memory management (~1ns vs ~20-270ns for malloc) +- **Arena allocation** for efficient memory management, and to group related lifetimes together for simplicity ### String Formatting @@ -131,6 +130,8 @@ std::string_view response = format(arena, static_cast(body.size()), body.data()); ``` +- Offer APIs that let you avoid concatenating strings if possible - e.g. if the bytes are going to get written to a file descriptor you can skip concatenating and use scatter/gather writev-type calls. + ### Complexity Control - **Encapsulation is the main tool for controlling complexity** @@ -141,7 +142,7 @@ std::string_view response = format(arena, - Thread safety guarantees - Performance characteristics - Ownership and lifetime semantics -- **Do not rely on undocumented interface properties** - if it's not in the header, don't depend on it +- **Do not rely on undocumented properties of an interface** - if it's not in the header, don't depend on it ______________________________________________________________________ @@ -165,17 +166,16 @@ int32_t initial_block_size_; - **Public members first, private after** - puts the interface users care about at the top, implementation details below - **Full encapsulation still applies** - use `private:` sections to hide implementation details and maintain deep, capable structs - The struct keyword doesn't mean shallow design - it means interface-first organization for human readers +- Omit the `public` keyword when inheriting from a struct. It's public by default. E.g. `struct A : B {};` instead of `struct A : public B {};` ```cpp -struct Arena { +struct MyClass { // Public interface first - explicit Arena(int64_t initial_size = 1024); - void* allocate_raw(int64_t size); + void do_thing(); private: // Private members after - int32_t initial_block_size_; - Block* current_block_; + int thing_count_; }; ``` @@ -183,6 +183,7 @@ private: - **PascalCase** for enum class names - **PascalCase** for enum values (not SCREAMING_SNAKE_CASE) +- C-style enums are acceptable where implicit int conversion is desirable, like for bitflags ```cpp enum class Type { @@ -270,7 +271,7 @@ ______________________________________________________________________ - **Move-only semantics** for resource-owning types - **Explicit constructors** to prevent implicit conversions -- **Delete copy operations** when inappropriate +- **Delete copy operations** when copying is inappropriate or should be discouraged ```cpp struct Arena { @@ -313,7 +314,7 @@ Arena(Arena &&source) noexcept; ### Factory Patterns & Ownership -- **Static factory methods** for complex construction requiring shared ownership +- **Static factory methods** for complex construction requirements like enforcing shared ownership - **Friend-based factories** for access control when constructor should be private - **Ownership guidelines:** - **unique_ptr** for exclusive ownership (most common case) @@ -329,7 +330,8 @@ auto connection = Connection::createForServer(addr, fd, connection_id, handler, // Friend-based factory for access control struct Connection { - void append_message(std::string_view message_data); + WeakRef get_weak_ref() const; + private: Connection(struct sockaddr_storage client_addr, int file_descriptor, int64_t connection_id, ConnectionHandler* request_handler, @@ -382,7 +384,7 @@ ______________________________________________________________________ ### Ownership & Allocation -- **Arena allocators** for request-scoped memory with **STL allocator adapters** (see Performance Focus section for characteristics) +- **Arena** for request-scoped memory with **STL allocator adapters** - **String views** pointing to arena-allocated memory to avoid unnecessary copying - **STL containers with arena allocators require default construction after arena reset** - `clear()` is not sufficient @@ -425,7 +427,7 @@ ______________________________________________________________________ - **Error codes are the API contract** - use enums for programmatic decisions - **Error messages are human-readable only** - never parse message strings - **Consistent error boundaries** - each component defines what it can/cannot recover from -- **Interface precondition violations are undefined behavior** - acceptable to skip checks for performance in hot paths +- **Interface precondition violations are undefined behavior** - it's acceptable to skip checks for performance in hot paths - **Error code types must be nodiscard** - mark error code enums with `[[nodiscard]]` to prevent silent failures ```cpp @@ -439,7 +441,7 @@ if (!memory) { } // ... use memory, eventually std::free(memory) -// Programming error - precondition violation (may be omitted for performance) +// Programming error - precondition violation (gets compiled out in release builds) assert(ptr != nullptr && "Precondition violated: pointer must be non-null"); ``` @@ -546,7 +548,7 @@ T *realloc(T *existing_ptr, int32_t current_size, int32_t requested_size); ### Code Comments -- **Explain why, not what** - code should be self-documenting +- **Explain why, not what** - *what* the code does should be clear without any comments - **Performance notes** for optimization decisions - **Thread safety** and ownership semantics @@ -573,7 +575,7 @@ ______________________________________________________________________ ### Test Structure - **Descriptive test names** explaining the scenario -- **SUBCASE** for related test variations +- **SUBCASE** for related test variations that share setup/teardown code - **Fresh instances** for each test to avoid state contamination ```cpp @@ -600,25 +602,14 @@ TEST_CASE("Arena basic allocation") { - **Prefer fakes to mocks** - use real implementations for internal components, fake external dependencies - **Always enable assertions in tests** - use `-UNDEBUG` pattern to ensure assertions are checked (see Build Integration section) +TODO make a new example here using APIs that exist + ```cpp -// Good: Testing through public API -TEST_CASE("Server accepts connections") { - auto config = Config::defaultConfig(); - auto handler = std::make_unique(); - auto server = Server::create(config, std::move(handler)); - - // Test observable behavior - server can accept connections - auto result = connectToServer(server->getPort()); - CHECK(result.connected); -} - -// Avoid: Testing internal implementation details -// TEST_CASE("Server creates epoll instance") { /* implementation detail */ } ``` ### What NOT to Test -**Avoid testing language features and plumbing:** +**Avoid testing language features:** - Don't test that virtual functions dispatch correctly - Don't test that standard library types work (unique_ptr, containers, etc.) @@ -716,8 +707,9 @@ cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ```cmake # Test target with assertions always enabled add_executable(test_example tests/test_example.cpp src/example.cpp) -target_link_libraries(test_example doctest::doctest) +target_link_libraries(test_example doctest_impl) target_compile_options(test_example PRIVATE -UNDEBUG) # Always enable assertions +add_test(NAME test_example COMMAND test_example) # Production target follows build type add_executable(example src/example.cpp src/main.cpp) diff --git a/tests/test_reference.cpp b/tests/test_reference.cpp index cab5a26..550303b 100644 --- a/tests/test_reference.cpp +++ b/tests/test_reference.cpp @@ -28,14 +28,14 @@ struct Base { virtual int get_value() const { return base_value; } }; -struct Derived : public Base { +struct Derived : Base { int derived_value; explicit Derived(int base_v, int derived_v) : Base(base_v), derived_value(derived_v) {} int get_value() const override { return base_value + derived_value; } }; -struct AnotherDerived : public Base { +struct AnotherDerived : Base { int another_value; explicit AnotherDerived(int base_v, int another_v) : Base(base_v), another_value(another_v) {} @@ -56,7 +56,7 @@ struct Interface2 { }; // Multiple inheritance - this will cause pointer address changes -struct MultipleInheritance : public Interface1, public Interface2 { +struct MultipleInheritance : Interface1, Interface2 { int own_data; explicit MultipleInheritance(int data) : own_data(data) {} int get_own_data() const { return own_data; }