use helper function for UserSignup creation in signup unit tests #494
Conversation
MatousJobanek
requested review from
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #494 +/- ##
=======================================
Coverage 85.38% 85.38%
=======================================
Files 32 32
Lines 3174 3174
=======================================
Hits 2710 2710
Misses 376 376
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. |
| }, | ||
| Status: crtapi.UserSignupStatus{}, | ||
| } | ||
| otherUserSignup := testusersignup.NewUserSignup( |
There was a problem hiding this comment.
minor and not strictly related to your changes - it looks like now there is not much difference in how the "usersignup it's stored" compared to the other tests. I mean I'm not sure what the test case description refers to. I thought it was about how the otherUserSignup is configured ( with the PropagatedClaims filed being populated ) .
There was a problem hiding this comment.
Yes, you are completely right. It's a leftover that wasn't properly cleaned up after migration of UserSignups from using userIds to using usernames as the name of the resource. The end-goal is to clean this up, but this will require a lot of changes, so I'm doing it in small batches (for now some changes in unit tests that will help me to minimize the size of the changes of the final PRs).
As for this test. It's not about how it's stored, but how it is retrieved - this test doesn't use the userID. Anyway, this sub-test will go away in the upcoming PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, 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 |
pkg/controller/signup_test.go
Outdated
| Conditions: []crtapi.Condition{ | ||
| signup := testusersignup.NewUserSignup( | ||
| testusersignup.WithName("bill"), | ||
| func(userSignup *crtapi.UserSignup) { |
There was a problem hiding this comment.
testusersignup.WithIncompleteSignup("test_reason", "test_message") would make the test more readable IMHO, even if it this would be the single use of it.
There was a problem hiding this comment.
done in a103f78
| }, | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| svc.MockSignup = func(ctx *gin.Context) (*crtapi.UserSignup, error) { | ||
| assert.Equal(s.T(), expectedUserID, ctx.GetString(context.SubKey)) |
There was a problem hiding this comment.
It's testing the rest of the signupCtrl.PostHandler - what happens after the Signup function returns the UserSignup resource.
Let's improve the tests later as soon as we get rid of the unnecessary mocks.
pkg/controller/signup_test.go
Outdated
| @@ -300,11 +275,11 @@ func (s *TestSignupSuite) TestInitVerificationHandler() { | |||
| BodyString("") | |||
|
|
|||
| data := []byte(fmt.Sprintf(`{"phone_number": "%s", "country_code": "1"}`, phoneNumber)) | |||
| rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, userID, "", http.MethodPut, "/api/v1/signup/verification") | |||
| rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, userID, "johny", http.MethodPut, "/api/v1/signup/verification") | |||
There was a problem hiding this comment.
grammar nazi nitpick: "johnny"
There was a problem hiding this comment.
🤦♂️ 638f7ab
|
MatousJobanek
merged commit adb9357
into
codeready-toolchain:master
a follow-up of #492
KUBESAW-254