Copilot commented on code in PR #3568:
URL: https://github.com/apache/brpc/pull/3568#discussion_r4100825623
##########
test/brpc_naming_service_unittest.cpp:
##########
@@ -442,15 +449,17 @@ TEST(NamingServiceTest, consul_with_backup_file) {
brpc::Server server;
ConsulNamingServiceImpl svc;
- std::string restful_map(brpc::policy::FLAGS_consul_service_discovery_url);
- restful_map.append("/");
+ std::string restful_map("/v1/health/service/");
restful_map.append(service_name);
restful_map.append(" => ListNames");
ASSERT_EQ(0, server.AddService(&svc,
brpc::SERVER_DOESNT_OWN_SERVICE,
restful_map.c_str()));
- ASSERT_EQ(0, server.Start("localhost:8500", nullptr));
+ ASSERT_EQ(0, server.Start(0, nullptr));
+ brpc::policy::FLAGS_consul_agent_addr = butil::string_printf(
+ "http://%s";, butil::endpoint2str(server.listen_address()).c_str());
Review Comment:
This test now assigns the process-global `FLAGS_consul_agent_addr` to an
ephemeral port and leaves it set after `server` is destroyed. Any later Consul
lookup in the same process will use a closed endpoint (and the other temporary
flags in this test are likewise not exception-safe); construct a
`GFLAGS_NAMESPACE::FlagSaver` at the start of the test, before any flag
assignments, so the complete test state is restored on every exit.
This issue also appears on line 847 of the same file.
##########
test/brpc_naming_service_unittest.cpp:
##########
@@ -665,8 +673,15 @@ TEST(NamingServiceTest, discovery_sanity) {
"/discovery/cancel => Cancel";
ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
rest_mapping.c_str()));
- ASSERT_EQ(0, server.Start("localhost:8635", nullptr));
-
+ ASSERT_EQ(0, server.Start(0, nullptr));
+ brpc::policy::FLAGS_discovery_api_addr = butil::string_printf(
+ "http://%s/discovery/nodes";, butil::endpoint2str(
+ server.listen_address()).c_str());
Review Comment:
With `Start(0)`, each `--gtest_repeat` run gets a different port, but
`DiscoveryNamingService` caches its channel behind a process-wide
`pthread_once` (see `discovery_naming_service.cpp:111-135`) and never rebuilds
it. Even after the response template is fixed, the second run will call the
first run's closed server and fail; reset/recreate that channel for each run,
or keep this test isolated from the process-global cache.
##########
test/brpc_naming_service_unittest.cpp:
##########
@@ -665,8 +673,15 @@ TEST(NamingServiceTest, discovery_sanity) {
"/discovery/cancel => Cancel";
ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
rest_mapping.c_str()));
- ASSERT_EQ(0, server.Start("localhost:8635", nullptr));
-
+ ASSERT_EQ(0, server.Start(0, nullptr));
+ brpc::policy::FLAGS_discovery_api_addr = butil::string_printf(
+ "http://%s/discovery/nodes";, butil::endpoint2str(
+ server.listen_address()).c_str());
+
+ const std::string server_address =
+ butil::endpoint2str(server.listen_address()).c_str();
+ s_nodes_result.replace(s_nodes_result.find("127.0.0.1:8635"),
+ strlen("127.0.0.1:8635"), server_address);
Review Comment:
This mutates the process-global `s_nodes_result` in place and never restores
it. On a repeated test run, `find("127.0.0.1:8635")` returns `npos` because the
first run already replaced that text, so `std::string::replace` throws and
aborts the test before the request; use a per-test response value (or restore
the template with RAII) instead of modifying the static string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
