test: add unit tests for consistency/forcefull.go#2916
test: add unit tests for consistency/forcefull.go#2916miparnisari merged 2 commits intoauthzed:mainfrom
Conversation
github-actions
bot
added
the
area/tooling
ivanauth
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (74.74%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2916 +/- ##
==========================================
+ Coverage 74.71% 74.74% +0.03%
==========================================
Files 494 494
Lines 60623 60623
==========================================
+ Hits 45290 45308 +18
+ Misses 12169 12148 -21
- Partials 3164 3167 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ds.AssertExpectations(t) | ||
| }) | ||
|
|
||
| t.Run("returns nil for request without consistency interface", func(t *testing.T) { |
There was a problem hiding this comment.
there is nothing returning nil here. you should change the assertion - call RevisionFromContext and then assert that you got nothing back.
| t.Run("returns nil for request without consistency interface", func(t *testing.T) { | ||
| t.Parallel() | ||
| ctx := ContextWithHandle(t.Context()) | ||
| err := setFullConsistencyRevisionToContext(ctx, "not-a-consistency-request", nil, "", TreatMismatchingTokensAsFullConsistency) |
There was a problem hiding this comment.
you are passing a request without consistency, but you are also passing nil as the datastore. as a reader of this test, i have no proof that the result that you got back is because of the former and not because of the latter. please pass a valid ds.
| resp, err := interceptor( | ||
| context.Background(), | ||
| &v1.ReadRelationshipsRequest{}, | ||
| &grpc.UnaryServerInfo{FullMethod: "/grpc.reflection.v1.ServerReflection/ServerReflectionInfo"}, |
There was a problem hiding this comment.
@josephschorr could you explain why the ForceFull interceptors use the bypassServiceWhitelist? what is the point? none of the methods in those services will satisfy the hasConsistency check here
| require.NoError(err) | ||
| require.Equal("response", resp) | ||
| require.True(handlerCalled) |
There was a problem hiding this comment.
these assertions don't show that the bypassServiceWhitelist is being used, right? you need to show that setFullConsistencyRevisionToContext didn't produce any side effects
|
I addressed all the comments. Thank you. |
|
can you fix the compilation errors? also, were you not able to use |
ivanauth
force-pushed
the
fix/issue-2900-add-forcefull-tests
branch
from
e8f727b to
0dbd233
Compare
|
Fixed the compilation errors. Also added |
ivanauth
force-pushed
the
fix/issue-2900-add-forcefull-tests
branch
3 times, most recently
from
bcfab08 to
f3887fd
Compare
miparnisari
force-pushed
the
fix/issue-2900-add-forcefull-tests
branch
from
2fd1b6d to
e299ceb
Compare
miparnisari
force-pushed
the
fix/issue-2900-add-forcefull-tests
branch
from
e299ceb to
10e0d29
Compare
miparnisari
force-pushed
the
fix/issue-2900-add-forcefull-tests
branch
from
10e0d29 to
261a0eb
Compare
2 participants
Fixes #2900
Adds 21 test cases covering all exported functions in
pkg/middleware/consistency/forcefull.go: unary and stream interceptors, bypass whitelist, error propagation, and readonly error rewriting.