[aclorch] unittest by gtest by yehjunying · Pull Request #870 · sonic-net/sonic-swss

yehjunying · 2019-05-01T09:53:06Z

What I did
Add unit test for aclorch.

Why I did it
Do unit test in development phase. The developer can check each variable in each stack frame. And also do memory leakage detection or buffer overrun test. That is different than pytest, it is black box test. But, this is white box test.

How I verified it
The new committed testes are all passed.

Details if related
The unit test of aclorch have two scopes. One is cover the whole aclorch behaviors. Second is for internal component like AclTable or AclRule.

The first scope. Each test will call doTask() that simulate configDB was changed. All reaction by AclOrch, AclTable or AclReult will save into libvs via SAI. Then we can verify the results via SAI to make sure every operation is correct.

The second scope using spy function to redirect SAI function pointer to c++ std::function for accessing local variable of test instance. It can verify the result directly without any library.

Because many orch will connect Redis when it is created by constructor, so run the test will need to start Redis first. The later tests will not access Redis.

Signed-off-by: JunYing Yeh [email protected]

msftclas · 2019-05-01T09:53:17Z

All CLA requirements met.

lguohan · 2019-05-01T22:16:47Z

how to run the test?

yehjunying · 2019-05-02T01:38:24Z

Launch Redis to listen unix socket /var/run/redis/redis-server.sock
Goto tests folder and make the executable file
Run by ./test. Then you can see 4 new test be run

[----------] 1 test from AclTest
[ RUN      ] AclTest.Create_L3_Acl_Table
[       OK ] AclTest.Create_L3_Acl_Table (1 ms)
[----------] 1 test from AclTest (2 ms total)

[----------] 3 tests from AclOrchTest
[ RUN      ] AclOrchTest.ACL_Creation_and_Destorying
[       OK ] AclOrchTest.ACL_Creation_and_Destorying (1003 ms)
[ RUN      ] AclOrchTest.L3Acl_Matches_Actions
[       OK ] AclOrchTest.L3Acl_Matches_Actions (1007 ms)
[ RUN      ] AclOrchTest.L3V6Acl_Matches_Actions
[       OK ] AclOrchTest.L3V6Acl_Matches_Actions (1007 ms)
[----------] 3 tests from AclOrchTest (3023 ms total)

qiluo-msft · 2019-05-02T06:24:45Z

tests/check.h

+#include "saiattributelist.h"
+
+struct Check {
+    static bool AttrListEq(sai_object_type_t objecttype, const std::vector<sai_attribute_t>& act_attr_list, /*const*/ SaiAttributeList& exp_attr_list)


/const/ [](start = 108, length = 9)

const or not? #Closed

No const. Will remove the comment.

qiluo-msft · 2019-05-02T06:25:08Z

tests/check.h

+
+            // auto act = sai_serialize_attr_value(*meta, act_attr_list[i].value, false);
+            // auto exp = sai_serialize_attr_value(*meta, &exp_attr_list.get_attr_list()[i].value, false);
+


Could you remove unused code? #Closed

yes, will remove these two lines.

qiluo-msft · 2019-05-02T06:26:11Z

tests/check.h

+                continue;
+            }
+
+            char act_buf[0x4000];


act_buf [](start = 17, length = 7)

Suggest not to create large array in stack. #Closed

Yes, shall not create large array in stack. I will fix it.

qiluo-msft · 2019-05-02T06:28:11Z

tests/check.h

+            sai_attr_id_t id = exp_attr_list.get_attr_list()[i].id;
+            auto meta = sai_metadata_get_attr_metadata(objecttype, id);
+
+            assert(meta != nullptr);


assert [](start = 12, length = 6)

'assert' will be nothing in non debug build and you skip the check. Is it your intent? #Closed

Because this meta query from expectation value. So it shall not happen in any time except developing phase.

qiluo-msft · 2019-05-02T06:33:57Z

tests/ut_helper.h

+// #if WITH_SAI == LIBSAIREDIS
+// #include "hiredis.h"
+// #include "saihelper.h"
+// #endif


remove? #Closed

yes, will remove this unused code.

qiluo-msft · 2019-05-02T06:48:20Z

tests/aclorch_ut.cpp

