minder icon indicating copy to clipboard operation
minder copied to clipboard

Additional test coverage and refactoring needed in ProfileService

Open dmjb opened this issue 1 year ago • 3 comments

Please describe the enhancement

Profile Create/Update was moved to a separate interface as part of this PR: https://github.com/stacklok/minder/pull/2653

There is a significant amount of logic in these two methods, and I decided to defer writing new tests for these methods.

Solution Proposal

Write a test suite for these two methods. Further refactoring may be necessary.

Describe alternatives you've considered

No response

Additional context

No response

Acceptance Criteria

No response

dmjb avatar Mar 14 '24 17:03 dmjb

Hey @gajananan, you mentioned you were interested in this issue, shall I assign it to you?

eleftherias avatar Sep 17 '24 09:09 eleftherias

hi @eleftherias yes, please. (I am sorry for late response)

gajananan avatar Sep 30 '24 07:09 gajananan

hi @dmjb

Could you share your suggestions on additional test cases. I thought of the followings.

Additional Test cases for CreateProfile Method:

  • Test invalid profiles to trigger ErrValidationFailed - e.g. consider example profiles from here: https://github.com/stacklok/minder-rules-and-profiles/tree/main/profiles/github
  • Test creating profile with valid and invalid labels
  • Test with a profile containing edge case values (e.g., maximum length strings).

gajananan avatar Sep 30 '24 08:09 gajananan

Hey @gajananan -- when you finish #4651, do you want to do this, or should we unassign?

evankanderson avatar Jan 28 '25 14:01 evankanderson

@evankanderson yes, I will have a crack at.

gajananan avatar Jan 28 '25 20:01 gajananan

@evankanderson

Could you clarify this issue is already addressed here: https://github.com/mindersec/minder/blob/main/internal/controlplane/handlers_profile_test.go ?

  • TestCreateProfile
  • TestDeleteProfile

gajananan avatar Feb 02 '25 09:02 gajananan

It looks like the issue attempting to cover the code here:

https://github.com/mindersec/minder/blob/main/pkg/profiles/service.go

That said, coverage in the pkg/profiles directory appears to be around 80%, which meets our expectations. In particular, here is the coverage for pkg/profiles/service.go:

https://coveralls.io/builds/72057881/source?filename=pkg%2Fprofiles%2Fservice.go

Thanks for flagging that this was already done!

evankanderson avatar Feb 03 '25 23:02 evankanderson