commit a5ed6585c102e0cb502080f4265ace9ad1bbbdcf
author Tom Joseph <rushtotom@gmail.com> Thu Jun 17 22:08:47 2021 -0700
committer Tom Joseph <rushtotom@gmail.com> Tue Jun 22 01:58:19 2021 +0000
tree aa0653d046c503c6788a066b37b82202c07da015
parent ea72f240bbb734b0faa8f9b29056eca5a163c1de

requester: Enhance the PLDM requester code

- Free the instance id on error paths
- Make the conditional checks consistent
- Make the unit tests robust

Signed-off-by: Tom Joseph <rushtotom@gmail.com>
Change-Id: Ieb70da2820ab818c36f5bf874a602fb090659836

requester/test/handler_test.cpp

diff --git a/requester/test/handler_test.cpp b/requester/test/handler_test.cpp
index 7230b08..aa0655f 100644
--- a/requester/test/handler_test.cpp
+++ b/requester/test/handler_test.cpp
@@ -78,9 +78,10 @@
                                               seconds(1), 2, milliseconds(100));
     pldm::Request request{};
     auto instanceId = dbusImplReq.getInstanceId(eid);
-    reqHandler.registerRequest(
+    auto rc = reqHandler.registerRequest(
         eid, instanceId, 0, 0, std::move(request),
         std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 
     pldm::Response response(sizeof(pldm_msg_hdr) + sizeof(uint8_t));
     auto responsePtr = reinterpret_cast<const pldm_msg*>(response.data());
@@ -89,8 +90,8 @@
 
     // handleResponse() will free the instance ID after calling the response
     // handler, so the same instance ID is granted next as well
-    ASSERT_EQ(validResponse, true);
-    ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+    EXPECT_EQ(validResponse, true);
+    EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
 }
 
 TEST_F(HandlerTest, singleRequestInstanceIdTimerExpired)
@@ -99,17 +100,18 @@
                                               seconds(1), 2, milliseconds(100));
     pldm::Request request{};
     auto instanceId = dbusImplReq.getInstanceId(eid);
-    reqHandler.registerRequest(
+    auto rc = reqHandler.registerRequest(
         eid, instanceId, 0, 0, std::move(request),
         std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 
     // Waiting for 500ms so that the instance ID expiry callback is invoked
     waitEventExpiry(milliseconds(500));
 
     // cleanup() will free the instance ID after calling the response
     // handler will no response, so the same instance ID is granted next
-    ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
-    ASSERT_EQ(nullResponse, true);
+    EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+    EXPECT_EQ(nullResponse, true);
 }
 
 TEST_F(HandlerTest, multipleRequestResponseScenario)
@@ -118,22 +120,24 @@
                                               seconds(2), 2, milliseconds(100));
     pldm::Request request{};
     auto instanceId = dbusImplReq.getInstanceId(eid);
-    reqHandler.registerRequest(
+    auto rc = reqHandler.registerRequest(
         eid, instanceId, 0, 0, std::move(request),
         std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 
     pldm::Request requestNxt{};
     auto instanceIdNxt = dbusImplReq.getInstanceId(eid);
-    reqHandler.registerRequest(
+    rc = reqHandler.registerRequest(
         eid, instanceIdNxt, 0, 0, std::move(requestNxt),
         std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 
     pldm::Response response(sizeof(pldm_msg_hdr) + sizeof(uint8_t));
     auto responsePtr = reinterpret_cast<const pldm_msg*>(response.data());
     reqHandler.handleResponse(eid, instanceIdNxt, 0, 0, responsePtr,
                               sizeof(response));
-    ASSERT_EQ(validResponse, true);
-    ASSERT_EQ(callbackCount, 1);
+    EXPECT_EQ(validResponse, true);
+    EXPECT_EQ(callbackCount, 1);
     validResponse = false;
 
     // Waiting for 500ms and handle the response for the first request, to
@@ -143,7 +147,7 @@
     reqHandler.handleResponse(eid, instanceId, 0, 0, responsePtr,
                               sizeof(response));
 
-    ASSERT_EQ(validResponse, true);
-    ASSERT_EQ(callbackCount, 2);
-    ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+    EXPECT_EQ(validResponse, true);
+    EXPECT_EQ(callbackCount, 2);
+    EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
 }
\ No newline at end of file

requester/test/request_test.cpp

diff --git a/requester/test/request_test.cpp b/requester/test/request_test.cpp
index 6471d9e..1b41ff6 100644
--- a/requester/test/request_test.cpp
+++ b/requester/test/request_test.cpp
@@ -54,7 +54,7 @@
         .Times(Exactly(1))
         .WillOnce(Return(PLDM_SUCCESS));
     auto rc = request.start();
-    ASSERT_EQ(rc, PLDM_SUCCESS);
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 }
 
 TEST_F(RequestIntfTest, 2Retries100msTimeout)
@@ -64,7 +64,7 @@
     // send() is called a total of 3 times, the original plus two retries
     EXPECT_CALL(request, send()).Times(3).WillRepeatedly(Return(PLDM_SUCCESS));
     auto rc = request.start();
-    ASSERT_EQ(rc, PLDM_SUCCESS);
+    EXPECT_EQ(rc, PLDM_SUCCESS);
     waitEventExpiry(milliseconds(500));
 }
 
@@ -82,7 +82,7 @@
         .Times(Between(5, 10))
         .WillRepeatedly(Return(PLDM_SUCCESS));
     auto rc = request.start();
-    ASSERT_EQ(rc, PLDM_SUCCESS);
+    EXPECT_EQ(rc, PLDM_SUCCESS);
 
     auto requestStopCallback = [&](void) { request.stop(); };
     phosphor::Timer timer(event.get(), requestStopCallback);
@@ -97,5 +97,5 @@
                         milliseconds(100));
     EXPECT_CALL(request, send()).Times(Exactly(1)).WillOnce(Return(PLDM_ERROR));
     auto rc = request.start();
-    ASSERT_EQ(rc, PLDM_ERROR);
+    EXPECT_EQ(rc, PLDM_ERROR);
 }