From 817ab1e403cd74dd4c67e54e49b5cb5e45311381 Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:05:03 +0200 Subject: [PATCH 1/7] refactor: drop gRPC reflection, and stand the tests up without it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reflection lets grpc_cli enumerate a running server's services. A plugin is reached only by its host, over a contract both already hold, so nothing here used it — and no shipped plugin has it: all three C++ plugin repos strip the call and the link out of this source with string replacement in their vcpkg port, because a cross-compiled gRPC does not ship grpc++_reflection. They can now drop that surgery. Removing it exposed what it had been holding up. gRPC starts a server only if some registered service has a synchronous method, and the reflection plugin was the only thing supplying one: the tests registered no service at all, so without it every server test failed with "At least one of the completion queues must be frequently polled". The tests therefore serve a service of their own, which is what a plugin does too. A shared fixture configures a server the way a host does — cookie in the environment, service registered, handshake captured — and takes the environment back down afterwards, so a test can no longer leak a cookie or a port range into the next. The connectivity check becomes the one worth making: a call placed against the advertised address is answered, rather than a socket merely accepting a connection. Co-Authored-By: Claude Opus 5 (1M context) --- src/CMakeLists.txt | 1 - src/server.cpp | 3 - tests/CMakeLists.txt | 38 ++++++- tests/plugin_fixture.hpp | 83 +++++++++++++++ tests/proto/probe.proto | 18 ++++ tests/test_handshake.cpp | 225 ++++++++++++--------------------------- tests/test_server.cpp | 160 ++++++++++------------------ 7 files changed, 259 insertions(+), 269 deletions(-) create mode 100644 tests/plugin_fixture.hpp create mode 100644 tests/proto/probe.proto diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 3c1bea6..ac4534d 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -12,7 +12,6 @@ target_include_directories(go_plugin target_link_libraries(go_plugin PUBLIC gRPC::grpc++ - gRPC::grpc++_reflection protobuf::libprotobuf OpenSSL::SSL OpenSSL::Crypto diff --git a/src/server.cpp b/src/server.cpp index 5f86038..c9f6843 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -14,13 +14,11 @@ #include #include -// POSIX socket headers for port probing #include #include #include #include -#include #include #include @@ -135,7 +133,6 @@ bool PluginServer::Start(std::string *out_error) { // ── 3. Build gRPC server ───────────────────────────────────────────── grpc::EnableDefaultHealthCheckService(true); - grpc::reflection::InitProtoReflectionServerBuilderPlugin(); grpc::ServerBuilder builder; int selected_port = 0; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6860a7b..d0ed081 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,12 +1,44 @@ find_package(GTest CONFIG REQUIRED) -# ── test_handshake (no gRPC server, purely logical tests) ───────────────────── +# ── the service the tests serve ─────────────────────────────────────────────── +# gRPC refuses to start a server with no synchronous method to answer, and a +# plugin with no services could not answer its host either, so the tests serve +# a real one. Generated into the build tree, never into the source tree. +set(PROBE_PROTO "${CMAKE_CURRENT_SOURCE_DIR}/proto/probe.proto") +set(PROBE_OUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/gen") + +file(MAKE_DIRECTORY "${PROBE_OUT_DIR}") + +add_custom_command( + OUTPUT + "${PROBE_OUT_DIR}/probe.pb.cc" "${PROBE_OUT_DIR}/probe.pb.h" + "${PROBE_OUT_DIR}/probe.grpc.pb.cc" "${PROBE_OUT_DIR}/probe.grpc.pb.h" + COMMAND protobuf::protoc + ARGS + --proto_path "${CMAKE_CURRENT_SOURCE_DIR}/proto" + --cpp_out "${PROBE_OUT_DIR}" + --grpc_out "${PROBE_OUT_DIR}" + --plugin=protoc-gen-grpc=$ + "${PROBE_PROTO}" + DEPENDS "${PROBE_PROTO}" protobuf::protoc gRPC::grpc_cpp_plugin + COMMENT "Generating the tests' Probe service" + VERBATIM +) + +add_library(probe_proto STATIC + "${PROBE_OUT_DIR}/probe.pb.cc" + "${PROBE_OUT_DIR}/probe.grpc.pb.cc" +) +target_link_libraries(probe_proto PUBLIC gRPC::grpc++ protobuf::libprotobuf) +target_include_directories(probe_proto PUBLIC "${PROBE_OUT_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}") + +# ── test_handshake (the line the host reads) ────────────────────────────────── add_executable(test_handshake test_handshake.cpp) -target_link_libraries(test_handshake PRIVATE go_plugin GTest::gtest GTest::gtest_main) +target_link_libraries(test_handshake PRIVATE go_plugin probe_proto GTest::gtest GTest::gtest_main) # ── test_server (starts a real gRPC server in-process) ─────────────────────── add_executable(test_server test_server.cpp) -target_link_libraries(test_server PRIVATE go_plugin GTest::gtest GTest::gtest_main) +target_link_libraries(test_server PRIVATE go_plugin probe_proto GTest::gtest GTest::gtest_main) include(GoogleTest) gtest_discover_tests(test_handshake) diff --git a/tests/plugin_fixture.hpp b/tests/plugin_fixture.hpp new file mode 100644 index 0000000..0301268 --- /dev/null +++ b/tests/plugin_fixture.hpp @@ -0,0 +1,83 @@ +#pragma once + +#include +#include +#include +#include +#include + +#include +#include + +#include "go_plugin/server.hpp" +#include "probe.grpc.pb.h" + +namespace go_plugin::test { + +/** Echoes its request, which is all a test needs a served method to do. */ +class ProbeService final : public Probe::Service { +public: + grpc::Status Ping(grpc::ServerContext*, const PingRequest* request, PingReply* reply) override { + reply->set_text(request->text()); + return grpc::Status::OK; + } +}; + +/** + * PluginFixture configures a server the way a host configures one — cookie in + * the environment, a service registered, the handshake captured — and takes + * the environment back down afterwards so no test can leak a cookie or a port + * range into the next. + */ +class PluginFixture : public ::testing::Test { +protected: + static constexpr const char* kCookieKey = "GO_PLUGIN_TEST_COOKIE"; + static constexpr const char* kCookieValue = "test-cookie"; + + void SetUp() override { + SetEnv(kCookieKey, kCookieValue); + config_.handshake.magic_cookie_key = kCookieKey; + config_.handshake.magic_cookie_value = kCookieValue; + config_.output = &handshake_; + config_.services = {&probe_}; + } + + void TearDown() override { + if (started_) { + server_->Shutdown(); + server_->Wait(); + } + UnsetEnv(kCookieKey); + UnsetEnv("PLUGIN_MIN_PORT"); + UnsetEnv("PLUGIN_MAX_PORT"); + } + + static void SetEnv(const char* key, const char* value) { ::setenv(key, value, 1); } + static void UnsetEnv(const char* key) { ::unsetenv(key); } + + /** Starts a server from the current config. Returns whether it came up. */ + bool Start(std::string* error = nullptr) { + std::string ignored; + server_ = std::make_unique(config_); + started_ = server_->Start(error != nullptr ? error : &ignored); + return started_; + } + + go_plugin::PluginServer& server() { return *server_; } + + /** The handshake line, newline included, as the host would read it. */ + std::string handshake() const { return handshake_.str(); } + + /** The address the handshake advertises. */ + std::string target() const { return "127.0.0.1:" + std::to_string(server_->port()); } + + go_plugin::ServeConfig config_; + +private: + ProbeService probe_; + std::ostringstream handshake_; + std::unique_ptr server_; + bool started_ = false; +}; + +} // namespace go_plugin::test diff --git a/tests/proto/probe.proto b/tests/proto/probe.proto new file mode 100644 index 0000000..cd39c14 --- /dev/null +++ b/tests/proto/probe.proto @@ -0,0 +1,18 @@ +syntax = "proto3"; + +package go_plugin.test; + +// Probe is the service the tests serve. gRPC will not start a server that has +// no synchronous method to answer, and neither could a plugin with no services, +// so a test that starts a server registers this one. +service Probe { + rpc Ping (PingRequest) returns (PingReply); +} + +message PingRequest { + string text = 1; +} + +message PingReply { + string text = 1; +} diff --git a/tests/test_handshake.cpp b/tests/test_handshake.cpp index 795bd74..6a3a9a5 100644 --- a/tests/test_handshake.cpp +++ b/tests/test_handshake.cpp @@ -1,184 +1,93 @@ -#include - -#include -#include +#include #include -#include -#include "go_plugin/server.hpp" +#include + +#include "plugin_fixture.hpp" -// ── helpers ─────────────────────────────────────────────────────────────────── +namespace go_plugin::test { +namespace { -static void SetEnv(const char *key, const char *val) { ::setenv(key, val, 1); } -static void UnsetEnv(const char *key) { ::unsetenv(key); } +using Handshake = PluginFixture; // ── magic-cookie validation ─────────────────────────────────────────────────── -TEST(HandshakeTest, FailsWithMissingCookie) { - UnsetEnv("TEST_MAGIC_MISSING"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_MAGIC_MISSING"; - cfg.handshake.magic_cookie_value = "expected"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - EXPECT_FALSE(server.Start(&err)); - EXPECT_FALSE(err.empty()); - // Nothing should have been written to the output - EXPECT_TRUE(out.str().empty()); +TEST_F(Handshake, FailsWithMissingCookie) { + UnsetEnv(kCookieKey); + + std::string error; + EXPECT_FALSE(Start(&error)); + EXPECT_FALSE(error.empty()); + EXPECT_TRUE(handshake().empty()) << "a refused plugin must advertise nothing"; } -TEST(HandshakeTest, FailsWithWrongCookieValue) { - SetEnv("TEST_MAGIC_WRONG", "bad_value"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_MAGIC_WRONG"; - cfg.handshake.magic_cookie_value = "expected_value"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - EXPECT_FALSE(server.Start(&err)); - EXPECT_FALSE(err.empty()); - EXPECT_TRUE(out.str().empty()); - UnsetEnv("TEST_MAGIC_WRONG"); +TEST_F(Handshake, FailsWithWrongCookieValue) { + SetEnv(kCookieKey, "not-the-expected-value"); + + std::string error; + EXPECT_FALSE(Start(&error)); + EXPECT_FALSE(error.empty()); + EXPECT_TRUE(handshake().empty()); } -TEST(HandshakeTest, SucceedsWithCorrectCookie) { - SetEnv("TEST_MAGIC_OK", "correct_value"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_MAGIC_OK"; - cfg.handshake.magic_cookie_value = "correct_value"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << "Start failed: " << err; - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_MAGIC_OK"); +TEST_F(Handshake, SucceedsWithCorrectCookie) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + EXPECT_FALSE(handshake().empty()); } // ── handshake line format ───────────────────────────────────────────────────── -TEST(HandshakeTest, OutputContainsCoreProtocolVersion) { - SetEnv("TEST_FORMAT_KEY", "test_value"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_FORMAT_KEY"; - cfg.handshake.magic_cookie_value = "test_value"; - cfg.handshake.protocol_version = 3; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - std::string line = out.str(); - // Must start with "1|" (core protocol version is always 1) - EXPECT_EQ(line.substr(0, 2), "1|"); - // App protocol version field must be "3" - EXPECT_NE(line.find("|3|"), std::string::npos); - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_FORMAT_KEY"); +TEST_F(Handshake, StatesTheCoreAndAppProtocolVersions) { + config_.handshake.protocol_version = 3; + + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + // The core protocol version is always 1; the app version is the plugin's. + EXPECT_EQ(handshake().substr(0, 2), "1|"); + EXPECT_NE(handshake().find("|3|"), std::string::npos) << handshake(); } -TEST(HandshakeTest, OutputContainsTcpAndGrpc) { - SetEnv("TEST_PROTO_KEY", "proto_val"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_PROTO_KEY"; - cfg.handshake.magic_cookie_value = "proto_val"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - std::string line = out.str(); - EXPECT_NE(line.find("|tcp|"), std::string::npos); - EXPECT_NE(line.find("|grpc|"), std::string::npos); - EXPECT_EQ(line.back(), '\n'); - - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_PROTO_KEY"); +TEST_F(Handshake, NamesTheTransportAndProtocol) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + EXPECT_NE(handshake().find("|tcp|"), std::string::npos) << handshake(); + EXPECT_NE(handshake().find("|grpc|"), std::string::npos) << handshake(); + EXPECT_EQ(handshake().back(), '\n') << "the host reads the line, so it has to be terminated"; } -TEST(HandshakeTest, OutputContainsListeningAddress) { - SetEnv("TEST_ADDR_KEY", "addr_val"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_ADDR_KEY"; - cfg.handshake.magic_cookie_value = "addr_val"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - EXPECT_GT(server.port(), 0); - std::string expected_addr = "127.0.0.1:" + std::to_string(server.port()); - EXPECT_NE(out.str().find(expected_addr), std::string::npos); - - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_ADDR_KEY"); +TEST_F(Handshake, AdvertisesTheListeningAddress) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + EXPECT_GT(server().port(), 0); + EXPECT_NE(handshake().find(target()), std::string::npos) << handshake(); } -TEST(HandshakeTest, HandshakeHasSixPipeFields) { - SetEnv("TEST_FIELDS_KEY", "fields_val"); - std::ostringstream out; - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_FIELDS_KEY"; - cfg.handshake.magic_cookie_value = "fields_val"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - // go-plugin expects exactly 6 pipe-separated fields (5 separators + trailing - // pipe before optional cert): CORE|APP|NET|ADDR|PROTO|CERT\n - std::string line = out.str(); - // strip trailing newline - if (!line.empty() && line.back() == '\n') - line.pop_back(); - int pipes = static_cast(std::count(line.begin(), line.end(), '|')); - EXPECT_EQ(pipes, 5); - - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_FIELDS_KEY"); +TEST_F(Handshake, HasTheSixFieldsTheHostSplitsOn) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + // CORE|APP|NET|ADDR|PROTO|CERT — six fields, so five separators. + std::string line = handshake(); + if (!line.empty() && line.back() == '\n') line.pop_back(); + EXPECT_EQ(std::count(line.begin(), line.end(), '|'), 5) << line; } // ── port range env vars ─────────────────────────────────────────────────────── -TEST(HandshakeTest, RespectsPortRange) { - SetEnv("TEST_RANGE_KEY", "range_val"); - SetEnv("PLUGIN_MIN_PORT", "19900"); - SetEnv("PLUGIN_MAX_PORT", "19999"); - std::ostringstream out; - - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "TEST_RANGE_KEY"; - cfg.handshake.magic_cookie_value = "range_val"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - EXPECT_GE(server.port(), 19900); - EXPECT_LE(server.port(), 19999); - - server.Shutdown(); - server.Wait(); - UnsetEnv("TEST_RANGE_KEY"); - UnsetEnv("PLUGIN_MIN_PORT"); - UnsetEnv("PLUGIN_MAX_PORT"); +TEST_F(Handshake, RespectsThePortRangeTheHostAsksFor) { + SetEnv("PLUGIN_MIN_PORT", "19900"); + SetEnv("PLUGIN_MAX_PORT", "19999"); + + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + EXPECT_GE(server().port(), 19900); + EXPECT_LE(server().port(), 19999); } + +} // namespace +} // namespace go_plugin::test diff --git a/tests/test_server.cpp b/tests/test_server.cpp index 1d04ab7..473cd74 100644 --- a/tests/test_server.cpp +++ b/tests/test_server.cpp @@ -1,135 +1,87 @@ -#include - #include #include -#include -#include #include #include #include +#include -#include "go_plugin/server.hpp" - -// ── helpers ─────────────────────────────────────────────────────────────────── - -static void SetEnv(const char *k, const char *v) { ::setenv(k, v, 1); } -static void UnsetEnv(const char *k) { ::unsetenv(k); } - -// ── basic server lifecycle ──────────────────────────────────────────────────── +#include "plugin_fixture.hpp" -TEST(ServerTest, StartAndShutdown) { - SetEnv("SRV_MAGIC", "srv_val"); - std::ostringstream out; +namespace go_plugin::test { +namespace { - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "SRV_MAGIC"; - cfg.handshake.magic_cookie_value = "srv_val"; - cfg.output = &out; +using Server = PluginFixture; - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - EXPECT_GT(server.port(), 0); +// ── lifecycle ───────────────────────────────────────────────────────────────── - server.Shutdown(); - server.Wait(); - UnsetEnv("SRV_MAGIC"); +TEST_F(Server, ReportsNoPortUntilStarted) { + go_plugin::PluginServer server(config_); + EXPECT_EQ(server.port(), 0); } -TEST(ServerTest, PortIsPositiveAfterStart) { - SetEnv("SRV_PORT_MAGIC", "portval"); - std::ostringstream out; - - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "SRV_PORT_MAGIC"; - cfg.handshake.magic_cookie_value = "portval"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - EXPECT_EQ(server.port(), 0); // before Start() +TEST_F(Server, StartsAndShutsDown) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + EXPECT_GT(server().port(), 0); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - EXPECT_GT(server.port(), 0); // after Start() - - server.Shutdown(); - server.Wait(); - UnsetEnv("SRV_PORT_MAGIC"); + server().Shutdown(); + server().Wait(); } -// ── connectivity check ──────────────────────────────────────────────────────── - -TEST(ServerTest, ChannelConnects) { - SetEnv("SRV_HEALTH_MAGIC", "health_val"); - std::ostringstream out; +// ── the address the handshake advertises actually serves ───────────────────── - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "SRV_HEALTH_MAGIC"; - cfg.handshake.magic_cookie_value = "health_val"; - cfg.output = &out; +// The handshake exists so the host can reach the plugin, so the test that +// matters is whether a call placed against the advertised address is answered — +// not merely whether a socket accepts a connection. +TEST_F(Server, AnswersACallOnTheAdvertisedAddress) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; + auto channel = grpc::CreateChannel(target(), grpc::InsecureChannelCredentials()); + ASSERT_TRUE(channel->WaitForConnected(std::chrono::system_clock::now() + std::chrono::seconds(5))); - std::string target = "127.0.0.1:" + std::to_string(server.port()); - auto channel = grpc::CreateChannel(target, grpc::InsecureChannelCredentials()); + auto stub = Probe::NewStub(channel); + PingRequest request; + request.set_text("hello"); - // Wait up to 5 s for the channel to reach READY state. - bool connected = channel->WaitForConnected( - std::chrono::system_clock::now() + std::chrono::seconds(5)); - EXPECT_TRUE(connected); + grpc::ClientContext context; + PingReply reply; + const grpc::Status status = stub->Ping(&context, request, &reply); - server.Shutdown(); - server.Wait(); - UnsetEnv("SRV_HEALTH_MAGIC"); + ASSERT_TRUE(status.ok()) << status.error_message(); + EXPECT_EQ(reply.text(), "hello"); } // ── Serve() convenience function ───────────────────────────────────────────── -TEST(ServerTest, ServeFailsWithWrongCookie) { - UnsetEnv("SRV_WRONG_MAGIC"); - std::ostringstream out; +TEST_F(Server, ServeRefusesTheWrongCookie) { + UnsetEnv(kCookieKey); - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "SRV_WRONG_MAGIC"; - cfg.handshake.magic_cookie_value = "expected"; - cfg.output = &out; - - // Serve() should return immediately with ok=false (no blocking Wait). - auto result = go_plugin::Serve(cfg); - EXPECT_FALSE(result.ok); - EXPECT_FALSE(result.error.empty()); + const auto result = go_plugin::Serve(config_); + EXPECT_FALSE(result.ok); + EXPECT_FALSE(result.error.empty()); } // ── concurrent shutdown ─────────────────────────────────────────────────────── -TEST(ServerTest, WaitUnblocksAfterShutdown) { - SetEnv("SRV_CONC_MAGIC", "conc_val"); - std::ostringstream out; - - go_plugin::ServeConfig cfg; - cfg.handshake.magic_cookie_key = "SRV_CONC_MAGIC"; - cfg.handshake.magic_cookie_value = "conc_val"; - cfg.output = &out; - - go_plugin::PluginServer server(cfg); - std::string err; - ASSERT_TRUE(server.Start(&err)) << err; - - std::atomic wait_returned{false}; - std::thread t([&] { - server.Wait(); - wait_returned = true; - }); - - // Wait() should be blocking now - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - EXPECT_FALSE(wait_returned); - - server.Shutdown(); - t.join(); - EXPECT_TRUE(wait_returned); - UnsetEnv("SRV_CONC_MAGIC"); +TEST_F(Server, WaitUnblocksAfterShutdown) { + std::string error; + ASSERT_TRUE(Start(&error)) << error; + + std::atomic wait_returned{false}; + std::thread waiter([&] { + server().Wait(); + wait_returned = true; + }); + + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + EXPECT_FALSE(wait_returned) << "Wait must block while the server is up"; + + server().Shutdown(); + waiter.join(); + EXPECT_TRUE(wait_returned); } + +} // namespace +} // namespace go_plugin::test From 3198421e4ccb4ffae7500ea6d39562929bebc150 Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:06:59 +0200 Subject: [PATCH 2/7] feat: write the log format the host can read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A host parses a plugin's standard error as hclog JSON and reads nothing else. A line in any other shape arrives as one opaque string at the host's own level, the plugin's severity and fields buried inside it — so a plugin error cannot surface as an error, and nothing downstream can filter on a field. Every C++ plugin logs through Abseil with the prefix switched off, which is exactly that case. go_plugin::log writes the format, and depends on nothing but the standard library so including it costs a plugin nothing. The severities and the timestamp are a contract, not a preference. Only five level names are understood, and @timestamp is parsed with a layout that demands exactly six fractional digits and an offset written as "Z" or with a colon — strftime's %z, which writes "+0200", is rejected. A rejected timestamp makes the host discard the parse and report the whole raw line at its own level, so getting it wrong looks identical to the bug this exists to fix. Both are pinned by tests, as is the escaping: a newline reaching the output unescaped would split one record into two, since the host reads standard error a line at a time. A backend adapter sits on Submit, which takes an assembled record so a library that already knows the time, severity and origin of a line does not lose them to a second timestamp. The Abseil bridge is the first, in its own target so a plugin links only the backend it uses; adding another touches nothing in the core. It maps VLOG onto debug and trace, which Abseil has no severities for, and silences Abseil's own writer so a line is not shipped twice. Co-Authored-By: Claude Opus 5 (1M context) --- CMakeLists.txt | 35 +++++- README.md | 50 ++++++++ include/go_plugin/log.hpp | 121 +++++++++++++++++++ include/go_plugin/log_absl.hpp | 38 ++++++ src/CMakeLists.txt | 19 +++ src/log.cpp | 209 +++++++++++++++++++++++++++++++++ src/log_absl.cpp | 80 +++++++++++++ tests/CMakeLists.txt | 13 ++ tests/test_log.cpp | 135 +++++++++++++++++++++ tests/test_log_absl.cpp | 76 ++++++++++++ vcpkg.json | 1 + 11 files changed, 774 insertions(+), 3 deletions(-) create mode 100644 include/go_plugin/log.hpp create mode 100644 include/go_plugin/log_absl.hpp create mode 100644 src/log.cpp create mode 100644 src/log_absl.cpp create mode 100644 tests/test_log.cpp create mode 100644 tests/test_log_absl.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 3959eea..1c57bc4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -9,16 +9,40 @@ find_package(gRPC CONFIG REQUIRED) find_package(Protobuf CONFIG REQUIRED) find_package(OpenSSL REQUIRED) +set(GO_PLUGIN_LOG_ABSL "AUTO" CACHE STRING "Build the Abseil logging bridge (ON/OFF/AUTO)") +set_property(CACHE GO_PLUGIN_LOG_ABSL PROPERTY STRINGS ON OFF AUTO) + +if(GO_PLUGIN_LOG_ABSL) + if(GO_PLUGIN_LOG_ABSL STREQUAL "AUTO") + find_package(absl CONFIG QUIET) + if(absl_FOUND) + set(GO_PLUGIN_LOG_ABSL ON) + else() + set(GO_PLUGIN_LOG_ABSL OFF) + message(STATUS "go-plugin-cpp: Abseil not found, skipping the Abseil logging bridge") + endif() + else() + find_package(absl CONFIG REQUIRED) + endif() +endif() + add_subdirectory(src) -option(GO_PLUGIN_BUILD_TESTS "Build tests" ON) +# Off unless asked for: gtest is a manifest feature, so a consumer that only +# wants the library never installs it. Build them with +# vcpkg install --x-feature=tests +# cmake -B build -DGO_PLUGIN_BUILD_TESTS=ON +option(GO_PLUGIN_BUILD_TESTS "Build tests" OFF) if(GO_PLUGIN_BUILD_TESTS) find_package(GTest CONFIG REQUIRED) enable_testing() add_subdirectory(tests) endif() -option(GO_PLUGIN_BUILD_EXAMPLES "Build examples" ON) +# Off unless asked for: the example is the only thing here that generates +# protobuf code, so with it off the library needs no protoc and no +# grpc_cpp_plugin to build. +option(GO_PLUGIN_BUILD_EXAMPLES "Build examples" OFF) if(GO_PLUGIN_BUILD_EXAMPLES) add_subdirectory(example) endif() @@ -38,7 +62,12 @@ write_basic_package_version_file( COMPATIBILITY SameMajorVersion ) -install(TARGETS go_plugin +set(GO_PLUGIN_INSTALL_TARGETS go_plugin) +if(GO_PLUGIN_LOG_ABSL) + list(APPEND GO_PLUGIN_INSTALL_TARGETS go_plugin_log_absl) +endif() + +install(TARGETS ${GO_PLUGIN_INSTALL_TARGETS} EXPORT go_plugin-targets ARCHIVE DESTINATION lib LIBRARY DESTINATION lib diff --git a/README.md b/README.md index e68f65a..a0b5829 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,56 @@ This library handles: configured automatically. - **Handshake line output** – writes the correctly formatted line to stdout so the host can connect. - **Health-check service** – the built-in gRPC health-check service is registered automatically. +- **Logging the host can read** – see below. + +## Logging + +A host parses a plugin's standard error as hclog JSON and reads nothing else. A line in any other shape reaches the +host's logs as one opaque string at the host's own level, with the plugin's severity and fields buried inside it — so +a plugin error cannot surface as an error, and nothing downstream can filter on a field. + +`go_plugin::log` writes that format. It depends on nothing but the standard library: + +```cpp +#include "go_plugin/log.hpp" + +go_plugin::log::Info("sink opened", {{"rate", 48000}, {"path", pipe_path}}); +go_plugin::log::Error("write failed", {{"error", strerror(errno)}}); +``` + +### Bridging an existing logging library + +A plugin that already logs through a library keeps its call sites and installs a bridge. Each backend is a separate +target, so a plugin links only the one it uses: + +| Backend | Target | Header | Install with | +|---------|--------|--------|--------------| +| Abseil (`LOG`/`VLOG`) | `go_plugin::go_plugin_log_absl` | `go_plugin/log_absl.hpp` | `go_plugin::log::InstallAbslBridge()` | + +```cpp +absl::InitializeLog(); +go_plugin::log::InstallAbslBridge(); // LOG(WARNING) now reaches the host as a warning +``` + +Abseil has no debug or trace severity of its own — they exist only as `VLOG` verbosities — so the bridge maps `VLOG(1)` +to debug and `VLOG(2)` and above to trace, and it stops Abseil writing its own copy of each line to standard error. + +To add another backend, translate its records into `go_plugin::log::Submit` and add a target beside the Abseil one; +nothing in the core changes. + +### Logging from a C library + +A C library that writes its own diagnostics can be routed through the same path rather than left to print unattributed +text. FFmpeg, for example, takes a callback, which lets the library's own name travel as a field instead of a pointer +address that makes every line unique: + +```cpp +av_log_set_level(AV_LOG_WARNING); +av_log_set_callback([](void *avcl, int level, const char *fmt, va_list args) { + // format into a buffer, then: + go_plugin::log::Write(LevelFor(level), text, {{"avclass", av_default_item_name(avcl)}}); +}); +``` ## Building diff --git a/include/go_plugin/log.hpp b/include/go_plugin/log.hpp new file mode 100644 index 0000000..2e153d8 --- /dev/null +++ b/include/go_plugin/log.hpp @@ -0,0 +1,121 @@ +#pragma once + +#include +#include +#include +#include +#include + +namespace go_plugin::log { + +/** + * Level is the severity a host reads off a line. These are the only five + * severities go-plugin understands; anything else leaves it unable to tell the + * level and it files the line at its own. + */ +enum class Level { Trace, Debug, Info, Warn, Error }; + +/** + * Field is one key/value pair carried by a line. Values are converted at the + * call site, so a field costs a small string and nothing else. + */ +class Field { +public: + Field(std::string_view key, std::string_view value); + Field(std::string_view key, const char* value); + Field(std::string_view key, const std::string& value); + Field(std::string_view key, bool value); + Field(std::string_view key, int value); + Field(std::string_view key, long long value); + Field(std::string_view key, unsigned long long value); + Field(std::string_view key, double value); + + const std::string& key() const { return key_; } + const std::string& value() const { return value_; } + + /** True when the value is to be written as a JSON literal rather than a string. */ + bool literal() const { return literal_; } + +private: + Field(std::string_view key, std::string value, bool literal); + + std::string key_; + std::string value_; + bool literal_ = false; +}; + +/** + * Record is one line's worth of content, handed to a Sink already assembled. + * A backend that wants to encode differently — a file, a test buffer, a second + * transport — reads the record rather than parsing the encoded line back. + */ +struct Record { + Level level = Level::Info; + std::string_view message; + const Field* fields = nullptr; + std::size_t field_count = 0; + std::chrono::system_clock::time_point timestamp; +}; + +/** + * Sink receives every record that passes the level filter. The default sink + * encodes the record for the host and writes it to standard error in a single + * write, so lines from different threads cannot interleave. + */ +using Sink = std::function; + +/** Replaces the sink. Pass nullptr to restore the default. */ +void SetSink(Sink sink); + +/** Records below this level are dropped before they are encoded. Info by default. */ +void SetLevel(Level min); +Level GetLevel(); +bool Enabled(Level level); + +/** Writes one record. Cheap and thread-safe; a dropped level costs a comparison. */ +void Write(Level level, std::string_view message, std::initializer_list fields = {}); + +/** + * Submit hands an already-assembled record to the sink, level filter included. + * This is the seam a backend adapter sits on: a logging library that already + * knows the time, severity and origin of a line reports it through here rather + * than losing them to a second timestamp. + */ +void Submit(const Record& record); + +inline void Trace(std::string_view message, std::initializer_list fields = {}) { + Write(Level::Trace, message, fields); +} +inline void Debug(std::string_view message, std::initializer_list fields = {}) { + Write(Level::Debug, message, fields); +} +inline void Info(std::string_view message, std::initializer_list fields = {}) { + Write(Level::Info, message, fields); +} +inline void Warn(std::string_view message, std::initializer_list fields = {}) { + Write(Level::Warn, message, fields); +} +inline void Error(std::string_view message, std::initializer_list fields = {}) { + Write(Level::Error, message, fields); +} + +/** + * Encode renders a record in the format the host parses, without the trailing + * newline. Public so a backend can reuse the encoding, and so it can be tested + * directly. + */ +std::string Encode(const Record& record); + +/** + * FormatTimestamp renders a time the way the host's parser demands: exactly six + * fractional digits, and an offset written either as "Z" or with a colon. A + * timestamp in any other shape makes the host reject the whole line and fall + * back to reporting it as unparsed text at its own level, so this is a contract + * and not a preference. + */ +std::string FormatTimestamp(std::chrono::system_clock::time_point tp); + +/** The level's name as the host spells it. */ +std::string_view LevelName(Level level); + +} // namespace go_plugin::log diff --git a/include/go_plugin/log_absl.hpp b/include/go_plugin/log_absl.hpp new file mode 100644 index 0000000..1ac7072 --- /dev/null +++ b/include/go_plugin/log_absl.hpp @@ -0,0 +1,38 @@ +#pragma once + +namespace go_plugin::log { + +/** + * AbslBridgeOptions tunes how Abseil's severities are mapped onto the five the + * host understands. Abseil has no debug or trace severity of its own — those + * only exist as VLOG verbosities — so the mapping has to be stated somewhere. + */ +struct AbslBridgeOptions { + /** VLOG(n) at or above this verbosity is reported as trace rather than debug. */ + int trace_from_verbosity = 2; + + /** + * Carry the source file and line of each line as a `caller` field. Abseil + * knows them and the host has nowhere else to learn them. + */ + bool include_caller = true; + + /** + * Stop Abseil writing its own copy of every line to standard error. Left + * on, each line reaches the host twice: once as unparsed text at the host's + * level, once in the format it can read. + */ + bool silence_absl_stderr = true; +}; + +/** + * InstallAbslBridge routes Abseil's LOG() and VLOG() through the format the + * host parses, so a plugin keeps its own severity and gains structured fields + * without touching a single call site. + * + * Safe to call more than once; only the first call installs a sink. Call it + * after absl::InitializeLog(). + */ +void InstallAbslBridge(const AbslBridgeOptions& options = {}); + +} // namespace go_plugin::log diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index ac4534d..4eca5b1 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,4 +1,5 @@ add_library(go_plugin STATIC + log.cpp server.cpp tls.cpp ) @@ -17,6 +18,24 @@ target_link_libraries(go_plugin OpenSSL::Crypto ) +# One backend adapter per logging library, each its own target, so a consumer +# links only the one it uses and adding another touches nothing here. +if(GO_PLUGIN_LOG_ABSL) + add_library(go_plugin_log_absl STATIC log_absl.cpp) + target_link_libraries(go_plugin_log_absl + PUBLIC + go_plugin + absl::log + absl::log_sink + absl::log_sink_registry + absl::log_entry + absl::log_globals + absl::log_severity + absl::time + ) + target_compile_options(go_plugin_log_absl PRIVATE -Wall -Wextra -Wpedantic) +endif() + target_compile_options(go_plugin PRIVATE -Wall -Wextra -Wpedantic ) diff --git a/src/log.cpp b/src/log.cpp new file mode 100644 index 0000000..9152f04 --- /dev/null +++ b/src/log.cpp @@ -0,0 +1,209 @@ +#include "go_plugin/log.hpp" + +#include +#include +#include +#include +#include +#include +#include + +namespace go_plugin::log { +namespace { + +std::atomic g_level{static_cast(Level::Info)}; + +std::mutex& SinkMutex() { + static std::mutex m; + return m; +} + +std::mutex& WriteMutex() { + static std::mutex m; + return m; +} + +Sink& CurrentSink() { + static Sink sink; + return sink; +} + +/** Appends s as a JSON string body, escaping what would break the parse. */ +void AppendEscaped(std::string& out, std::string_view s) { + for (unsigned char c : s) { + switch (c) { + case '"': out += "\\\""; break; + case '\\': out += "\\\\"; break; + case '\n': out += "\\n"; break; + case '\r': out += "\\r"; break; + case '\t': out += "\\t"; break; + default: + if (c < 0x20) { + char buf[7]; + std::snprintf(buf, sizeof buf, "\\u%04x", c); + out += buf; + } else { + out += static_cast(c); + } + } + } +} + +void WriteToStderr(const Record& record) { + std::string line = Encode(record); + line += '\n'; + + // One write of the whole line: the host reads standard error a line at a + // time, and a plugin logs from its gRPC threads and from whatever library + // it drives, so a line assembled in pieces would interleave with another's. + std::lock_guard lock(WriteMutex()); + ssize_t written = 0; + while (written < static_cast(line.size())) { + ssize_t n = ::write(STDERR_FILENO, line.data() + written, line.size() - written); + if (n <= 0) { + if (n < 0 && errno == EINTR) continue; + return; + } + written += n; + } +} + +} // namespace + +Field::Field(std::string_view key, std::string value, bool literal) + : key_(key), value_(std::move(value)), literal_(literal) {} + +Field::Field(std::string_view key, std::string_view value) : Field(key, std::string(value), false) {} +Field::Field(std::string_view key, const char* value) + : Field(key, std::string(value ? value : ""), false) {} +Field::Field(std::string_view key, const std::string& value) : Field(key, value, false) {} +Field::Field(std::string_view key, bool value) : Field(key, value ? "true" : "false", true) {} +Field::Field(std::string_view key, int value) : Field(key, std::to_string(value), true) {} +Field::Field(std::string_view key, long long value) : Field(key, std::to_string(value), true) {} +Field::Field(std::string_view key, unsigned long long value) + : Field(key, std::to_string(value), true) {} + +Field::Field(std::string_view key, double value) : Field(key, std::string(), true) { + char buf[32]; + std::snprintf(buf, sizeof buf, "%.17g", value); + value_ = buf; +} + +std::string_view LevelName(Level level) { + switch (level) { + case Level::Trace: return "trace"; + case Level::Debug: return "debug"; + case Level::Info: return "info"; + case Level::Warn: return "warn"; + case Level::Error: return "error"; + } + return "info"; +} + +std::string FormatTimestamp(std::chrono::system_clock::time_point tp) { + using namespace std::chrono; + + // floor, not a cast: a cast truncates towards zero, which would hand back a + // negative sub-second remainder for a clock still set before the epoch. + const auto secs = floor(tp); + const auto micros = duration_cast(tp - secs).count(); + + const std::time_t t = system_clock::to_time_t(secs); + std::tm tm{}; + if (localtime_r(&t, &tm) == nullptr) { + gmtime_r(&t, &tm); + } + + char stamp[32]; + std::strftime(stamp, sizeof stamp, "%Y-%m-%dT%H:%M:%S", &tm); + + char frac[16]; + std::snprintf(frac, sizeof frac, ".%06lld", static_cast(micros)); + + std::string out = stamp; + out += frac; + + // "Z" or "+hh:mm" — strftime's %z writes "+0200", which the host rejects. + const long offset = tm.tm_gmtoff; + if (offset == 0) { + out += 'Z'; + } else { + const long abs_offset = offset < 0 ? -offset : offset; + const int hours = static_cast((abs_offset / 3600) % 100); + const int minutes = static_cast((abs_offset % 3600) / 60); + char zone[8]; + std::snprintf(zone, sizeof zone, "%c%02d:%02d", offset < 0 ? '-' : '+', hours, minutes); + out += zone; + } + return out; +} + +std::string Encode(const Record& record) { + std::string out; + out.reserve(128 + record.message.size()); + + out += "{\"@level\":\""; + out += LevelName(record.level); + out += "\",\"@message\":\""; + AppendEscaped(out, record.message); + out += "\",\"@timestamp\":\""; + out += FormatTimestamp(record.timestamp); + out += '"'; + + for (std::size_t i = 0; i < record.field_count; ++i) { + const Field& field = record.fields[i]; + out += ",\""; + AppendEscaped(out, field.key()); + out += "\":"; + if (field.literal()) { + out += field.value(); + } else { + out += '"'; + AppendEscaped(out, field.value()); + out += '"'; + } + } + + out += '}'; + return out; +} + +void SetSink(Sink sink) { + std::lock_guard lock(SinkMutex()); + CurrentSink() = std::move(sink); +} + +void SetLevel(Level min) { g_level.store(static_cast(min), std::memory_order_relaxed); } + +Level GetLevel() { return static_cast(g_level.load(std::memory_order_relaxed)); } + +bool Enabled(Level level) { return static_cast(level) >= g_level.load(std::memory_order_relaxed); } + +void Submit(const Record& record) { + if (!Enabled(record.level)) return; + + Sink sink; + { + std::lock_guard lock(SinkMutex()); + sink = CurrentSink(); + } + if (sink) { + sink(record); + } else { + WriteToStderr(record); + } +} + +void Write(Level level, std::string_view message, std::initializer_list fields) { + if (!Enabled(level)) return; + + Record record; + record.level = level; + record.message = message; + record.fields = fields.begin(); + record.field_count = fields.size(); + record.timestamp = std::chrono::system_clock::now(); + Submit(record); +} + +} // namespace go_plugin::log diff --git a/src/log_absl.cpp b/src/log_absl.cpp new file mode 100644 index 0000000..f84d9e8 --- /dev/null +++ b/src/log_absl.cpp @@ -0,0 +1,80 @@ +#include "go_plugin/log_absl.hpp" + +#include + +#include "absl/base/log_severity.h" +#include "absl/log/globals.h" +#include "absl/log/log_entry.h" +#include "absl/log/log_sink.h" +#include "absl/log/log_sink_registry.h" +#include "absl/time/time.h" + +#include "go_plugin/log.hpp" + +namespace go_plugin::log { +namespace { + +Level LevelFor(const absl::LogEntry& entry, const AbslBridgeOptions& options) { + // A verbose entry is a VLOG, which Abseil files at info severity. Its + // verbosity is the only thing that separates it from an ordinary info line. + if (entry.verbosity() != absl::LogEntry::kNoVerbosityLevel) { + return entry.verbosity() >= options.trace_from_verbosity ? Level::Trace : Level::Debug; + } + + switch (entry.log_severity()) { + case absl::LogSeverity::kInfo: return Level::Info; + case absl::LogSeverity::kWarning: return Level::Warn; + case absl::LogSeverity::kError: return Level::Error; + case absl::LogSeverity::kFatal: return Level::Error; + } + return Level::Info; +} + +class Bridge final : public absl::LogSink { +public: + explicit Bridge(const AbslBridgeOptions& options) : options_(options) {} + + void Send(const absl::LogEntry& entry) override { + Record record; + record.level = LevelFor(entry, options_); + // text_message() carries no Abseil prefix, which is what belongs in the + // record: the severity and the time travel as fields of their own. + record.message = entry.text_message(); + record.timestamp = absl::ToChronoTime(entry.timestamp()); + + if (!options_.include_caller) { + Submit(record); + return; + } + + std::string caller(entry.source_basename()); + caller += ':'; + caller += std::to_string(entry.source_line()); + const Field field("caller", caller); + record.fields = &field; + record.field_count = 1; + Submit(record); + } + +private: + AbslBridgeOptions options_; +}; + +} // namespace + +void InstallAbslBridge(const AbslBridgeOptions& options) { + static bool installed = false; + if (installed) return; + installed = true; + + // Never destroyed: Abseil holds the pointer for the life of the process and + // a LOG(FATAL) unwinds nothing. + static Bridge bridge(options); + absl::AddLogSink(&bridge); + + if (options.silence_absl_stderr) { + absl::SetStderrThreshold(absl::LogSeverityAtLeast::kInfinity); + } +} + +} // namespace go_plugin::log diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d0ed081..145eea3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -40,6 +40,19 @@ target_link_libraries(test_handshake PRIVATE go_plugin probe_proto GTest::gtest add_executable(test_server test_server.cpp) target_link_libraries(test_server PRIVATE go_plugin probe_proto GTest::gtest GTest::gtest_main) +# ── test_log (encoding and the host's timestamp contract) ──────────────────── +add_executable(test_log test_log.cpp) +target_link_libraries(test_log PRIVATE go_plugin GTest::gtest GTest::gtest_main) + +if(GO_PLUGIN_LOG_ABSL) + add_executable(test_log_absl test_log_absl.cpp) + target_link_libraries(test_log_absl PRIVATE go_plugin_log_absl GTest::gtest GTest::gtest_main) +endif() + include(GoogleTest) gtest_discover_tests(test_handshake) gtest_discover_tests(test_server) +gtest_discover_tests(test_log) +if(GO_PLUGIN_LOG_ABSL) + gtest_discover_tests(test_log_absl) +endif() diff --git a/tests/test_log.cpp b/tests/test_log.cpp new file mode 100644 index 0000000..0d495b0 --- /dev/null +++ b/tests/test_log.cpp @@ -0,0 +1,135 @@ +#include +#include +#include +#include + +#include + +#include "go_plugin/log.hpp" + +using go_plugin::log::Field; +using go_plugin::log::Level; +using go_plugin::log::Record; + +namespace { + +// Capture holds every record a test produces, and restores the default sink so +// one test cannot leak its sink into the next. +class Capture { +public: + Capture() { + go_plugin::log::SetSink([this](const Record& record) { lines_.push_back(go_plugin::log::Encode(record)); }); + } + ~Capture() { go_plugin::log::SetSink(nullptr); } + + const std::vector& lines() const { return lines_; } + +private: + std::vector lines_; +}; + +} // namespace + +// The host parses @timestamp with Go's "2006-01-02T15:04:05.000000Z07:00", +// which demands exactly six fractional digits and an offset written as "Z" or +// with a colon. Anything else and the host discards the parse and reports the +// whole raw line at its own level — the failure is silent and looks exactly +// like the bug this format exists to fix. +TEST(Log, TimestampMatchesTheHostsLayout) { + const std::regex layout(R"(^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{6}(Z|[+-]\d{2}:\d{2})$)"); + + for (auto tp : {std::chrono::system_clock::now(), + std::chrono::system_clock::time_point{}, + std::chrono::system_clock::now() + std::chrono::hours(24 * 365)}) { + const std::string stamp = go_plugin::log::FormatTimestamp(tp); + EXPECT_TRUE(std::regex_match(stamp, layout)) << "timestamp not in the host's layout: " << stamp; + } +} + +TEST(Log, TimestampKeepsSixFractionalDigits) { + // A whole second must still render six digits rather than being trimmed. + const auto whole = std::chrono::time_point_cast(std::chrono::system_clock::now()); + const std::string stamp = go_plugin::log::FormatTimestamp(whole); + EXPECT_NE(stamp.find(".000000"), std::string::npos) << stamp; +} + +TEST(Log, CarriesTheHostsKeys) { + Capture capture; + go_plugin::log::Info("agent registered", {Field("capability", "NoInputNoOutput")}); + + ASSERT_EQ(capture.lines().size(), 1u); + const std::string& line = capture.lines().front(); + EXPECT_NE(line.find(R"("@level":"info")"), std::string::npos) << line; + EXPECT_NE(line.find(R"("@message":"agent registered")"), std::string::npos) << line; + EXPECT_NE(line.find(R"("@timestamp":")"), std::string::npos) << line; + EXPECT_NE(line.find(R"("capability":"NoInputNoOutput")"), std::string::npos) << line; +} + +TEST(Log, LevelNamesAreTheOnesTheHostAccepts) { + Capture capture; + go_plugin::log::SetLevel(Level::Trace); + go_plugin::log::Trace("t"); + go_plugin::log::Debug("d"); + go_plugin::log::Info("i"); + go_plugin::log::Warn("w"); + go_plugin::log::Error("e"); + go_plugin::log::SetLevel(Level::Info); + + ASSERT_EQ(capture.lines().size(), 5u); + const char* expected[] = {"trace", "debug", "info", "warn", "error"}; + for (std::size_t i = 0; i < 5; ++i) { + const std::string want = std::string(R"("@level":")") + expected[i] + R"(")"; + EXPECT_NE(capture.lines()[i].find(want), std::string::npos) << capture.lines()[i]; + } +} + +// The host reads standard error one line at a time, so a newline that reached +// the output unescaped would split one record into two and leave the remainder +// unparseable. +TEST(Log, EscapesWhatWouldBreakTheLine) { + Capture capture; + go_plugin::log::Info("first\nsecond\ttabbed \"quoted\" back\\slash", + {Field("ctl", std::string_view("\x01", 1))}); + + ASSERT_EQ(capture.lines().size(), 1u); + const std::string& line = capture.lines().front(); + EXPECT_EQ(line.find('\n'), std::string::npos) << "an unescaped newline splits the record: " << line; + EXPECT_NE(line.find("first\\nsecond"), std::string::npos) << line; + EXPECT_NE(line.find("\\t"), std::string::npos) << line; + EXPECT_NE(line.find("\\\"quoted\\\""), std::string::npos) << line; + EXPECT_NE(line.find("back\\\\slash"), std::string::npos) << line; + EXPECT_NE(line.find("\\u0001"), std::string::npos) << line; +} + +TEST(Log, NumbersAndBoolsAreJsonLiterals) { + Capture capture; + go_plugin::log::Info("readings", {Field("count", 7), Field("ok", true), Field("ratio", 0.5)}); + + const std::string& line = capture.lines().front(); + EXPECT_NE(line.find(R"("count":7)"), std::string::npos) << line; + EXPECT_NE(line.find(R"("ok":true)"), std::string::npos) << line; + EXPECT_NE(line.find(R"("ratio":0.5)"), std::string::npos) << line; +} + +// The shape the README hands a caller: fields written inline, no Field spelled +// out and no vector built. +TEST(Log, TakesFieldsAsBracedPairs) { + Capture capture; + go_plugin::log::Info("sink opened", {{"rate", 48000}, {"path", std::string("/tmp/pipe")}}); + + ASSERT_EQ(capture.lines().size(), 1u); + const std::string& line = capture.lines().front(); + EXPECT_NE(line.find(R"("rate":48000)"), std::string::npos) << line; + EXPECT_NE(line.find(R"("path":"/tmp/pipe")"), std::string::npos) << line; +} + +TEST(Log, DropsRecordsBelowTheLevel) { + Capture capture; + go_plugin::log::SetLevel(Level::Warn); + go_plugin::log::Info("dropped"); + go_plugin::log::Error("kept"); + go_plugin::log::SetLevel(Level::Info); + + ASSERT_EQ(capture.lines().size(), 1u); + EXPECT_NE(capture.lines().front().find("kept"), std::string::npos); +} diff --git a/tests/test_log_absl.cpp b/tests/test_log_absl.cpp new file mode 100644 index 0000000..8c6983b --- /dev/null +++ b/tests/test_log_absl.cpp @@ -0,0 +1,76 @@ +#include +#include + +#include + +#include "absl/log/globals.h" +#include "absl/log/initialize.h" +#include "absl/log/log.h" + +#include "go_plugin/log.hpp" +#include "go_plugin/log_absl.hpp" + +using go_plugin::log::Record; + +namespace { + +std::vector& Lines() { + static std::vector lines; + return lines; +} + +class AbslBridge : public ::testing::Test { +protected: + static void SetUpTestSuite() { + absl::InitializeLog(); + go_plugin::log::InstallAbslBridge(); + go_plugin::log::SetSink([](const Record& record) { Lines().push_back(go_plugin::log::Encode(record)); }); + } + + void SetUp() override { + Lines().clear(); + go_plugin::log::SetLevel(go_plugin::log::Level::Trace); + absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); + absl::SetGlobalVLogLevel(4); + } +}; + +} // namespace + +// Abseil files everything it emits at info or above; a plugin's own severity is +// exactly what was being lost, so this is the point of the bridge. +TEST_F(AbslBridge, KeepsTheSeverity) { + LOG(INFO) << "an info line"; + LOG(WARNING) << "a warning line"; + LOG(ERROR) << "an error line"; + + ASSERT_EQ(Lines().size(), 3u); + EXPECT_NE(Lines()[0].find(R"("@level":"info")"), std::string::npos) << Lines()[0]; + EXPECT_NE(Lines()[1].find(R"("@level":"warn")"), std::string::npos) << Lines()[1]; + EXPECT_NE(Lines()[2].find(R"("@level":"error")"), std::string::npos) << Lines()[2]; +} + +// Abseil has no debug or trace severity: they exist only as VLOG verbosities, +// so the bridge is the only thing that can tell them apart. +TEST_F(AbslBridge, MapsVerbosityOntoDebugAndTrace) { + VLOG(1) << "a verbose line"; + VLOG(3) << "a very verbose line"; + + ASSERT_EQ(Lines().size(), 2u); + EXPECT_NE(Lines()[0].find(R"("@level":"debug")"), std::string::npos) << Lines()[0]; + EXPECT_NE(Lines()[1].find(R"("@level":"trace")"), std::string::npos) << Lines()[1]; +} + +TEST_F(AbslBridge, CarriesTheMessageWithoutAbseilsPrefix) { + LOG(INFO) << "plain message"; + + ASSERT_EQ(Lines().size(), 1u); + EXPECT_NE(Lines()[0].find(R"("@message":"plain message")"), std::string::npos) << Lines()[0]; +} + +TEST_F(AbslBridge, CarriesTheCaller) { + LOG(INFO) << "located"; + + ASSERT_EQ(Lines().size(), 1u); + EXPECT_NE(Lines()[0].find(R"("caller":"test_log_absl.cpp:)"), std::string::npos) << Lines()[0]; +} diff --git a/vcpkg.json b/vcpkg.json index 2eec772..689a075 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -2,6 +2,7 @@ "name": "go-plugin-cpp", "version": "0.1.1", "dependencies": [ + "abseil", "grpc", "openssl", { From 43a76e003f8286a9ad56c39105eea0a4144dd3c0 Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:09:04 +0200 Subject: [PATCH 3/7] build: stop installing gtest for the host, and make tests opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gtest was declared a host dependency, which is what vcpkg means by "a tool the build runs", not "a library the build links". These tests are target binaries that link it, so cross-compiling built gtest for the host where nothing could use it, and a target build of the tests could not have found it there at all. It is a manifest feature now, so a consumer installing the library never installs gtest. Tests and the example follow the same rule and are off unless asked for: the example is the only thing here that generates protobuf code, so with it off the library builds without protoc or grpc_cpp_plugin. CI and the README ask for all three explicitly. What this does not fix is the larger half. The grpc port depends on itself for the host with its codegen feature, and on host protobuf, so cross-compiling builds gRPC twice however this manifest is written — a consumer cannot decline it. Only a warm vcpkg binary cache makes that cost once rather than every build. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 3 +++ README.md | 13 +++++++++++++ vcpkg.json | 14 +++++++++----- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 79289aa..6059281 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,9 @@ jobs: cmake -B build \ -DCMAKE_TOOLCHAIN_FILE="$VCPKG_INSTALLATION_ROOT/scripts/buildsystems/vcpkg.cmake" \ -DCMAKE_BUILD_TYPE=Release \ + -DVCPKG_MANIFEST_FEATURES=tests \ + -DGO_PLUGIN_BUILD_TESTS=ON \ + -DGO_PLUGIN_BUILD_EXAMPLES=ON \ -G Ninja - name: Build diff --git a/README.md b/README.md index a0b5829..47ef45e 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,19 @@ cmake -B build \ cmake --build build -j ``` +That builds the library alone. The tests and the example are opt-in, so a consumer installs neither gtest nor the +protobuf code generators they need: + +```bash +cmake -B build \ + -DCMAKE_TOOLCHAIN_FILE="$VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake" \ + -DCMAKE_BUILD_TYPE=Release \ + -DVCPKG_MANIFEST_FEATURES=tests \ + -DGO_PLUGIN_BUILD_TESTS=ON \ + -DGO_PLUGIN_BUILD_EXAMPLES=ON +cmake --build build -j +``` + ### Running the tests ```bash diff --git a/vcpkg.json b/vcpkg.json index 689a075..ec0bd54 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -4,10 +4,14 @@ "dependencies": [ "abseil", "grpc", - "openssl", - { - "name": "gtest", - "host": true + "openssl" + ], + "features": { + "tests": { + "description": "Build the test suite", + "dependencies": [ + "gtest" + ] } - ] + } } From 246fb88a82f30443dba3db077ae46d104eb6708c Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:20:18 +0200 Subject: [PATCH 4/7] chore: 0.2.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logging module adds to the surface and dropping gRPC reflection takes from it, so this is not a patch release. The two version numbers had also drifted apart — the manifest said 0.1.1 while the CMake project, which is what a consumer's find_package compares against, still said 0.1.0. Both now say the same thing. Co-Authored-By: Claude Opus 5 (1M context) --- CMakeLists.txt | 2 +- vcpkg.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c57bc4..9acdd50 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,5 +1,5 @@ cmake_minimum_required(VERSION 3.20) -project(go-plugin-cpp VERSION 0.1.0 LANGUAGES CXX) +project(go-plugin-cpp VERSION 0.2.0 LANGUAGES CXX) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) diff --git a/vcpkg.json b/vcpkg.json index ec0bd54..df48bd6 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -1,6 +1,6 @@ { "name": "go-plugin-cpp", - "version": "0.1.1", + "version": "0.2.0", "dependencies": [ "abseil", "grpc", From 612950175ca5476a364cc8ea2d580ff5b9fc0d8b Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:24:56 +0200 Subject: [PATCH 5/7] fix: let a backend's record through whatever level is set here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A record reaching Submit comes from a library that has already decided to emit it, under its own thresholds. Filtering it again against this module's level dropped lines a plugin meant to say: Abseil emits a VLOG because its own verbosity allows it, the bridge reports it as debug, and the default level here — info — silently threw it away. That is the failure this whole format exists to prevent, arriving through the fix. SetLevel now governs Write, the direct API, and nothing else. A backend gates with its own knobs, which is where its users look. Co-Authored-By: Claude Opus 5 (1M context) --- include/go_plugin/log.hpp | 15 ++++++++++----- src/log.cpp | 2 -- tests/test_log.cpp | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/include/go_plugin/log.hpp b/include/go_plugin/log.hpp index 2e153d8..bfba52d 100644 --- a/include/go_plugin/log.hpp +++ b/include/go_plugin/log.hpp @@ -67,7 +67,7 @@ using Sink = std::function; /** Replaces the sink. Pass nullptr to restore the default. */ void SetSink(Sink sink); -/** Records below this level are dropped before they are encoded. Info by default. */ +/** Records written through Write below this level are dropped. Info by default. */ void SetLevel(Level min); Level GetLevel(); bool Enabled(Level level); @@ -76,10 +76,15 @@ bool Enabled(Level level); void Write(Level level, std::string_view message, std::initializer_list fields = {}); /** - * Submit hands an already-assembled record to the sink, level filter included. - * This is the seam a backend adapter sits on: a logging library that already - * knows the time, severity and origin of a line reports it through here rather - * than losing them to a second timestamp. + * Submit hands an already-assembled record to the sink. This is the seam a + * backend adapter sits on: a logging library that already knows the time, + * severity and origin of a line reports it through here rather than losing them + * to a second timestamp. + * + * The level set here is deliberately not applied. A record reaching Submit came + * from a library that has already decided to emit it, under its own thresholds, + * and dropping it a second time here would lose exactly what a plugin meant to + * say — which is the whole reason this format exists. SetLevel governs Write. */ void Submit(const Record& record); diff --git a/src/log.cpp b/src/log.cpp index 9152f04..8a4e925 100644 --- a/src/log.cpp +++ b/src/log.cpp @@ -180,8 +180,6 @@ Level GetLevel() { return static_cast(g_level.load(std::memory_order_rela bool Enabled(Level level) { return static_cast(level) >= g_level.load(std::memory_order_relaxed); } void Submit(const Record& record) { - if (!Enabled(record.level)) return; - Sink sink; { std::lock_guard lock(SinkMutex()); diff --git a/tests/test_log.cpp b/tests/test_log.cpp index 0d495b0..565c1ec 100644 --- a/tests/test_log.cpp +++ b/tests/test_log.cpp @@ -123,6 +123,25 @@ TEST(Log, TakesFieldsAsBracedPairs) { EXPECT_NE(line.find(R"("path":"/tmp/pipe")"), std::string::npos) << line; } +// A backend hands over records its own library already decided to emit. Gating +// them again here would silently drop what a plugin meant to say, which is the +// failure this format exists to prevent. +TEST(Log, DoesNotRegateARecordFromABackend) { + Capture capture; + go_plugin::log::SetLevel(Level::Error); + + Record record; + record.level = Level::Debug; + record.message = "a backend already let this through"; + record.timestamp = std::chrono::system_clock::now(); + go_plugin::log::Submit(record); + + go_plugin::log::SetLevel(Level::Info); + + ASSERT_EQ(capture.lines().size(), 1u); + EXPECT_NE(capture.lines().front().find(R"("@level":"debug")"), std::string::npos); +} + TEST(Log, DropsRecordsBelowTheLevel) { Capture capture; go_plugin::log::SetLevel(Level::Warn); From d68f563c6e08f24c3d70ca45eeda4c7f68bf1593 Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:37:25 +0200 Subject: [PATCH 6/7] fix: address review, and trim the comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all of them real. A double was formatted with printf's %g, which follows the locale: under a comma-decimal locale it wrote "0,5", and a NaN wrote "nan". Both are invalid JSON, so the host rejects the line and reports it as raw text — the silent failure this format exists to avoid. Doubles now format locale-independently, and a non-finite one travels as a string, since JSON has no literal for it. The installed package did not declare Abseil, so its exported target named absl:: libraries that a consumer's find_package(go_plugin) had no reason to have defined. It declares them now, when built with the bridge. That bridge is an explicit option rather than an auto-detected one. Abseil arrives with gRPC either way, so detection only made the installed package's dependencies vary with what happened to be present when it was built. Installing the bridge is guarded with call_once rather than a plain static bool, and the test that calls the plugin over gRPC now sets a deadline, so a server that stops answering fails the test instead of hanging it. The comments went back over too. The doc blocks mostly restated the names and signatures beneath them, and the banners between test groups said nothing the test names did not. What is left is the traps: the timestamp layout, %z, %g and the locale, the deliberate leak, floor over a cast, and why Submit does not filter. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 1 + CMakeLists.txt | 27 ++++----------- README.md | 7 ++-- cmake/go_plugin-config.cmake.in | 7 ++++ include/go_plugin/log.hpp | 61 ++++++++++----------------------- include/go_plugin/log_absl.hpp | 30 +++++++--------- src/CMakeLists.txt | 3 +- src/log.cpp | 28 ++++++++++----- src/log_absl.cpp | 32 ++++++++--------- tests/CMakeLists.txt | 8 +---- tests/plugin_fixture.hpp | 15 +++----- tests/test_handshake.cpp | 6 ---- tests/test_log.cpp | 48 +++++++++++++++++--------- tests/test_log_absl.cpp | 6 ++-- tests/test_server.cpp | 13 ++----- 15 files changed, 126 insertions(+), 166 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6059281..3351c86 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,7 @@ jobs: -DCMAKE_TOOLCHAIN_FILE="$VCPKG_INSTALLATION_ROOT/scripts/buildsystems/vcpkg.cmake" \ -DCMAKE_BUILD_TYPE=Release \ -DVCPKG_MANIFEST_FEATURES=tests \ + -DGO_PLUGIN_LOG_ABSL=ON \ -DGO_PLUGIN_BUILD_TESTS=ON \ -DGO_PLUGIN_BUILD_EXAMPLES=ON \ -G Ninja diff --git a/CMakeLists.txt b/CMakeLists.txt index 9acdd50..d76196a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -9,29 +9,17 @@ find_package(gRPC CONFIG REQUIRED) find_package(Protobuf CONFIG REQUIRED) find_package(OpenSSL REQUIRED) -set(GO_PLUGIN_LOG_ABSL "AUTO" CACHE STRING "Build the Abseil logging bridge (ON/OFF/AUTO)") -set_property(CACHE GO_PLUGIN_LOG_ABSL PROPERTY STRINGS ON OFF AUTO) +# Explicit, not detected: the installed package's dependencies should not vary +# with whatever happened to be present when it was built. +option(GO_PLUGIN_LOG_ABSL "Build the Abseil logging bridge" OFF) if(GO_PLUGIN_LOG_ABSL) - if(GO_PLUGIN_LOG_ABSL STREQUAL "AUTO") - find_package(absl CONFIG QUIET) - if(absl_FOUND) - set(GO_PLUGIN_LOG_ABSL ON) - else() - set(GO_PLUGIN_LOG_ABSL OFF) - message(STATUS "go-plugin-cpp: Abseil not found, skipping the Abseil logging bridge") - endif() - else() - find_package(absl CONFIG REQUIRED) - endif() + find_package(absl CONFIG REQUIRED) endif() add_subdirectory(src) -# Off unless asked for: gtest is a manifest feature, so a consumer that only -# wants the library never installs it. Build them with -# vcpkg install --x-feature=tests -# cmake -B build -DGO_PLUGIN_BUILD_TESTS=ON +# gtest is a manifest feature, so a consumer of the library never installs it. option(GO_PLUGIN_BUILD_TESTS "Build tests" OFF) if(GO_PLUGIN_BUILD_TESTS) find_package(GTest CONFIG REQUIRED) @@ -39,9 +27,8 @@ if(GO_PLUGIN_BUILD_TESTS) add_subdirectory(tests) endif() -# Off unless asked for: the example is the only thing here that generates -# protobuf code, so with it off the library needs no protoc and no -# grpc_cpp_plugin to build. +# The only thing here that generates protobuf code, so with it off the library +# needs neither protoc nor grpc_cpp_plugin. option(GO_PLUGIN_BUILD_EXAMPLES "Build examples" OFF) if(GO_PLUGIN_BUILD_EXAMPLES) add_subdirectory(example) diff --git a/README.md b/README.md index 47ef45e..1bf3a83 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,9 @@ go_plugin::log::Error("write failed", {{"error", strerror(errno)}}); A plugin that already logs through a library keeps its call sites and installs a bridge. Each backend is a separate target, so a plugin links only the one it uses: -| Backend | Target | Header | Install with | -|---------|--------|--------|--------------| -| Abseil (`LOG`/`VLOG`) | `go_plugin::go_plugin_log_absl` | `go_plugin/log_absl.hpp` | `go_plugin::log::InstallAbslBridge()` | +| Backend | Target | Header | Build with | Install with | +|---------|--------|--------|------------|--------------| +| Abseil (`LOG`/`VLOG`) | `go_plugin::go_plugin_log_absl` | `go_plugin/log_absl.hpp` | `-DGO_PLUGIN_LOG_ABSL=ON` | `go_plugin::log::InstallAbslBridge()` | ```cpp absl::InitializeLog(); @@ -95,6 +95,7 @@ cmake -B build \ -DCMAKE_TOOLCHAIN_FILE="$VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake" \ -DCMAKE_BUILD_TYPE=Release \ -DVCPKG_MANIFEST_FEATURES=tests \ + -DGO_PLUGIN_LOG_ABSL=ON \ -DGO_PLUGIN_BUILD_TESTS=ON \ -DGO_PLUGIN_BUILD_EXAMPLES=ON cmake --build build -j diff --git a/cmake/go_plugin-config.cmake.in b/cmake/go_plugin-config.cmake.in index 20a7f22..0b71a72 100644 --- a/cmake/go_plugin-config.cmake.in +++ b/cmake/go_plugin-config.cmake.in @@ -5,6 +5,13 @@ find_dependency(gRPC CONFIG REQUIRED) find_dependency(OpenSSL REQUIRED) find_dependency(Protobuf CONFIG REQUIRED) +# The Abseil bridge's exported target names absl:: libraries, so they have to +# exist by the time the export is read — a consumer that never links the bridge +# would otherwise fail on find_package(go_plugin) alone. +if(@GO_PLUGIN_LOG_ABSL@) + find_dependency(absl CONFIG REQUIRED) +endif() + include("${CMAKE_CURRENT_LIST_DIR}/go_plugin-targets.cmake") check_required_components(go_plugin) diff --git a/include/go_plugin/log.hpp b/include/go_plugin/log.hpp index bfba52d..75b7493 100644 --- a/include/go_plugin/log.hpp +++ b/include/go_plugin/log.hpp @@ -9,16 +9,11 @@ namespace go_plugin::log { /** - * Level is the severity a host reads off a line. These are the only five - * severities go-plugin understands; anything else leaves it unable to tell the - * level and it files the line at its own. + * The only five severities go-plugin understands. Anything else leaves the host + * unable to tell the level and it files the line at its own. */ enum class Level { Trace, Debug, Info, Warn, Error }; -/** - * Field is one key/value pair carried by a line. Values are converted at the - * call site, so a field costs a small string and nothing else. - */ class Field { public: Field(std::string_view key, std::string_view value); @@ -33,7 +28,7 @@ class Field { const std::string& key() const { return key_; } const std::string& value() const { return value_; } - /** True when the value is to be written as a JSON literal rather than a string. */ + /** Whether the value is written as a JSON literal rather than a string. */ bool literal() const { return literal_; } private: @@ -44,11 +39,6 @@ class Field { bool literal_ = false; }; -/** - * Record is one line's worth of content, handed to a Sink already assembled. - * A backend that wants to encode differently — a file, a test buffer, a second - * transport — reads the record rather than parsing the encoded line back. - */ struct Record { Level level = Level::Info; std::string_view message; @@ -57,37 +47,19 @@ struct Record { std::chrono::system_clock::time_point timestamp; }; -/** - * Sink receives every record that passes the level filter. The default sink - * encodes the record for the host and writes it to standard error in a single - * write, so lines from different threads cannot interleave. - */ +/** Receives every record. The default encodes it and writes it to stderr. */ using Sink = std::function; -/** Replaces the sink. Pass nullptr to restore the default. */ +/** Pass nullptr to restore the default. */ void SetSink(Sink sink); -/** Records written through Write below this level are dropped. Info by default. */ +/** Governs Write only, not Submit. Info by default. */ void SetLevel(Level min); Level GetLevel(); bool Enabled(Level level); -/** Writes one record. Cheap and thread-safe; a dropped level costs a comparison. */ void Write(Level level, std::string_view message, std::initializer_list fields = {}); -/** - * Submit hands an already-assembled record to the sink. This is the seam a - * backend adapter sits on: a logging library that already knows the time, - * severity and origin of a line reports it through here rather than losing them - * to a second timestamp. - * - * The level set here is deliberately not applied. A record reaching Submit came - * from a library that has already decided to emit it, under its own thresholds, - * and dropping it a second time here would lose exactly what a plugin meant to - * say — which is the whole reason this format exists. SetLevel governs Write. - */ -void Submit(const Record& record); - inline void Trace(std::string_view message, std::initializer_list fields = {}) { Write(Level::Trace, message, fields); } @@ -105,22 +77,25 @@ inline void Error(std::string_view message, std::initializer_list fields } /** - * Encode renders a record in the format the host parses, without the trailing - * newline. Public so a backend can reuse the encoding, and so it can be tested - * directly. + * The seam a backend adapter sits on, so a library that already knows a line's + * time, severity and origin does not lose them to a second timestamp. + * + * SetLevel is deliberately not applied: the record comes from a library that + * has already decided to emit it, and dropping it again here would lose what a + * plugin meant to say. */ +void Submit(const Record& record); + +/** Renders a record in the host's format, without the trailing newline. */ std::string Encode(const Record& record); /** - * FormatTimestamp renders a time the way the host's parser demands: exactly six - * fractional digits, and an offset written either as "Z" or with a colon. A - * timestamp in any other shape makes the host reject the whole line and fall - * back to reporting it as unparsed text at its own level, so this is a contract - * and not a preference. + * Exactly six fractional digits, and an offset written either as "Z" or with a + * colon. A timestamp in any other shape makes the host reject the whole line + * and report it as unparsed text at its own level. */ std::string FormatTimestamp(std::chrono::system_clock::time_point tp); -/** The level's name as the host spells it. */ std::string_view LevelName(Level level); } // namespace go_plugin::log diff --git a/include/go_plugin/log_absl.hpp b/include/go_plugin/log_absl.hpp index 1ac7072..75e7fa7 100644 --- a/include/go_plugin/log_absl.hpp +++ b/include/go_plugin/log_absl.hpp @@ -2,36 +2,30 @@ namespace go_plugin::log { -/** - * AbslBridgeOptions tunes how Abseil's severities are mapped onto the five the - * host understands. Abseil has no debug or trace severity of its own — those - * only exist as VLOG verbosities — so the mapping has to be stated somewhere. - */ struct AbslBridgeOptions { - /** VLOG(n) at or above this verbosity is reported as trace rather than debug. */ - int trace_from_verbosity = 2; - /** - * Carry the source file and line of each line as a `caller` field. Abseil - * knows them and the host has nowhere else to learn them. + * Abseil has no debug or trace severity of its own — they exist only as + * VLOG verbosities — so the mapping has to be stated. VLOG at or above this + * verbosity is reported as trace, below it as debug. */ + int trace_from_verbosity = 2; + + /** Carry Abseil's source file and line as a `caller` field. */ bool include_caller = true; /** - * Stop Abseil writing its own copy of every line to standard error. Left - * on, each line reaches the host twice: once as unparsed text at the host's - * level, once in the format it can read. + * Stop Abseil writing its own copy of every line. Left on, each line + * reaches the host twice: once as unparsed text, once in the format it can + * read. */ bool silence_absl_stderr = true; }; /** - * InstallAbslBridge routes Abseil's LOG() and VLOG() through the format the - * host parses, so a plugin keeps its own severity and gains structured fields - * without touching a single call site. + * Routes Abseil's LOG() and VLOG() through the format the host parses, so a + * plugin keeps its own severity without touching a call site. * - * Safe to call more than once; only the first call installs a sink. Call it - * after absl::InitializeLog(). + * Call after absl::InitializeLog(). Only the first call installs a sink. */ void InstallAbslBridge(const AbslBridgeOptions& options = {}); diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 4eca5b1..56c3a20 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -18,8 +18,7 @@ target_link_libraries(go_plugin OpenSSL::Crypto ) -# One backend adapter per logging library, each its own target, so a consumer -# links only the one it uses and adding another touches nothing here. +# One target per backend, so a consumer links only the one it uses. if(GO_PLUGIN_LOG_ABSL) add_library(go_plugin_log_absl STATIC log_absl.cpp) target_link_libraries(go_plugin_log_absl diff --git a/src/log.cpp b/src/log.cpp index 8a4e925..e7163db 100644 --- a/src/log.cpp +++ b/src/log.cpp @@ -3,8 +3,12 @@ #include #include #include +#include #include #include +#include +#include +#include #include #include @@ -28,7 +32,6 @@ Sink& CurrentSink() { return sink; } -/** Appends s as a JSON string body, escaping what would break the parse. */ void AppendEscaped(std::string& out, std::string_view s) { for (unsigned char c : s) { switch (c) { @@ -53,9 +56,8 @@ void WriteToStderr(const Record& record) { std::string line = Encode(record); line += '\n'; - // One write of the whole line: the host reads standard error a line at a - // time, and a plugin logs from its gRPC threads and from whatever library - // it drives, so a line assembled in pieces would interleave with another's. + // One write of the whole line: a plugin logs from its gRPC threads and from + // whatever library it drives, and the host reads a line at a time. std::lock_guard lock(WriteMutex()); ssize_t written = 0; while (written < static_cast(line.size())) { @@ -84,9 +86,17 @@ Field::Field(std::string_view key, unsigned long long value) : Field(key, std::to_string(value), true) {} Field::Field(std::string_view key, double value) : Field(key, std::string(), true) { - char buf[32]; - std::snprintf(buf, sizeof buf, "%.17g", value); - value_ = buf; + // printf would write "0,5" under a comma-decimal locale, and "nan" for a + // NaN — either makes the host reject the line. + if (!std::isfinite(value)) { + value_ = std::isnan(value) ? "\"NaN\"" : (value > 0 ? "\"+Inf\"" : "\"-Inf\""); + return; + } + + std::ostringstream out; + out.imbue(std::locale::classic()); + out << std::setprecision(17) << value; + value_ = out.str(); } std::string_view LevelName(Level level) { @@ -103,8 +113,8 @@ std::string_view LevelName(Level level) { std::string FormatTimestamp(std::chrono::system_clock::time_point tp) { using namespace std::chrono; - // floor, not a cast: a cast truncates towards zero, which would hand back a - // negative sub-second remainder for a clock still set before the epoch. + // floor, not a cast: a cast truncates towards zero, handing back a negative + // remainder for a clock still set before the epoch. const auto secs = floor(tp); const auto micros = duration_cast(tp - secs).count(); diff --git a/src/log_absl.cpp b/src/log_absl.cpp index f84d9e8..827f3d7 100644 --- a/src/log_absl.cpp +++ b/src/log_absl.cpp @@ -1,5 +1,6 @@ #include "go_plugin/log_absl.hpp" +#include #include #include "absl/base/log_severity.h" @@ -15,8 +16,8 @@ namespace go_plugin::log { namespace { Level LevelFor(const absl::LogEntry& entry, const AbslBridgeOptions& options) { - // A verbose entry is a VLOG, which Abseil files at info severity. Its - // verbosity is the only thing that separates it from an ordinary info line. + // A VLOG is filed at info severity; its verbosity is the only thing that + // separates it from an ordinary info line. if (entry.verbosity() != absl::LogEntry::kNoVerbosityLevel) { return entry.verbosity() >= options.trace_from_verbosity ? Level::Trace : Level::Debug; } @@ -37,9 +38,7 @@ class Bridge final : public absl::LogSink { void Send(const absl::LogEntry& entry) override { Record record; record.level = LevelFor(entry, options_); - // text_message() carries no Abseil prefix, which is what belongs in the - // record: the severity and the time travel as fields of their own. - record.message = entry.text_message(); + record.message = entry.text_message(); // prefix-free record.timestamp = absl::ToChronoTime(entry.timestamp()); if (!options_.include_caller) { @@ -63,18 +62,17 @@ class Bridge final : public absl::LogSink { } // namespace void InstallAbslBridge(const AbslBridgeOptions& options) { - static bool installed = false; - if (installed) return; - installed = true; - - // Never destroyed: Abseil holds the pointer for the life of the process and - // a LOG(FATAL) unwinds nothing. - static Bridge bridge(options); - absl::AddLogSink(&bridge); - - if (options.silence_absl_stderr) { - absl::SetStderrThreshold(absl::LogSeverityAtLeast::kInfinity); - } + static std::once_flag once; + std::call_once(once, [&options] { + // Never destroyed: Abseil holds the pointer for the life of the process + // and a LOG(FATAL) unwinds nothing. + static Bridge bridge(options); + absl::AddLogSink(&bridge); + + if (options.silence_absl_stderr) { + absl::SetStderrThreshold(absl::LogSeverityAtLeast::kInfinity); + } + }); } } // namespace go_plugin::log diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 145eea3..a1b6e0a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,9 +1,6 @@ find_package(GTest CONFIG REQUIRED) -# ── the service the tests serve ─────────────────────────────────────────────── -# gRPC refuses to start a server with no synchronous method to answer, and a -# plugin with no services could not answer its host either, so the tests serve -# a real one. Generated into the build tree, never into the source tree. +# Generated into the build tree, never into the source tree. set(PROBE_PROTO "${CMAKE_CURRENT_SOURCE_DIR}/proto/probe.proto") set(PROBE_OUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/gen") @@ -32,15 +29,12 @@ add_library(probe_proto STATIC target_link_libraries(probe_proto PUBLIC gRPC::grpc++ protobuf::libprotobuf) target_include_directories(probe_proto PUBLIC "${PROBE_OUT_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}") -# ── test_handshake (the line the host reads) ────────────────────────────────── add_executable(test_handshake test_handshake.cpp) target_link_libraries(test_handshake PRIVATE go_plugin probe_proto GTest::gtest GTest::gtest_main) -# ── test_server (starts a real gRPC server in-process) ─────────────────────── add_executable(test_server test_server.cpp) target_link_libraries(test_server PRIVATE go_plugin probe_proto GTest::gtest GTest::gtest_main) -# ── test_log (encoding and the host's timestamp contract) ──────────────────── add_executable(test_log test_log.cpp) target_link_libraries(test_log PRIVATE go_plugin GTest::gtest GTest::gtest_main) diff --git a/tests/plugin_fixture.hpp b/tests/plugin_fixture.hpp index 0301268..674a9d1 100644 --- a/tests/plugin_fixture.hpp +++ b/tests/plugin_fixture.hpp @@ -14,7 +14,10 @@ namespace go_plugin::test { -/** Echoes its request, which is all a test needs a served method to do. */ +/** + * gRPC starts a server only if a registered service has a synchronous method, + * and a plugin with no services could not answer its host either. + */ class ProbeService final : public Probe::Service { public: grpc::Status Ping(grpc::ServerContext*, const PingRequest* request, PingReply* reply) override { @@ -23,12 +26,7 @@ class ProbeService final : public Probe::Service { } }; -/** - * PluginFixture configures a server the way a host configures one — cookie in - * the environment, a service registered, the handshake captured — and takes - * the environment back down afterwards so no test can leak a cookie or a port - * range into the next. - */ +/** Takes the environment back down so no test leaks a cookie into the next. */ class PluginFixture : public ::testing::Test { protected: static constexpr const char* kCookieKey = "GO_PLUGIN_TEST_COOKIE"; @@ -55,7 +53,6 @@ class PluginFixture : public ::testing::Test { static void SetEnv(const char* key, const char* value) { ::setenv(key, value, 1); } static void UnsetEnv(const char* key) { ::unsetenv(key); } - /** Starts a server from the current config. Returns whether it came up. */ bool Start(std::string* error = nullptr) { std::string ignored; server_ = std::make_unique(config_); @@ -65,10 +62,8 @@ class PluginFixture : public ::testing::Test { go_plugin::PluginServer& server() { return *server_; } - /** The handshake line, newline included, as the host would read it. */ std::string handshake() const { return handshake_.str(); } - /** The address the handshake advertises. */ std::string target() const { return "127.0.0.1:" + std::to_string(server_->port()); } go_plugin::ServeConfig config_; diff --git a/tests/test_handshake.cpp b/tests/test_handshake.cpp index 6a3a9a5..063bde5 100644 --- a/tests/test_handshake.cpp +++ b/tests/test_handshake.cpp @@ -10,8 +10,6 @@ namespace { using Handshake = PluginFixture; -// ── magic-cookie validation ─────────────────────────────────────────────────── - TEST_F(Handshake, FailsWithMissingCookie) { UnsetEnv(kCookieKey); @@ -36,8 +34,6 @@ TEST_F(Handshake, SucceedsWithCorrectCookie) { EXPECT_FALSE(handshake().empty()); } -// ── handshake line format ───────────────────────────────────────────────────── - TEST_F(Handshake, StatesTheCoreAndAppProtocolVersions) { config_.handshake.protocol_version = 3; @@ -76,8 +72,6 @@ TEST_F(Handshake, HasTheSixFieldsTheHostSplitsOn) { EXPECT_EQ(std::count(line.begin(), line.end(), '|'), 5) << line; } -// ── port range env vars ─────────────────────────────────────────────────────── - TEST_F(Handshake, RespectsThePortRangeTheHostAsksFor) { SetEnv("PLUGIN_MIN_PORT", "19900"); SetEnv("PLUGIN_MAX_PORT", "19999"); diff --git a/tests/test_log.cpp b/tests/test_log.cpp index 565c1ec..4d4381c 100644 --- a/tests/test_log.cpp +++ b/tests/test_log.cpp @@ -1,4 +1,6 @@ #include +#include +#include #include #include #include @@ -13,8 +15,6 @@ using go_plugin::log::Record; namespace { -// Capture holds every record a test produces, and restores the default sink so -// one test cannot leak its sink into the next. class Capture { public: Capture() { @@ -30,11 +30,8 @@ class Capture { } // namespace -// The host parses @timestamp with Go's "2006-01-02T15:04:05.000000Z07:00", -// which demands exactly six fractional digits and an offset written as "Z" or -// with a colon. Anything else and the host discards the parse and reports the -// whole raw line at its own level — the failure is silent and looks exactly -// like the bug this format exists to fix. +// A shape the host cannot parse makes it discard the parse and report the raw +// line at its own level, so the failure looks like the bug this format fixes. TEST(Log, TimestampMatchesTheHostsLayout) { const std::regex layout(R"(^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{6}(Z|[+-]\d{2}:\d{2})$)"); @@ -47,7 +44,6 @@ TEST(Log, TimestampMatchesTheHostsLayout) { } TEST(Log, TimestampKeepsSixFractionalDigits) { - // A whole second must still render six digits rather than being trimmed. const auto whole = std::chrono::time_point_cast(std::chrono::system_clock::now()); const std::string stamp = go_plugin::log::FormatTimestamp(whole); EXPECT_NE(stamp.find(".000000"), std::string::npos) << stamp; @@ -83,9 +79,8 @@ TEST(Log, LevelNamesAreTheOnesTheHostAccepts) { } } -// The host reads standard error one line at a time, so a newline that reached -// the output unescaped would split one record into two and leave the remainder -// unparseable. +// The host reads a line at a time, so an unescaped newline would split one +// record into two. TEST(Log, EscapesWhatWouldBreakTheLine) { Capture capture; go_plugin::log::Info("first\nsecond\ttabbed \"quoted\" back\\slash", @@ -111,8 +106,6 @@ TEST(Log, NumbersAndBoolsAreJsonLiterals) { EXPECT_NE(line.find(R"("ratio":0.5)"), std::string::npos) << line; } -// The shape the README hands a caller: fields written inline, no Field spelled -// out and no vector built. TEST(Log, TakesFieldsAsBracedPairs) { Capture capture; go_plugin::log::Info("sink opened", {{"rate", 48000}, {"path", std::string("/tmp/pipe")}}); @@ -123,9 +116,32 @@ TEST(Log, TakesFieldsAsBracedPairs) { EXPECT_NE(line.find(R"("path":"/tmp/pipe")"), std::string::npos) << line; } -// A backend hands over records its own library already decided to emit. Gating -// them again here would silently drop what a plugin meant to say, which is the -// failure this format exists to prevent. + +// printf's %g follows the locale, which would write "0,5" and be rejected. +TEST(Log, WritesDoublesTheSameInAnyLocale) { + Capture capture; + const char* previous = std::setlocale(LC_NUMERIC, "de_DE.UTF-8"); + go_plugin::log::Info("readings", {Field("ratio", 0.5)}); + if (previous != nullptr) std::setlocale(LC_NUMERIC, previous); + + ASSERT_EQ(capture.lines().size(), 1u); + EXPECT_NE(capture.lines().front().find(R"("ratio":0.5)"), std::string::npos) + << capture.lines().front(); +} + +// JSON has no NaN or infinity. +TEST(Log, QuotesNonFiniteDoubles) { + Capture capture; + go_plugin::log::Info("a", {Field("v", std::numeric_limits::quiet_NaN())}); + go_plugin::log::Info("b", {Field("v", std::numeric_limits::infinity())}); + + ASSERT_EQ(capture.lines().size(), 2u); + EXPECT_NE(capture.lines()[0].find(R"("v":"NaN")"), std::string::npos) << capture.lines()[0]; + EXPECT_NE(capture.lines()[1].find(R"("v":"+Inf")"), std::string::npos) << capture.lines()[1]; +} + +// The backend's library already decided to emit these; gating them again would +// drop what a plugin meant to say. TEST(Log, DoesNotRegateARecordFromABackend) { Capture capture; go_plugin::log::SetLevel(Level::Error); diff --git a/tests/test_log_absl.cpp b/tests/test_log_absl.cpp index 8c6983b..6d15411 100644 --- a/tests/test_log_absl.cpp +++ b/tests/test_log_absl.cpp @@ -37,8 +37,7 @@ class AbslBridge : public ::testing::Test { } // namespace -// Abseil files everything it emits at info or above; a plugin's own severity is -// exactly what was being lost, so this is the point of the bridge. +// A plugin's own severity is what was being lost, so this is the point. TEST_F(AbslBridge, KeepsTheSeverity) { LOG(INFO) << "an info line"; LOG(WARNING) << "a warning line"; @@ -50,8 +49,7 @@ TEST_F(AbslBridge, KeepsTheSeverity) { EXPECT_NE(Lines()[2].find(R"("@level":"error")"), std::string::npos) << Lines()[2]; } -// Abseil has no debug or trace severity: they exist only as VLOG verbosities, -// so the bridge is the only thing that can tell them apart. +// Abseil has no debug or trace severity; only VLOG verbosities. TEST_F(AbslBridge, MapsVerbosityOntoDebugAndTrace) { VLOG(1) << "a verbose line"; VLOG(3) << "a very verbose line"; diff --git a/tests/test_server.cpp b/tests/test_server.cpp index 473cd74..6316282 100644 --- a/tests/test_server.cpp +++ b/tests/test_server.cpp @@ -13,8 +13,6 @@ namespace { using Server = PluginFixture; -// ── lifecycle ───────────────────────────────────────────────────────────────── - TEST_F(Server, ReportsNoPortUntilStarted) { go_plugin::PluginServer server(config_); EXPECT_EQ(server.port(), 0); @@ -29,11 +27,7 @@ TEST_F(Server, StartsAndShutsDown) { server().Wait(); } -// ── the address the handshake advertises actually serves ───────────────────── - -// The handshake exists so the host can reach the plugin, so the test that -// matters is whether a call placed against the advertised address is answered — -// not merely whether a socket accepts a connection. +// A socket accepting a connection is not the same as the plugin answering. TEST_F(Server, AnswersACallOnTheAdvertisedAddress) { std::string error; ASSERT_TRUE(Start(&error)) << error; @@ -46,6 +40,7 @@ TEST_F(Server, AnswersACallOnTheAdvertisedAddress) { request.set_text("hello"); grpc::ClientContext context; + context.set_deadline(std::chrono::system_clock::now() + std::chrono::seconds(5)); PingReply reply; const grpc::Status status = stub->Ping(&context, request, &reply); @@ -53,8 +48,6 @@ TEST_F(Server, AnswersACallOnTheAdvertisedAddress) { EXPECT_EQ(reply.text(), "hello"); } -// ── Serve() convenience function ───────────────────────────────────────────── - TEST_F(Server, ServeRefusesTheWrongCookie) { UnsetEnv(kCookieKey); @@ -63,8 +56,6 @@ TEST_F(Server, ServeRefusesTheWrongCookie) { EXPECT_FALSE(result.error.empty()); } -// ── concurrent shutdown ─────────────────────────────────────────────────────── - TEST_F(Server, WaitUnblocksAfterShutdown) { std::string error; ASSERT_TRUE(Start(&error)) << error; From b2eb3f97d8fa4ba8227ca6237445fbf9d6bc8dc4 Mon Sep 17 00:00:00 2001 From: devgianlu Date: Thu, 10 Sep 2026 14:51:49 +0200 Subject: [PATCH 7/7] test: stop the locale test leaking its locale, or passing without one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setlocale returns the locale it has just set, not the one it replaced, so restoring its return value left LC_NUMERIC comma-decimal for every test that ran after it in this binary. Read the locale to restore before changing it, and assert at the end that it came back. And skip rather than assert when de_DE.UTF-8 is not installed. Without it setlocale fails, the decimal point never changes, and the test goes green having exercised nothing — which is the shape of failure it exists to catch. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_log.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/test_log.cpp b/tests/test_log.cpp index 4d4381c..8e3718e 100644 --- a/tests/test_log.cpp +++ b/tests/test_log.cpp @@ -119,14 +119,23 @@ TEST(Log, TakesFieldsAsBracedPairs) { // printf's %g follows the locale, which would write "0,5" and be rejected. TEST(Log, WritesDoublesTheSameInAnyLocale) { + // setlocale returns the locale it just set, so the one to restore has to be + // read first. Without a comma-decimal locale installed there is nothing to + // test against, and asserting anyway would pass without exercising it. + const std::string original = std::setlocale(LC_NUMERIC, nullptr); + if (std::setlocale(LC_NUMERIC, "de_DE.UTF-8") == nullptr) { + GTEST_SKIP() << "no de_DE.UTF-8 locale to test against"; + } + Capture capture; - const char* previous = std::setlocale(LC_NUMERIC, "de_DE.UTF-8"); go_plugin::log::Info("readings", {Field("ratio", 0.5)}); - if (previous != nullptr) std::setlocale(LC_NUMERIC, previous); + std::setlocale(LC_NUMERIC, original.c_str()); ASSERT_EQ(capture.lines().size(), 1u); EXPECT_NE(capture.lines().front().find(R"("ratio":0.5)"), std::string::npos) << capture.lines().front(); + EXPECT_EQ(std::string(std::setlocale(LC_NUMERIC, nullptr)), original) + << "a leaked locale would follow this test into the next"; } // JSON has no NaN or infinity.