New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Porting C++ Test Agent to 1.0.1-RC1 #90

Merged

gregmedd merged 8 commits into eclipse-uprotocol:main from agosh01:port_cpp_TA_rc1

Aug 13, 2024

+1,144 −923

Member

agosh01 commented Jul 28, 2024 •

edited

Loading

This PR update c++ test agent to work with 1.6.0 core api with addition on L2 apis (closes #59 )

Updating to 1.6.0
Refactoring
Feature file updates

agosh01 force-pushed the port_cpp_TA_rc1 branch 8 times, most recently from da80698 to 2312651 Compare

July 29, 2024 00:26

agosh01 marked this pull request as ready for review

July 29, 2024 00:31

agosh01 force-pushed the port_cpp_TA_rc1 branch 2 times, most recently from fc7dcdd to e71a74b Compare

July 29, 2024 00:37

agosh01 changed the title ~~Port cpp ta rc1~~ Porting C++ Test Agent to 1.0.1-RC1

agosh01 commented

View reviewed changes

test_agent/cpp/CMakeLists.txt Outdated Show resolved Hide resolved

agosh01 force-pushed the port_cpp_TA_rc1 branch from d5fd500 to c507283 Compare

July 30, 2024 00:34

agosh01 mentioned this pull request

Porting C++ Test Agent to 1.5.8 #86

Closed

agosh01 force-pushed the port_cpp_TA_rc1 branch from c507283 to 82aa07b Compare

July 31, 2024 14:15

agosh01 mentioned this pull request

Update up-tck C++ test agent for 1.5.8 API #59

Closed

agosh01 force-pushed the port_cpp_TA_rc1 branch 2 times, most recently from 0a42edd to 84cb785 Compare

July 31, 2024 14:49

matthewd0123 reviewed

View reviewed changes

Contributor

matthewd0123 left a comment

Honestly only one thing I found... not the best guy for reviewing C++ but it all looks good from me. Feature files are looking great

test_agent/cpp/include/utils.h Show resolved Hide resolved

agosh01 force-pushed the port_cpp_TA_rc1 branch from 84cb785 to 9ab81ec Compare

July 31, 2024 18:58

matthewd0123 approved these changes

View reviewed changes

Contributor

matthewd0123 left a comment

LGTM

agosh01 force-pushed the port_cpp_TA_rc1 branch 3 times, most recently from 7546941 to e8ca283 Compare

August 1, 2024 22:48

agosh01 added 5 commits

August 1, 2024 18:49


          Porting L1 APIs to 1.5.8

d367caf


          Refactoring api handlers to base class

7aac1f0


          Adding L2 apis

cf61720


          Updating test cases

7fe4956


          Updating readme

34cbf71

agosh01 added 2 commits

August 1, 2024 18:49


          Porting to up-cpp rc1

1b5fb43


          Adding notification sink and src feature

c1ee862

agosh01 force-pushed the port_cpp_TA_rc1 branch from e8ca283 to 5c077eb Compare

August 1, 2024 22:49

agosh01 requested a review from gregmedd

August 1, 2024 22:59

billpittman suggested changes

View reviewed changes

billpittman left a comment

Only a few comments/questions.

test_agent/cpp/src/APIWrapper.cpp Outdated

Comment on lines 58 to 55

+              	if (transportType_ == "socket") {
+              		return std::make_shared<SocketUTransport>(uri);
+              	} else {
+              		// If the transport type is neither "socket" nor "zenoh", log an error
+              		// and return null.
+              		spdlog::error("Invalid transport type: {}", transportType_);
+              		return nullptr;
+              	}

billpittman Aug 6, 2024

I think I'd rather see this transportType_ be some sort of enum or #define or something less likely to be mis-typed.
The comment mentions "socket" or "zenoh" as valid inputs, but zenoh's been removed ... Right?

Member Author

agosh01 Aug 6, 2024

I will implement 1 and fix 2. Zenoh is added later next PR. At this time of this PR zenoh was not available

test_agent/cpp/src/APIWrapper.cpp

+              	                                          jsonData.GetAllocator());
+              	// Create transport with the created URI
+              	transportPtr_ = createTransport(def_src_uuri_);

billpittman Aug 6, 2024

I'm a little confused by this call to createTransport. Wasn't the transportPtr_ set in the constructor? And if so, is it expected/necessary to change the transportPtr_ within this method ?

test_agent/cpp/src/APIWrapper.cpp Show resolved Hide resolved

agosh01 force-pushed the port_cpp_TA_rc1 branch 2 times, most recently from 6ec30be to 0bc7e8e Compare

August 6, 2024 20:11

gregmedd assigned agosh01

agosh01 commented

View reviewed changes

test_agent/cpp/README.md Outdated Show resolved Hide resolved

test_agent/cpp/src/APIWrapper.cpp Show resolved Hide resolved

test_agent/cpp/src/APIWrapper.cpp Outdated Show resolved Hide resolved

test_agent/cpp/src/ProtoConverter.cpp Outdated Show resolved Hide resolved

test_agent/cpp/src/TestAgent.cpp Outdated Show resolved Hide resolved

test_agent/cpp/src/APIWrapper.cpp Outdated

Comment on lines 58 to 55

+              	if (transportType_ == "socket") {
+              		return std::make_shared<SocketUTransport>(uri);
+              	} else {
+              		// If the transport type is neither "socket" nor "zenoh", log an error
+              		// and return null.
+              		spdlog::error("Invalid transport type: {}", transportType_);
+              		return nullptr;
+              	}

Member Author

agosh01 Aug 6, 2024

I will implement 1 and fix 2. Zenoh is added later next PR. At this time of this PR zenoh was not available


          Addressing comments

51414a6

agosh01 force-pushed the port_cpp_TA_rc1 branch from 0bc7e8e to 51414a6 Compare

August 13, 2024 16:40

gregmedd approved these changes

View reviewed changes

test_manager/reports/summary/summary.json Outdated Show resolved Hide resolved

billpittman approved these changes

View reviewed changes

gregmedd merged commit 42e2387 into eclipse-uprotocol:main

3 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet