UT coverage to 95%, code improve and minor fix #34

jcaiMR · 2023-03-17T17:51:41Z

Description:

Add gtest/gmock based gmock-global, Improve UT coverage from 80% to 95%
code cleanup, remove unused code
a) remove unused functions handleSwssNotification(), stopSwssNotificationPoll()
b) change file name to GUN style, keep same style with the other files
code improvement based on the findings in UT
a) DHCPv6Msg::MarshalBinary/RelayMsg::MarshalBinary add protection for super frame, case for new packet length > 65536
b) loop_relay() event base creation failure will call exit(EXIT_FAILURE)
c) shutdown() name changed to shutdown_relay() as name duplication with glibc library.
change PR target from 60% to 90%
Makefile changes due to git clone MIT license based library, gmock-global

How I test it

make test
load new image to DUT, run test_dhcpv6_relay

yxieca · 2023-03-17T22:33:09Z

@jcaiMR

can you look into the build failures?
it appears that you added bug fixes (not just code cleanups). Can you update the PR title and comments accordingly?

kellyyeh · 2023-03-17T22:45:40Z

@jcaiMR Looks like build failed due to package dependency. Adding sudo apt-get update in .azure-pipelines/build.yml might fix it

saiarcot895 · 2023-03-17T23:20:51Z

Build fix is in #35.

jcaiMR · 2023-03-18T03:15:35Z

@kellyyeh @saiarcot895 Thanks all for fix this. I guess it not due to my PR code change, I raised a master based pipeline it also failed at arm64 and armhf, which looks good several weeks ago. Is it due to something changed/updated in our build system ?

jcaiMR · 2023-03-18T03:32:51Z

@jcaiMR

can you look into the build failures?

it appears that you added bug fixes (not just code cleanups). Can you update the PR title and comments accordingly?

Got it, just updated title and details in description.

saiarcot895 · 2023-03-18T16:09:20Z

@kellyyeh @saiarcot895 Thanks all for fix this. I guess it not due to my PR code change, I raised a master based pipeline it also failed at arm64 and armhf, which looks good several weeks ago. Is it due to something changed/updated in our build system ?

The failures were due to changes in the slave container builds that removed the apt package cache at the end of the build. This meant that any users of the slave image who wanted to install new packages would need to do an apt update first.

jcaiMR · 2023-03-20T14:56:21Z

@kellyyeh @saiarcot895 Thanks all for fix this. I guess it not due to my PR code change, I raised a master based pipeline it also failed at arm64 and armhf, which looks good several weeks ago. Is it due to something changed/updated in our build system ?

The failures were due to changes in the slave container builds that removed the apt package cache at the end of the build. This meant that any users of the slave image who wanted to install new packages would need to do an apt update first.

Got it, thank you for the detailed explanation.

test/mock_relay.cpp

saiarcot895 · 2023-04-04T16:07:28Z

test/mock_relay.cpp

