unit tests: identity deserializer - #1412
Merged
Merged
Conversation
aaadir
force-pushed
the
cov_identity_deserializer
branch
from
March 8, 2026 12:13
8aa13e5 to
67b0af4
Compare
AkramBitar
requested changes
Mar 9, 2026
| verifyFunc func(message, sigma []byte) error | ||
| } | ||
|
|
||
| func (m *mockVerifier) Verify(message, sigma []byte) error { |
Contributor
There was a problem hiding this comment.
Did you check if we can use Counterfeiter to generate these mocks?
| return []byte("signature"), nil | ||
| } | ||
|
|
||
| type mockTypedVerifierDeserializer struct { |
Contributor
There was a problem hiding this comment.
Did you check if we can use Counterfeiter to generate these mocks?
Please check all of them
Contributor
Author
There was a problem hiding this comment.
Done.
- Counterfeiters for the following classes are now used instead of the previous manually defined mocks:
drivermock.Matcher
drivermock.Signer
drivermock.Verifier
drivermock.AuditInfoProvider
identitydrivermock.AuditInfo
identitydrivermock.AuditInfoDeserializer
identitydrivermock.TypedSignerDeserializer
identitydrivermock.TypedVerifierDeserializer
- Note that for the above I had to move the definition of the interface TypedVerifierDeserializer
to be intoken/services/identity/driver/deserialization.gonext to the defnition of the interface TypedSignerDeserializer.
It used to be in the filetoken/services/identity/deserializer/verifier.gowhich includes implementations of the TypedVerifierDeserializer interface.
This caused dependency cycles for the newtoken/services/identity/deserializer/deserializer_test.go
which was resolved when I moved the interface defintion to its proper place.
aaadir
force-pushed
the
cov_identity_deserializer
branch
from
March 11, 2026 07:42
67b0af4 to
765e0ad
Compare
AkramBitar
approved these changes
Mar 11, 2026
adecaro
requested changes
Mar 11, 2026
adecaro
left a comment
Contributor
There was a problem hiding this comment.
Please, improve the tests to check:
- when an error is expected, to check also that the programmed errors are in there. For example, in
t.Run("MatchError", func(t *testing.T) {
expectedErr := errors.New("match failed")
matcher := &drivermock.Matcher{}
matcher.MatchReturns(expectedErr)
typedMatcher := &TypedAuditInfoMatcher{matcher: matcher}
typedID := createTypedIdentity(t, "test-type", []byte("raw-identity"))
err := typedMatcher.Match(context.Background(), typedID)
require.Error(t, err)
assert.Contains(t, err.Error(), "failed to match identity")
})The test should check that expectedErr is in the error chain.
- If no error is expected, then the expected value should be checked. For example, in
t.Run("GetAuditInfoMatcher_Success", func(t *testing.T) {
mockVerifierDeserializer := &drivermock.VerifierDeserializer{}
mockMatcher := &drivermock.Matcher{}
mockMatcherDeserializer := &drivermock.MatcherDeserializer{}
mockMatcherDeserializer.GetAuditInfoMatcherReturns(mockMatcher, nil)
deserializer := NewTypedIdentityVerifierDeserializer(mockVerifierDeserializer, mockMatcherDeserializer)
typedID := createTypedIdentity(t, "test-type", []byte("raw-identity"))
matcher, err := deserializer.GetAuditInfoMatcher(context.Background(), typedID, []byte("audit-info"))
require.NoError(t, err)
assert.NotNil(t, matcher)
})The test should check that matcher is mockMatcher.
- counterfeiter allows the developer to check also how many times a function has been called. Please, add also the following checks.
Contributor
|
@aaadir , please, set the status of the PR and link it to its Issue. Thanks. |
aaadir
force-pushed
the
cov_identity_deserializer
branch
from
March 11, 2026 11:43
7e21588 to
d6666b8
Compare
Contributor
Author
|
pushed a new version addressing all the above review comments. |
aaadir
force-pushed
the
cov_identity_deserializer
branch
3 times, most recently
from
March 12, 2026 05:03
9f0f96c to
c546b9c
Compare
adecaro
self-requested a review
March 12, 2026 15:04
Signed-off-by: aaadir <adir@il.ibm.com>
adecaro
force-pushed
the
cov_identity_deserializer
branch
from
March 12, 2026 15:04
c546b9c to
3b2ae28
Compare
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
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.
Improve coverage of token/services/identity/deserializer/deserializer_test.go