-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for MACsec statistics #892
Conversation
1c578b7
to
c8db2bf
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
to pass coverage.Azure.sonic-sairedis.amd64 you would need to add unittests at 50% for added code, can you also add unittests for your code? |
@kcudnik Can you point me to similar unittests for sairedis FlexCounter support? |
you can start adding tests in sonic-sairedis/unittest/syncd path, creating file TestFlexCounter.cpp and follow pattern like for other files in sonic-sairedis/unittest/*, you would just need to cover like 50% of code added of this PR to pass the tests, you probably need to get familiar with gtest framework |
@kcudnik @Pterosaur I am only assigned to work on SONiC PRs for another week, so I cannot undertake providing all new coverage infrastructure for sairedis FlexCounters in the context of this PR. If coverage is now a requirement for sairedis PR approval, then Ze will have to take up that effort in a separate PR. |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@qbdwlr ok, then i can force merge this after tests passes, skipping coverage but it needs to be added later |
@kcudnik After rebasing to include latest commits to sairedis, I see the following failures in lgtm.com that are unrelated to any changes I introduced. Is this a known issue? |
this is related to latest swss-common starting using gmock lib, it's attempted to be fixed here : 42e5427 |
ee9e6d3
to
886642c
Compare
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@kcudnik @Pterosaur Can we get this PR approved/merged? Or do we have to wait for someone to fix these flaky vstests? I was able to re-use recently merged FlexCounter unittest infra to get >50% coverage for this PR, so that is no longer an issue. |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@kcudnik This PR is approved but hasn't been merged yet because vstests have been broken for two weeks. I don't see any fixes for vstest happening soon. Can this PR be merged despite the vstest failures? |
ok |
Build dependency on sonic-net/sonic-swss-common#520