-    EXPECT_TRUE(false);
-  }
+   // same name to override static global variable
+  uint8_t server_recv_buffer[BUFFER_SIZE] = {


I don't think you can override a static global variable in another .cpp file. Are you sure this is working?

Yes, Saikrishna, you are right. It's some dummy code I tried in my testing, I will remove them for less confusion.

Can you remove this code?

Sure, my miss, will apply in latest diff.

Makefile

saiarcot895 · 2023-04-23T03:26:37Z

test/mock_relay.cpp

  config.local_sock = -1;

+  std::unordered_map<std::string, struct relay_config> vlans;
+
+  uint8_t client_recv_buffer[] = {


Is this variable used? It doesn't seem like it.

It only used to get a valid dhcp client packet size. I leave it there for an example of good dhcp client packet, maybe it would help to understand the original function meaning in future. :-)

removed in latest diff.

* Update package cache, and bail on the first error Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Incorrectly added line continuation Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> --------- Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

linux-foundation-easycla · 2023-05-07T14:55:12Z

The committers listed above are authorized under a signed CLA.

✅ login: jcaiMR (c98cdff, dfb8e92)
✅ login: saiarcot895 / name: Saikrishna Arcot (031018a)

jcaiMR changed the title ~~global mock, ut coverage to 95%, code cleanup and refine~~ Improve UT coverage to 95% Mar 17, 2023

global mock, ut coverage to 95%, code cleanup and refine

c98cdff

jcaiMR force-pushed the dev/dhcprelay_ut_improve_new branch from 3bd2891 to c98cdff Compare March 17, 2023 18:05

jcaiMR marked this pull request as ready for review March 17, 2023 18:16

jcaiMR requested review from kellyyeh and yxieca March 17, 2023 18:16

jcaiMR changed the title ~~Improve UT coverage to 95%~~ Improve UT coverage to 95%, code improve Mar 18, 2023

jcaiMR changed the title ~~Improve UT coverage to 95%, code improve~~ UT coverage to 95%, code improve and minor fix Mar 18, 2023

kellyyeh approved these changes Apr 4, 2023

View reviewed changes

test/mock_relay.cpp Show resolved Hide resolved

saiarcot895 reviewed Apr 4, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jcaiMR requested a review from saiarcot895 April 22, 2023 05:52

saiarcot895 reviewed Apr 23, 2023

View reviewed changes

jcaiMR force-pushed the dev/dhcprelay_ut_improve_new branch from 544f8b9 to 7ff9806 Compare May 7, 2023 14:55

adjust printf format

dfb8e92

jcaiMR force-pushed the dev/dhcprelay_ut_improve_new branch from 7ff9806 to dfb8e92 Compare May 7, 2023 15:03

jcaiMR requested a review from saiarcot895 May 9, 2023 01:20

jcaiMR merged commit 4b17265 into sonic-net:master May 13, 2023

vivekrnv mentioned this pull request Jun 28, 2023

Pick dependency files in submodules. sonic-net/sonic-buildimage#15142

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UT coverage to 95%, code improve and minor fix #34

UT coverage to 95%, code improve and minor fix #34

jcaiMR commented Mar 17, 2023 •

edited

Loading

yxieca commented Mar 17, 2023

kellyyeh commented Mar 17, 2023

saiarcot895 commented Mar 17, 2023

jcaiMR commented Mar 18, 2023

jcaiMR commented Mar 18, 2023

saiarcot895 commented Mar 18, 2023

jcaiMR commented Mar 20, 2023

saiarcot895 Apr 4, 2023

jcaiMR Apr 6, 2023

saiarcot895 Apr 23, 2023

jcaiMR May 6, 2023

saiarcot895 Apr 23, 2023

jcaiMR May 6, 2023

jcaiMR May 7, 2023

linux-foundation-easycla bot commented May 7, 2023 •

edited

Loading

UT coverage to 95%, code improve and minor fix #34

UT coverage to 95%, code improve and minor fix #34

Conversation

jcaiMR commented Mar 17, 2023 • edited Loading

yxieca commented Mar 17, 2023

kellyyeh commented Mar 17, 2023

saiarcot895 commented Mar 17, 2023

jcaiMR commented Mar 18, 2023

jcaiMR commented Mar 18, 2023

saiarcot895 commented Mar 18, 2023

jcaiMR commented Mar 20, 2023

saiarcot895 Apr 4, 2023

Choose a reason for hiding this comment

jcaiMR Apr 6, 2023

Choose a reason for hiding this comment

saiarcot895 Apr 23, 2023

Choose a reason for hiding this comment

jcaiMR May 6, 2023

Choose a reason for hiding this comment

saiarcot895 Apr 23, 2023

Choose a reason for hiding this comment

jcaiMR May 6, 2023

Choose a reason for hiding this comment

jcaiMR May 7, 2023

Choose a reason for hiding this comment

linux-foundation-easycla bot commented May 7, 2023 • edited Loading

jcaiMR commented Mar 17, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 7, 2023 •

edited

Loading