Increase unit test coverage of credentials/attestation_verifier/ by 3.1%
#40670
+308
−76
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LyudmilaKostanyan
changed the title
credentials/attestation_verifier/ by 3%credentials/attestation_verifier/ by 3%
There was a problem hiding this comment.
Pull Request Overview
This PR increases unit test coverage for the credentials/attestation_verifier/ module from 67.9% to 71% by adding comprehensive test cases for certificate and attestation validation in the partial DAC verifier. The tests focus on error scenarios including certificate validity periods, malformed data, and nonce validation.
- Adds tests for expired and future-valid DAC certificates
- Validates handling of malformed attestation elements and nonce mismatches
- Tests PAA certificate format validation failure paths
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/credentials/tests/TestDacOnlyPartialAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully increases unit test coverage for the partial DAC verifier by adding several valuable test cases for certificate and attestation validation. The new tests are well-structured and cover important edge cases.
My review includes a few suggestions to improve maintainability and clarity, such as renaming a misleading variable, replacing magic numbers with sizeof, and clarifying a nonce value in one of the tests. Additionally, there is an opportunity to reduce code duplication across the new tests by refactoring common setup and signing logic into a shared helper function. Addressing these points will make the new tests even more robust and easier to maintain.
src/credentials/tests/TestDacOnlyPartialAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
src/credentials/tests/TestDacOnlyPartialAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
src/credentials/tests/TestDacOnlyPartialAttestationVerifier.cpp
Outdated
Show resolved
Hide resolved
LyudmilaKostanyan
changed the title
credentials/attestation_verifier/ by 3%credentials/attestation_verifier/ by 3.1%
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
4 times, most recently
from
6168d5a to
c207fa5
Compare
|
/gemini-review |
|
Thank you for requesting a review! I am currently reviewing the pull request and will provide feedback shortly. |
LyudmilaKostanyan
moved this to In Progress
in Cluster and example long term maintainability
|
PR #40670: Size comparison from 6e50acf to c207fa5 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
c207fa5 to
aa283a8
Compare
|
PR #40670: Size comparison from a2a07d4 to aa283a8 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
840025c to
ad5b58a
Compare
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
ad5b58a to
aa283a8
Compare
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
aa283a8 to
c741cd8
Compare
|
PR #40670: Size comparison from 233d3f7 to c741cd8 Full report (5 builds for cc32xx, realtek, stm32)
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
2 times, most recently
from
dfdf7fa to
da501c9
Compare
|
PR #40670: Size comparison from 233d3f7 to da501c9 Full report (17 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, qpg, realtek, stm32)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40670 +/- ##
==========================================
+ Coverage 50.78% 50.80% +0.02%
==========================================
Files 1358 1358
Lines 99477 99477
Branches 12877 12868 -9
==========================================
+ Hits 50521 50541 +20
+ Misses 48956 48936 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
4 times, most recently
from
139e85c to
19e9069
Compare
|
PR #40670: Size comparison from 233d3f7 to 19e9069 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
19e9069 to
d4324c9
Compare
|
PR #40670: Size comparison from b31081d to d4324c9 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
2 times, most recently
from
54d5f4b to
76e92e4
Compare
|
PR #40670: Size comparison from f1631b6 to 76e92e4 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
- Add test TestWithExpiredDACCertificate - Add test TestWithExpiredDACCertificate
- Add test for malformed attestation elements: TestWithMalformedAttestationElements - Add test for mismatched nonce handling: TestWithMismatchedNonce
…C certificate validation
LyudmilaKostanyan
force-pushed
the
TestDacOnlyAttestationVerifier
branch
from
76e92e4 to
06af551
Compare
|
PR #40670: Size comparison from 71aed77 to 06af551 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR increases
credentials/attestation_verifier/unit test coverage from 67.9% to 71% by adding tests for certificate and attestation validation in the partial DAC verifier.The new tests cover the following scenarios:
Related issues
Main issue: #37234
Testing
This PR only adds new unit tests. No changes were made to production code.