Addressing comments

eclipse-uprotocol · Jul 31, 2024 · 82aa07b · 82aa07b
1 parent e71a74b
commit 82aa07b
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 38 deletions.
diff --git a/test_agent/cpp/CMakeLists.txt b/test_agent/cpp/CMakeLists.txt
@@ -18,17 +18,12 @@ find_package(up-cpp REQUIRED)
 find_package(spdlog REQUIRED)
 find_package(fmt REQUIRED CONFIG)
 find_package(RapidJSON CONFIG REQUIRED)
-# find_package(up-client-zenoh-cpp REQUIRED)
 find_package(up_client_socket REQUIRED)
 find_package(OpenSSL REQUIRED)
-# TODO:: Remove GTest
-find_package(GTest REQUIRED)
-include(GoogleTest)
-# TODO NEEDED?
-#add_definitions(-DSPDLOG_FMT_EXTERNAL)
+# TODO: Update when zenoh transport is ready
+# find_package(up-client-zenoh-cpp REQUIRED)
 
 # This is the root CMakeLists.txt file; We can set project wide settings here
-# TODO: Is this needed?
 if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
 	set(CMAKE_CXX_STANDARD 17)
 	# place libraries in a lib directory and executables in a bin directory,

diff --git a/test_agent/cpp/README.md b/test_agent/cpp/README.md
@@ -25,7 +25,7 @@ conan create --version 1.6.0 --build=missing up-core-api/release
 ### up-cpp
 
 ```
-conan create --version 1.0.0 --build=missing up-cpp/release/
+conan create --version 1.0.1-rc1 --build=missing up-cpp/release/
 ```
 
 ### up_client_socket

diff --git a/test_agent/cpp/conanfile.txt b/test_agent/cpp/conanfile.txt
@@ -8,9 +8,6 @@ spdlog/[>=1.13.0]
 fmt/10.2.1
 openssl/1.1.1w
 
-[test_requires]
-gtest/1.14.0
-
 [generators]
 CMakeDeps
 CMakeToolchain

diff --git a/test_agent/cpp/src/APIWrapper.cpp b/test_agent/cpp/src/APIWrapper.cpp
@@ -121,11 +121,10 @@ UStatus APIWrapper::removeHandleOrProvideError(const UUri& uri) {
 	if (count == 0) {
 		spdlog::error("APIWrapper::removeCallbackToMap, URI not found.");
 		status.set_code(UCode::NOT_FOUND);
-		return status;
+	} else {
+		status.set_code(UCode::OK);
 	}
 
-	status.set_code(UCode::OK);
-
 	return status;
 }
 
@@ -175,6 +174,7 @@ UStatus APIWrapper::handleInvokeMethodCommand(Document& jsonData) {
 	std::string strTest_id = jsonData[Constants::TEST_ID].GetString();
 	UStatus status;
 
+	// Build attributes
 	auto attributes = ProtoConverter::distToAttributes(
 	    data[Constants::ATTRIBUTES], jsonData.GetAllocator());
 	auto uri = attributes.sink();
@@ -271,16 +271,16 @@ UStatus APIWrapper::handleRpcServerCommand(Document& jsonData) {
 	        uriCallbackMap_, serializedUri)) {
 		status.set_code(UCode::ALREADY_EXISTS);
 		status.set_message("RPC Server already exists for the given URI.");
-		return status;
-	}
-
-	// Create RPC Client object
-	auto rpcServer = uprotocol::communication::RpcServer::create(
-	    transportPtr_, uri, rpcServerCallback, format);
+	} else {
+		// Create RPC Client object
+		auto rpcServer = uprotocol::communication::RpcServer::create(
+		    transportPtr_, uri, rpcServerCallback, format);
 
-	return rpcServer.has_value()
-	           ? addHandleToUriCallbackMap(std::move(rpcServer).value(), uri)
-	           : rpcServer.error();
+		status = rpcServer.has_value() ? addHandleToUriCallbackMap(
+		                                     std::move(rpcServer).value(), uri)
+		                               : rpcServer.error();
+	}
+	return status;
 }
 
 UStatus APIWrapper::handlePublisherCommand(Document& jsonData) {
@@ -415,15 +415,17 @@ UStatus APIWrapper::handleNotificationSinkCommand(Document& jsonData) {
 		status.set_message(
 		    "Notification Sink already exists for the given URI.");
 		return status;
+	} else {
+		// Create NotificationSink object
+		auto NotificationSinkHandle =
+		    uprotocol::communication::NotificationSink::create(
+		        transportPtr_, std::move(callback), std::move(uri));
+
+		status = NotificationSinkHandle.has_value()
+		             ? addHandleToUriCallbackMap(
+		                   std::move(NotificationSinkHandle).value(), uri)
+		             : NotificationSinkHandle.error();
 	}
 
-	// Create NotificationSink object
-	auto NotificationSinkHandle =
-	    uprotocol::communication::NotificationSink::create(
-	        transportPtr_, std::move(callback), std::move(uri));
-
-	return NotificationSinkHandle.has_value()
-	           ? addHandleToUriCallbackMap(
-	                 std::move(NotificationSinkHandle).value(), uri)
-	           : NotificationSinkHandle.error();
+	return status;
 }
