move EncodeUserIdentifier & drop userID from tests #500
Conversation
MatousJobanek
requested review from
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
mfrancisc,
rsoaresd,
fbm3307 and
metlos
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #500 +/- ##
==========================================
- Coverage 85.38% 85.12% -0.26%
==========================================
Files 32 32
Lines 3174 3146 -28
==========================================
- Hits 2710 2678 -32
- Misses 376 380 +4
Partials 88 88
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| svc.MockSignup = func(ctx *gin.Context) (*crtapi.UserSignup, error) { | ||
| assert.Equal(s.T(), expectedUserID, ctx.GetString(context.SubKey)) | ||
| assert.Equal(s.T(), "bill", ctx.GetString(context.UsernameKey)) |
There was a problem hiding this comment.
What is the purpose of these asserts here? As far as I understand the flow, they're only checking that we set the appropriate values in the context a couple of lines above. Is that really needed?
What we're really should be testing there, IMHO, is that the signup service has been used. So what about instead of all those asserts on the context, having a "signupCalled" boolean that would be set to true in the svc.MockSignup and checked for being true after the call to handler?
Or maybe that's assumed by the correct status code of the response and we don't need any of that either?
There was a problem hiding this comment.
TBH, (to keep my sanity intact) I didn't try to question the tests themself, I only modified the related changes (or dropped tests that didn't make sense anymore after my changes).
There are many things weird, wrong, missing, or redundant in the unit tests, I hope that we will get to fixing them later as soon as the whole code is simplified/refactored.
Anyway, I tried to improve the test here: ec1eefa
There was a problem hiding this comment.
and as for you question:
What we're really should be testing there
As I understand it, it verifies that the signup handler is called properly an returns corresponding response values
|
|
@metlos I've address/answered all your review comments, let me know if it looks good to you now. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, fbm3307, MatousJobanek, metlos, mfrancisc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MatousJobanek
merged commit c7f9087
into
codeready-toolchain:master
depends on: codeready-toolchain/toolchain-common#452
KUBESAW-254
This is hopefully the last PR before final dropping of the usage of userIDs as the UserSignup names