Conversation
Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
|
@taoyl-ms , @stcheng , @qiluo-msft to review |
|
|
||
| override_dh_auto_configure: | ||
| dh_auto_configure -- | ||
| dh_auto_configure -- --enable-gtest |
There was a problem hiding this comment.
--enable-gtest [](start = 22, length = 14)
It will not work in sonic-slave. The tests require redis service running.
There was a problem hiding this comment.
Not sure how sonic prepares unit test environment in Jenkins build. I install the debian package in sonic-slave and start redis service with "redis-server &" commond.
There was a problem hiding this comment.
You got it clear. So just keep original version, and manually test it with
./configure --enable-gtest
make
tests/tests
|
|
||
| CFLAGS_GTEST = | ||
| LDADD_GTEST = | ||
| CFLAGS_GTEST = -I $(top_srcdir)/../sonic-swss-common/googletest/googletest/include |
There was a problem hiding this comment.
$(top_srcdir)/.. [](start = 18, length = 16)
We should not refer any files outside the $(top_srcdir)
There was a problem hiding this comment.
We only have google unit test environment in swss-common. What is your suggestion on how to build and run unit test in swss?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, the question is do we want to build unit test binary automatically like that in swss-common? Or it is ok just put unit test code there, whoever is interested in running the unit test can figure out how to build and run it themselves?
The other option is to make gtest common library like swss-common.
There was a problem hiding this comment.
agree that we shall not include files outside this repository. I think the option 1 in the link Qi provided is preferred.
There was a problem hiding this comment.
We plan to remove googletest from sonic-swss-common repo, and use installed Google Test DEB package near future.
It's ok to build the test if easy. I guess you will need to install Google Test DEB package in the sonic-slave docker.
| #include "consumertable.h" | ||
|
|
||
| using namespace std; | ||
| using namespace swss; |
There was a problem hiding this comment.
using namespace should not be used in header file.
https://stackoverflow.com/questions/5849457/using-namespace-in-c-headers
There was a problem hiding this comment.
Ok, will remove them.
| typedef pair<string, Consumer> ConsumerMapPair; | ||
| typedef map<string, Consumer> ConsumerMap; | ||
|
|
||
| class CfgOrch |
There was a problem hiding this comment.
CfgOrch [](start = 6, length = 7)
inherit from Orch?
There was a problem hiding this comment.
Orch looks too heavy for config orchestration, but cfgorch does have major overlap with orch. Will look into it further to see how to reduce the duplication.
| Consumer& consumer = consumer_it->second; | ||
|
|
||
| int data_popped = 0; | ||
| while (1) |
There was a problem hiding this comment.
1 [](start = 11, length = 1)
true
There was a problem hiding this comment.
ok, will change it.
| break; | ||
| } | ||
| data_popped++; | ||
| SWSS_LOG_DEBUG("%s", (dumpTuple(consumer, new_data)).c_str()); |
There was a problem hiding this comment.
( [](start = 29, length = 1)
You could remove this pair.
| { | ||
| KeyOpFieldsValuesTuple existing_data = consumer.m_toSync[key]; | ||
|
|
||
| auto new_values = kfvFieldsValues(new_data); |
There was a problem hiding this comment.
auto [](start = 12, length = 4)
Many places, you could use "auto &" to reduce copying cost.
There was a problem hiding this comment.
ok, will check how to sync with orch class too.
| m_db(db) | ||
| { | ||
| Consumer consumer(new SubscriberStateTable(m_db, tableName)); | ||
| m_consumerMap.insert(ConsumerMapPair(tableName, consumer)); |
There was a problem hiding this comment.
insert [](start = 18, length = 6)
Use emplace to prevent object copy.
|
With the orch class change in #325, probably it is unnecessary to have a separate cfgOrch class. The two new methods could be easily added there. Future stateDB notification handling also fits there with minor update in We may create a new PR for orch unit test. |
|
Obsoleted by #360 |
Signed-off-by: Guohan Lu <lguohan@gmail.com>
4 participants
Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com
What I did
Add cfgOrch class to be used by cfgDB enforcers
Why I did it
Common function for cfgDB enforcers
How I verified it
jipan@27bcdfdac3ca:/sonic/src/sonic-swss/tests$ ./tests
Running main() from gtest_main.cc
[==========] Running 9 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 8 tests from swssnet
[ RUN ] swssnet.copy1_v6
[ OK ] swssnet.copy1_v6 (0 ms)
[ RUN ] swssnet.copy1_v4
[ OK ] swssnet.copy1_v4 (0 ms)
[ RUN ] swssnet.copy2_v6
[ OK ] swssnet.copy2_v6 (0 ms)
[ RUN ] swssnet.copy2_v4
[ OK ] swssnet.copy2_v4 (0 ms)
[ RUN ] swssnet.copy3_v6
[ OK ] swssnet.copy3_v6 (0 ms)
[ RUN ] swssnet.copy3_v4
[ OK ] swssnet.copy3_v4 (0 ms)
[ RUN ] swssnet.subnet_v6
[ OK ] swssnet.subnet_v6 (0 ms)
[ RUN ] swssnet.subnet_v4
[ OK ] swssnet.subnet_v4 (0 ms)
[----------] 8 tests from swssnet (0 ms total)
[----------] 1 test from CfgOrch
[ RUN ] CfgOrch.test
Starting cfgOrch testing
Done.
[ OK ] CfgOrch.test (11023 ms)
[----------] 1 test from CfgOrch (11023 ms total)
[----------] Global test environment tear-down
[==========] 9 tests from 2 test cases ran. (11023 ms total)
[ PASSED ] 9 tests.
jipan@27bcdfdac3ca:/sonic/src/sonic-swss/tests$
Details if related