+        assert(sai_acl_api == nullptr);
+
+        sai_acl_api = new sai_acl_api_t();
+        auto sai_acl = std::shared_ptr<sai_acl_api_t>(sai_acl_api, [](sai_acl_api_t* p) {


std::shared_ptr<sai_acl_api_t> [](start = 23, length = 30)

use make_shared() directly? #Closed

No. Because I want to reset sai_acl_api. make_shared() under C++11, can not pass a customized remove function.

qiluo-msft · 2019-05-02T06:49:17Z

tests/aclorch_ut.cpp

+
+    AclTest()
+    {
+        m_config_db = std::make_shared<swss::DBConnector>(CONFIG_DB, swss::DBConnector::DEFAULT_UNIXSOCKET, 0);


std:: [](start = 22, length = 5)

you can remove 'std::' #Closed

yes. It can be removed.

qiluo-msft · 2019-05-02T07:05:03Z

tests/aclorch_ut.cpp

+    return entries.size();
+}
+
+struct AclTestBase : public ::testing::Test {


AclTestBase [](start = 7, length = 11)

Name is confusing. Has nothing related Acl. #Closed

I think the name is no confuse. That is base class of AclTest and AclOrchTest.

qiluo-msft · 2019-05-02T07:05:58Z

tests/aclorch_ut.cpp

+
+    void SetUp() override
+    {
+        assert(gCrmOrch == nullptr);


assert [](start = 8, length = 6)

the same #Closed

Using assert() because I want catch it on development phase. That is different between ASSERT_TRUE() that is used to catch result of test.

qiluo-msft · 2019-05-02T07:11:36Z

tests/saispy.h

+#include "saitypes.h"
+
+// Spy C functin pointer to std::function to access closure
+// Internal using static `spy` function pointer to invoke std::function `fake`


I read the comment block and also the use case in aclorch_ut.cpp, still not get the design idea of SpyOn. Could you elaborate? We expected a design doc and/or community talk. #Closed

I've put the unit test for how to use the code in saispy_ut.cpp can you check it first ?

qiluo-msft · 2019-05-02T07:13:06Z

@lguohan Could you help add a PR check for the unit test? #Closed

yehjunying · 2019-05-02T08:11:14Z

Should I add [aclorch] for each commit during code review ?

yehjunying · 2019-05-03T02:14:18Z

Hi @lguohan, I found the check error is test_system_warmreboot_neighbor_syncup from pytest. The last blocked PR is #866. It seem this test is not stable. Because my first commit is passed and second commit is failed. Both commit doesn't touch any code in oarchagent and pytest.

Should we continue to review the code. At meantime monitor the error status ?

What's your suggestion ?

yehjunying · 2019-05-03T08:51:41Z

Hi @lguohan @qiluo-msft , I find the test in test_system_warmreboot_neighbor_syncup may have a timing issue.

def setup_initial_neighbors(dvs):
 ...
    for i in range(8, 8+NUM_INTF):
        for j in range(NUM_NEIGH_PER_INTF):
            dvs.runcmd(['sh', '-c', "ping -c 1 -W 0 -q {}.0.0.{} > /dev/null 2>&1".format(i*4,j+2)])
            dvs.runcmd(['sh', '-c', "ping6 -c 1 -W 0 -q {}00::{} > /dev/null 2>&1".format(i*4,j+2)])

def test_system_warmreboot_neighbor_syncup(dvs, testlog):
 ...
    setup_initial_neighbors(dvs)

    # Check the neighbor entries are inserted correctly
    db = swsscommon.DBConnector(0, dvs.redis_sock, 0)
    tbl = swsscommon.Table(db, "NEIGH_TABLE")

    # number of neighbors should match what we configured
    # ipv4/ipv6 entries and loopback
    check_redis_neigh_entries(dvs, tbl, 2*NUM_OF_NEIGHS)

I found setup_initial_neighbors using ping[6] command to trigger neighsyncd to learn new neighbor. But, in the test it read Redis after ping[6] immediately. That may cause neighsyncd too later to learn new neighbor (the path will from kernel -> netlink -> neighsyncd -> write (IPC) Redis).

Do you think ... if add a sleep(N) between setup_initial_neighbors(dvs) and check_redis_neigh_entries(dvs, tbl, 2*NUM_OF_NEIGHS) for 1 or more second to fix this issue ? That is accept to you ?

qiluo-msft · 2019-05-03T18:35:42Z

tests/aclorch_ut.cpp

+        sai_acl_api = new sai_acl_api_t();
+        auto sai_acl = shared_ptr<sai_acl_api_t>(sai_acl_api, [](sai_acl_api_t* p) {
+            delete p;
+            sai_acl_api = nullptr;


sai_acl_api [](start = 12, length = 11)

It abuses the design of deleter that you modify lambda-captured variable. #Closed

No, it is the raw pointer ... however, I move the allocate/delete into SetUp() and TearDown().

pavel-shirshov · 2019-05-06T22:28:10Z

tests/aclorch_ut.cpp

+
+using namespace std;
+
+static size_t consumerAddToSync(Consumer* consumer, const deque<KeyOpFieldsValuesTuple>& entries)


As I know static won't work here.
It's only C keyword for local functions

Will remove static keyword.

pavel-shirshov · 2019-05-06T22:31:20Z

tests/aclorch_ut.cpp

+    acltable.type = ACL_TABLE_L3;
+    auto res = createAclTable(acltable);
+
+    ASSERT_TRUE(res->ret_val == true);


ASSERT_TRUE(res->ret_val);
It's already about true or not.
Otherwise we need to use ASSERT_EQ()

I will fix it.

pavel-shirshov · 2019-05-06T22:33:06Z

tests/aclorch_ut.cpp

+        };
+
+        auto status = sai_api_initialize(0, (sai_service_method_table_t*)&test_services);
+        ASSERT_TRUE(status == SAI_STATUS_SUCCESS);


Yes, will fix it.

pavel-shirshov · 2019-05-06T22:33:54Z

tests/aclorch_ut.cpp

+            { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
+        };
+
+        assert(gPortsOrch == nullptr);


Should we use ASSERT_EQ here?

Yes, will fix it.

pavel-shirshov

Please address my comments.
Please use proper ASSERT_*.

qiluo-msft · 2019-05-10T00:58:35Z

ok to me. Still @lguohan please help add PR checker. #Closed

yehjunying · 2019-05-13T12:55:26Z

Hi @lguohan,
Anything I can help you with?

yehjunying · 2019-05-14T09:51:25Z

Hi @lguohan,
Do you still have other questions about this pull request ? Or something does not clear need me to explain ?

pavel-shirshov · 2019-05-15T00:20:32Z

tests/saispy_ut.cpp

+    sai_object_id_t oid;
+
+    auto status = acl_api->create_acl_table(&oid, 1, 0, nullptr);
+    ASSERT_TRUE(oid == 1);


Can you please use ASSERT_EQ?

Yes. Will Fix.

pavel-shirshov · 2019-05-15T00:20:37Z

tests/saispy_ut.cpp

+
+    auto status = acl_api->create_acl_table(&oid, 1, 0, nullptr);
+    ASSERT_TRUE(oid == 1);
+    ASSERT_TRUE(status == SAI_STATUS_SUCCESS);


Can you please use ASSERT_EQ?

Yes. Will Fix.

pavel-shirshov · 2019-05-15T00:20:44Z

tests/saispy_ut.cpp

+    ASSERT_TRUE(status == SAI_STATUS_SUCCESS);
+
+    status = acl_api->remove_acl_table(2);
+    ASSERT_TRUE(status == SAI_STATUS_SUCCESS);


Can you please use ASSERT_EQ?

Yes. Will Fix.

pavel-shirshov · 2019-05-15T00:21:00Z

tests/saispy_ut.cpp

+    auto z_spy_ref = &z->spy;
+
+    acl_api->create_acl_table(&oid, 1, 0, nullptr);
+    ASSERT_TRUE(oid == exp_oid_1);


Can you please use ASSERT_EQ? here and below

Yes. Will Fix.

yehjunying · 2019-05-15T05:18:27Z

retest this please

yehjunying · 2019-05-15T12:32:54Z

Last time I check the status of Jenkins. It seem show 'can not find test'. But now it show "502 Bad Gateway". Does something wrong in Jenkins ?

yehjunying · 2019-05-16T06:29:33Z

Do any suggestions for this pull request ?
I know Jenkins said failure ... But, I can make sure the modification will not cause pytest failed.

tests/aclorch_ut.cpp

lguohan · 2019-05-20T14:25:07Z

by default debian package build will run make test or make check to run test packages. there is not need to add the test to pr build specifically. @yehjunying can add it to debian packaging.

yehjunying · 2019-05-21T01:33:00Z

Hi @lguohan,
I think the test still run on default debian packaging ... no change anything for the default build behavior.
But to run the test. It will need to launch Redis first. For that.
Plan A, I will remove the limitation to create new mock function to replace DBConnector ctor. (prefer this)
Plan B, lanuch Redis before run test on debian packaging.

Is OK to you ?

yehjunying · 2019-05-22T01:44:30Z

Hi @lguohan,
About this "there is not need to add the test to pr build specifically. @yehjunying can add it to debian packaging." I can not understand ... and don't know what's you want me to do.

After check the build_swss.sh, copy from Jenkins, it call dpkg-buildpackage -us -uc -b to build debin package. It do nothing about test. Include integrate test by pytest.

yehjunying · 2019-05-23T01:28:09Z

Hi @lguohan, Does your means is:
edit debian\rules to enable gtest by dh_auto_configure -- --enable-gtest

Then add new test command in Jenkins for build script ?

pushd sonic-swss/tests
+ ./tests
sudo py.test -v

lguohan · 2019-06-04T16:48:33Z

@yehjunying , check this one. https://www.systutorials.com/docs/linux/man/1-dh_auto_test/

yehjunying · 2019-06-05T09:34:24Z

@lguohan, because this PR is conflicting, so I created another PR #924. You can check the unit test had run by dh_auth_test in Jenkins console.

yehjunying · 2019-06-06T10:05:18Z

Code merged by #924

Added support for VXLAN config and commands as described in the PR sonic-net/SONiC#437 config vxlan add/del and config vxlan evpn_nvo add/del config vxlan map/map_range add show vxlan remote vni/show vxlan remote mac show vxlan tunnel Co-authored-by: Tapash Das <[email protected]> Co-authored-by: Karthikeyan Ananthakrishnan <[email protected]> Co-authored-by: Tapash Das <[email protected]>

8db1eaf 2018-09-07 | added vxlan tunnel as possible condition for underlay interface attribute (sonic-net#870) (HEAD, origin/v1.3) [YonatanPitz] Signed-off-by: Guohan Lu <[email protected]>

* Add Bookworm build to PR checks Signed-off-by: Saikrishna Arcot <[email protected]> * Keep debian_version as buster Signed-off-by: Saikrishna Arcot <[email protected]> --------- Signed-off-by: Saikrishna Arcot <[email protected]>

[aclorch] unittest by gtest

55a2d00

lguohan requested review from pavel-shirshov, qiluo-msft and stcheng May 1, 2019 22:16

qiluo-msft reviewed May 2, 2019

View reviewed changes

yehjunying added 2 commits May 2, 2019 15:20

Modify by code review.

ff555a7

Remove std::

478beb1

qiluo-msft reviewed May 3, 2019

View reviewed changes

[aclorch] init global variable sai_acl_api to SetUp() function.

c7aa97d

pavel-shirshov reviewed May 6, 2019

View reviewed changes

pavel-shirshov suggested changes May 6, 2019

View reviewed changes

pavel-shirshov reviewed May 15, 2019

View reviewed changes

[aclorch] Use proper ASSERT_*. for saispy_ut.cpp

4486c4d

pavel-shirshov approved these changes May 16, 2019

View reviewed changes

lguohan reviewed May 16, 2019

View reviewed changes

tests/aclorch_ut.cpp Show resolved Hide resolved

[aclorch] Unit test can run without Redis server.

ba1dc5f

yehjunying added 2 commits June 5, 2019 12:01

Enable dh_auto_test

24db0f8

[aclorch] Sync code from (#802) Fix MIRROR_SESSION table macro name

7a1b835

yehjunying closed this Jun 6, 2019

yehjunying deleted the aclorch-unittest branch June 6, 2019 10:06

yehjunying restored the aclorch-unittest branch June 6, 2019 21:59

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

Update .gitignore (sonic-net#870)

c5e76c7


		// auto act = sai_serialize_attr_value(*meta, act_attr_list[i].value, false);
		// auto exp = sai_serialize_attr_value(*meta, &exp_attr_list.get_attr_list()[i].value, false);


		using namespace std;

		static size_t consumerAddToSync(Consumer* consumer, const deque<KeyOpFieldsValuesTuple>& entries)

Conversation

yehjunying commented May 1, 2019

Uh oh!

msftclas commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented May 1, 2019

Uh oh!

yehjunying commented May 2, 2019

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yehjunying commented May 2, 2019

Uh oh!

yehjunying commented May 3, 2019

Uh oh!

yehjunying commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented May 1, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft May 2, 2019 •

edited

Loading

qiluo-msft commented May 2, 2019 •

edited

Loading

yehjunying commented May 3, 2019 •

edited

Loading

qiluo-msft May 3, 2019 •

edited

Loading

qiluo-msft commented May 10, 2019 •

edited

Loading