sonic-swss
sonic-swss copied to clipboard
sonic-net
[UT] [Portsyncd] Added Unit Tests for portsyncd
What I did
1. Refactored portsyncd to improve testability:
- move free function declarations to header files and definitions to portsyncd_helper.cpp instead of portsyncd.cpp
- Move Global Data Structures into portsyncd_helper.cpp instead of portsyncd.cpp
2. Added Unit tests for portsyncd module:
- Create a new make target under mock_tests/Makefile.am to include a target for linksync.cpp
- Mocked a rtnl_link object for testing onMsg method of Linksync.cpp
- Used/updated the existing mock infra to allow for testing.
Why I did it
To increase code/feature coverage
How I verified it
vkarri@e4bc0a3e5630:/sonic$ make -f slave.mk target/debs/bullseye/swss_1.0.0_amd64.deb
..........
make[3]: Entering directory '/sonic/src/sonic-swss/tests'
Making check in mock_tests
make[4]: Entering directory '/sonic/src/sonic-swss/tests/mock_tests'
make check-TESTS
make[5]: Entering directory '/sonic/src/sonic-swss/tests/mock_tests'
make[6]: Entering directory '/sonic/src/sonic-swss/tests/mock_tests'
PASS: tests_portsyncd
PASS: tests_intfmgrd
PASS: tests
============================================================================
Testsuite summary for sonic-swss 1.0
============================================================================
# TOTAL: 3
# PASS: 3
# SKIP: 0
# XFAIL: 0
# FAIL: 0
# XPASS: 0
# ERROR: 0
============================================================================
vkarri@4897dcba242c:/sonic/src/sonic-swss/tests/mock_tests$ make tests_portsyncd
.............
..............
..............
vkarri@4897dcba242c:/sonic/src/sonic-swss/tests/mock_tests$ ./tests_portsyncd
Running main() from /usr/src/gtest/src/gtest_main.cc
[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from PortSyncdTest
[ RUN ] PortSyncdTest.test_linkSyncInit
[ OK ] PortSyncdTest.test_linkSyncInit (0 ms)
[ RUN ] PortSyncdTest.test_handlePortConfigFromConfigDB
[ OK ] PortSyncdTest.test_handlePortConfigFromConfigDB (1 ms)
[ RUN ] PortSyncdTest.test_handlePortConfigFromConfigDBWarmBoot
[ OK ] PortSyncdTest.test_handlePortConfigFromConfigDBWarmBoot (0 ms)
[ RUN ] PortSyncdTest.test_cacheOldIfaces
[ OK ] PortSyncdTest.test_cacheOldIfaces (0 ms)
[ RUN ] PortSyncdTest.test_onMsgNewLink
[ OK ] PortSyncdTest.test_onMsgNewLink (0 ms)
[ RUN ] PortSyncdTest.test_onMsgDelLink
[ OK ] PortSyncdTest.test_onMsgDelLink (0 ms)
[ RUN ] PortSyncdTest.test_onMsgMgmtIface
[ OK ] PortSyncdTest.test_onMsgMgmtIface (0 ms)
[ RUN ] PortSyncdTest.test_onMsgIgnoreOldNetDev
[ OK ] PortSyncdTest.test_onMsgIgnoreOldNetDev (0 ms)
[----------] 8 tests from PortSyncdTest (1 ms total)
[----------] Global test environment tear-down
[==========] 8 tests from 1 test case ran. (1 ms total)
[ PASSED ] 8 tests.
Details if related
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
This PR has conflict with https://github.com/Azure/sonic-swss/pull/2305. I suggest lets revisit this PR after 2305 is merged. The newly introduced helper file in this PR shall have to be revisited and I don't think it would be required.
This PR has conflict with #2305. I suggest lets revisit this PR after 2305 is merged. The newly introduced helper file in this PR shall have to be revisited and I don't think it would be required.
The file will still be needed as the structures g_portSet, g_init and the method handlePortConfigFromConfigDB are still required, but yeah this can wait until #2305 is merged.
This PR has conflict with #2305. I suggest lets revisit this PR after 2305 is merged. The newly introduced helper file in this PR shall have to be revisited and I don't think it would be required.
The file will still be needed as the structures g_portSet, g_init and the method handlePortConfigFromConfigDB are still required, but yeah this can wait until #2305 is merged. For just one function, I wouldn't recommend creating new file. IMO, it is unnecessary and future cherry-picks have issue if there is a bug fix.
as we still didnt close the HLD of the other PR, i suggest we use this one first and once the other PR can be approved and get in it will have to be aligned with this one. Can we then merge this one and update the other. @Junchao-Mellanox fyi
This PR has conflict with #2305. I suggest lets revisit this PR after 2305 is merged. The newly introduced helper file in this PR shall have to be revisited and I don't think it would be required.
The file will still be needed as the structures g_portSet, g_init and the method handlePortConfigFromConfigDB are still required, but yeah this can wait until #2305 is merged. For just one function, I wouldn't recommend creating new file. IMO, it is unnecessary and future cherry-picks have issue if there is a bug fix.
TBH, future cherry-picks will be an issue nevertheless because i can't keep these methods/data-structures "g_portSet, g_init and the method handlePortConfigFromConfigDB" in portsyncd.cpp. If not for the helper file, i've to move them to Linksync.cpp, so any change to those can't be cherry-picked directly to older branches.
@vivekrnv could you please handle conflicts and followup on checkers?
@vivekrnv could you please handle conflicts and followup on checkers?
Handled. @prsunny can you please review
i dont think we should combine refactoring and portsyncd changes together. Can you please have this PR only for the portsyncd?
i dont think we should combine refactoring and portsyncd changes together. Can you please have this PR only for the portsyncd?
Will update
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: vivekrnv / name: Vivek (1c6cd64135ebd0d25c08b3f07f7656e638bf711e, 0ce3d92d11c32783301241efdb845a818d751068, 7395544bdcc596bd29e2ba5da270c9267a63d56e, b98badf54093326e25b105540b56872ae0300ad8, dc384cddb81cdf38eadcd70bc3afa7ba6bff3b57, bc7528aa6cd5b601757d37aca5da75485afcb1c6, e052e201c5239adebd15394121b2a46040bb8f8b, e267fd9513a7ad700e557852fdadbc6fe9f43ce8, 67a086d549bf50880627f2a6b2ac9fa9657aa041, 64a8ee1cecf020dce23317dfca18168a46fda7e4, 6909333fce9be4fbafa9dfbc889ba9c0feb11bde, 7f26e83bbf9717c01ba3b42dbce3cf1c643b2e81, 04023301b5e6faab9f162c778ad4019c60a62eed, 0523b8c2f76598a07a48ea5e57e95bfd3b126ade, 8a3c138ce799f4248587db2a8db42c2613567634, dfcba645f586458067c8c8debb4f66560b09718f, 1bc2d97b0263b922bf85abb3f1376f709f2e2664, 6134e80d92670066363c909fd45745d7af3429ed, f7576681589b0bdec342d9b8d0851b955dcb6ef9, 06edeb9b2bfbc3e07b4a7f6f05aa6530336f0998, 98b63585196925ffff32e6a28da05fa73bbd802e, d436281c79a2820a67a008e511fca364c1dbc90a, 70099b43b898eb3d2ec0ba7c508361100823a685)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
i dont think we should combine refactoring and portsyncd changes together. Can you please have this PR only for the portsyncd?
updated