diff --git a/test_agent/cpp/src/ProtoConverter.cpp b/test_agent/cpp/src/ProtoConverter.cpp
@@ -82,6 +82,8 @@ void ProtoConverter::processNested(Value& parentJsonObj,
 			    base64Encoded.c_str(),
 			    static_cast<rapidjson::SizeType>(base64Encoded.length()),
 			    allocator);
+		} else {
+			spdlog::debug("No BYTES: prefix in data");
 		}
 	}
 }
@@ -101,7 +103,9 @@ void ProtoConverter::dictToProto(Value& parentJsonObj, Message& parentProtoObj,
 	parentJsonObj.Accept(writer);
 	std::string strBuf = buffer.GetString();
 
-	google::protobuf::util::JsonParseOptions options;
+	google::protobuf::util::JsonParseOptions options =
+	    google::protobuf::util::JsonParseOptions();
+
 	auto status = google::protobuf::util::JsonStringToMessage(
 	    strBuf, &parentProtoObj, options);
 	if (!status.ok()) {
@@ -128,17 +132,21 @@ uprotocol::v1::UUri ProtoConverter::distToUri(
 
 std::optional<uprotocol::v1::UPayloadFormat> ProtoConverter::distToUPayFormat(
     const rapidjson::Value& formatStrValue) {
+	std::optional<uprotocol::v1::UPayloadFormat> format = std::nullopt;
+
 	if (formatStrValue.IsString()) {
 		const std::string formatStr = formatStrValue.GetString();
 		const google::protobuf::EnumDescriptor* descriptor =
 		    uprotocol::v1::UPayloadFormat_descriptor();
 		const google::protobuf::EnumValueDescriptor* value =
 		    descriptor->FindValueByName(formatStr);
 		if (value) {
-			return static_cast<uprotocol::v1::UPayloadFormat>(value->number());
+			format =
+			    static_cast<uprotocol::v1::UPayloadFormat>(value->number());
 		}
 	}
-	return std::nullopt;
+
+	return format;
 }
 
 Value ProtoConverter::convertMessageToJson(

diff --git a/test_agent/cpp/src/TestAgent.cpp b/test_agent/cpp/src/TestAgent.cpp
@@ -353,7 +353,7 @@ void TestAgent::socketDisconnect() {
 
 int main(int argc, char* argv[]) {
 	// Uncomment this line to set log level to debug
-	spdlog::set_level(spdlog::level::level_enum::debug);
+	//spdlog::set_level(spdlog::level::level_enum::debug);
 
 	// Log the start of the Test Agent
 	spdlog::info(" *** Starting CPP Test Agent *** ");
@@ -371,10 +371,10 @@ int main(int argc, char* argv[]) {
 
 	// Iterate over command line arguments to find the transport type
 	for (auto it = args.begin(); it != args.end(); ++it) {
-		if (*it == "--transport" && (it + 1) != args.end()) {
+		if ((*it == "--transport") && ((it + 1) != args.end())) {
 			transportType = *(it + 1);
 			it++;
-		} else if (*it == "--sdkname" && (it + 1) != args.end()) {
+		} else if ((*it == "--sdkname") && ((it + 1) != args.end())) {
 			sdkNameValue = *(it + 1);
 			it++;
 		}