Open JunhongMao opened 8 months ago
you will need to pass code coverage to merge this
you will need to pass code coverage to merge this OK. Besides, I have a question. There was an error at the stage of CodeQL / Analyze (cpp) (pull_request_target) .
dpkg-buildpackage: info: host architecture amd64 dpkg-checkbuilddeps: error: Unmet build dependencies: nlohmann-json3-dev dpkg-buildpackage: warning: build dependencies/conflicts unsatisfied; aborting dpkg-buildpackage: warning: (Use -d flag to override.) Error: Process completed with exit code 3.
I checked the files sonic-slave-buster/Dockerfile.j2 and sonic-slave-bullseye/Dockerfile.j2. They all installed the library below. nlohmann-json3-dev. So, my question is, how do I fix this issue? Thanks.
this is for codeql ?
this is for code ql, edit sonic-sairedis/.github/workflows/codeql-analysis.yml 202305 branch
this is for codeql ?
Yes. And for the error during the stage of coverage.Azure.sonic-sairedis.amd64, I think I need add unit test codes to fix them, right?
yes, for code coverage you need add unittests for that
Hi, @kcudnik. I looked into the subdirectories: sonic-sairedis/tests and sonic-sairedis/unittest, there are not any existed saidump test codes. How did the previous saidump codes pass the code coverage test? Need I create a new code file sonic-sairedis/unittest/saidump/TestSaidump.cpp to do the whole unit test of it? Thanks.
Hi, @kcudnik. I looked into the subdirectories: sonic-sairedis/tests and sonic-sairedis/unittest, there are not any existed saidump test codes. How did the previous saidump codes pass the code coverage test? Need I create a new code file sonic-sairedis/unittest/saidump/TestSaidump.cpp to do the whole unit test of it? Thanks.
previous saidump passed since unittest coverage was enabled when saidump was already present
that project is old, starting from 2016, and its not brought up to todays code quality standard, it would need to be converted to OOP and have classes for command line and command line parser and saidump itsel, and be converted to library with samall main.cpp file, which would gratefully improve testing abbility,
please take a look at sairedis/saiplayer/cpp and .h and Makefile.am files as a base how it should be done in todays standard
saiplayer also dont have unittest enabled, if you want to see strucutre how to place test files, you can take a look for vslib and syncd, but those are much complicated, of course you dont need to look everything, just the main concept, and saiplayer is good enough for that to easy understand
and in the long run it will make sense to convert it o OOP since it will be more manageable to write future code and unittest, but at current state, its a lot of work to bring upfront
@JunhongMao Any update on the requires test cases? Can we wrap up this PR quick as it is blocking other PRs... Thanks!
you can make tests to code coverage this as is, but its not be easy, it will still needs to be converted to oop as suggested before, but that is even more work
Hi, @kcudnik @gechiang, @mlok-nokia, the PRs of the similar modification have merged into the master branch. This PR is only intended to fix a compiling error of 202305 branch. Could we bypass this code-testing coverage issue to merge it first and create a new PR to fix the code-testing coverage issue in the future? What are the details of how to convert to OOP as suggested before? I think there will be more communication. As Kcudnik commented, much effort will be needed. Thanks.
yes, you will need to make more changes in the test area to satisfy coverage
@kcudnik , how to update this PR's checking policy to bypass the coverage check blocking?
you cant bypass, you need to add unittests that will coverage the code in 80%
@kcudnik, @gechiang @mlok-nokia , could we bypass this code-testing coverage issue to merge it first and create a new PR to fix the code-testing coverage issue in the future? I'm on leave. I will go back to the office and fix this issue next Monday.
@kcudnik Junhong is new to this module. He may need some guideline how to add the UNIT test case. Is there any example which he can follow? Thanks.
Usually each PR have unittests, please take a look a recent prs and into unittests directory, for those each file needs to be added corresponding with Test prefix and each function should be tested, if you need more info let me know
Code coverage is strictly required policy from the top and is checked time to time, and we get into trouble for bypassing unittests
@kcudnik Thanks. I'm working on it.
Hi @kcudnik, I have added the unit test codes in sonic-sairedis\unittest\saidump, but the Coverage check results showed that the new unit test codes were not called. Could you help to check why? Thanks.
Total: 95 lines Missing: 95 lines Coverage: 0% Threshold: 80% Diff coverage
Hi @kcudnik, I have converted the saidump.cpp to libsaidump.a. But there was still an error below: dpkg-checkbuilddeps: error: Unmet build dependencies: nlohmann-json3-dev.
I checked the building scripts and found that they didn't include nlohmann-json3-dev. So, I think I need to create a separate PR that only installs nlohmann-json3-dev. After merging that PR, this PR could be checked successfully. sudo apt-get install -y libxml-simple-perl \ aspell \ ... ... libjansson-dev \ python
What do you think of it?
I created new PR: https://github.com/sonic-net/sonic-sairedis/pull/1350
approved
not sure why tou need json package if so far everything was compiling fine
@kcudnik @gechiang I found the failures of the below checks are wired.
Azure.sonic-sairedis (Build amd64) Failing after 33m — Build amd64 failed Azure.sonic-sairedis Failing after 33m — Build #20240416.3 failed CodeQL / Analyze (cpp) (pull_request_target) Failing after 10m
The similar failures are excited in many recent commits below: https://github.com/sonic-net/sonic-sairedis/commits/202305/ I guess they are not related to this PR. Please help to double-check them. Thanks.
/azp run
i think 202305 azpipeline eneeds to be updated
Checkig for white spaces ...
../saidump/main.cpp:#include "saidump.h"
../saidump/main.cpp:
../saidump/main.cpp:using namespace syncd;
../saidump/main.cpp:
../saidump/main.cpp:int main(int argc, char **argv)
../saidump/main.cpp:{
../saidump/main.cpp: SWSS_LOG_ENTER();
../saidump/main.cpp: swss::Logger::getInstance().setMinPrio(swss::Logger::SWSS_INFO);
../saidump/main.cpp:
../saidump/main.cpp: SaiDump m_saiDump;
../saidump/main.cpp:
../saidump/main.cpp: if(SAI_STATUS_SUCCESS != m_saiDump.handleCmdLine(argc, argv))
../saidump/main.cpp: {
../saidump/main.cpp: return EXIT_FAILURE;
../saidump/main.cpp: }
../saidump/main.cpp:
../saidump/main.cpp: m_saiDump.dumpFromRedisDb();
../saidump/main.cpp: return EXIT_SUCCESS;
../saidump/saidump.h: sai_status_t preProcessFile(const std::string file_name);
../unittest/saidump/main.cpp: syncd::SaiDump m_saiDump;
../unittest/saidump/main.cpp: m_saiDump.rdbJsonFile = "./dump.json";
ERROR: some files contain white spaces at the end of line, please fix
FAIL: checkwhitespace.sh
This is why you get unit tests to fail you have a lot of white spaces in those lines
Please carefully read errors on builds and tests
Why I did it
Based on the PR1288 https://github.com/sonic-net/sonic-sairedis/pull/1288
and the comment from gechiang https://github.com/sonic-net/sonic-sairedis/pull/1288#issuecomment-1865371815
in 202305, we should use "#include " swss/json.hpp " " other than #include <nlohmann/json.hpp>,
Below is the information on the PR1288. Fix issue: https://github.com/sonic-net/sonic-buildimage/issues/13561 The existing saidump use https://github.com/sonic-net/sonic-swss-common/blob/master/common/table_dump.lua script which loops the ASIC_DB more than 5 seconds and blocks other processes access.
This solution uses the redis-db SAVE option to save the snapshot of DB each time and recover later, instead of looping through each entry in the table. Related PRs: https://github.com/sonic-net/sonic-utilities/pull/2972 https://github.com/sonic-net/sonic-buildimage/pull/16466
MSFT ADO: 25892357
How I did it
To use the Redis-db SAVE option to save the snapshot of DB each time and recover later, instead of looping through each entry in the table and saving it.
How to verify it
Method 1 1) On T2 setup with more than 96K routes, execute CLI command -- generate_dump 2) No error should be shown 3) Download the generate_dump result and verify the saidump file after unpacking it.
Method 2 1) Run syncd0 bash docker exec -it syncd0 bash 2) Get saidump output by looping through each entry in the table root@ixre-egl-board40:/# saidump | sha1sum 821d442938028114b614bd810ecf844d564fd812 - 3) Get saidump output by using the redis-db SAVE option to save the snapshot of D root@ixre-egl-board40:/# saidump.sh | sha1sum 821d442938028114b614bd810ecf844d564fd812 - 4) The sha1sums are the same, so the test passed.
How to do unit tests manually. 1) Go into compile container bash. jumao@4b61df49afa7:/sonic/src/sonic-sairedis/unittest 2) Run saidump unit tests.
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)