-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance Unit Test Coverage for Skills and endorsement API Methods #166
base: develop
Are you sure you want to change the base?
Conversation
in the previous pull request, the unitTests files are not properly formatted. Steps followed to fix the mentioned issue.
Then ran the below maven command to check the build locallly
Screenshots for the build success in local @yesyash please review and let me know |
// Mock behaviour | ||
when(endorsementRepository.findBySkillId(skillId)).thenReturn(endorsements); | ||
when(rdsService.getUserDetails(endorsement1.getEndorseId())) | ||
.thenReturn(rdsGetUserDetailsResDto1); | ||
when(rdsService.getUserDetails(endorsement1.getEndorserId())) | ||
.thenReturn(rdsGetUserDetailsResDto2); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we mocking the Db calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes Prakash, correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we mocking the db calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As repository is a dependency for that method and we wanted to test in isolated space for unit tests, I have mocked it and provided pre defined data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prakashchoudhary07 could you please let me know, for the above explanation regarding the reporsitory mocking, if any chnages needed please let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, if I don't have DB code, if queries are wrong also then tests will pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prakashchoudhary07 so this part of db validation is being covered as part of integration testing, so should we include in unit tests as well. please let me know?
cc @yesyash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I will still insist we do it.
Now I am not sure what the test is being written for here. Is it a unit or integration?
So if you were testing the class why are you invoicing the db?
And if some behavior changes, then also since things are mocked, will tests pass without any issues right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you are only writing unit tests for a module, why involve different modules here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We are writing unit tests for the different methods in this pull request.
- I have written unit tests for 4 methods in this pull request with different methods for each test.
And if you are only writing unit tests for a module, why involve different modules here
For the above question, for example, when writing unit tests for the skills API getAll() method, we are retrieving all the skills from the skill repository. So, the dependency for this method is the skillRepository class. To test the getAll method in isolation, I have mocked this class implementation.
Based on the above conversation, do you think? should we include DB code as well? if yes should we use an in-memory database, as if we include the real db, as the data will be dynamic we should think of any strategy for this? could you please advise.
cc @yesyash
Date: 25 10 2024
Developer Name: Dheeraj Chepuri
Issue Ticket Number
#162
#163
#164
#165
Description
This pull request improves unit test coverage for the getAll(),create(),approveRejectSkillRequest() and getAllEndorsementsBySkillId in skillServiceImplementation and EndorsementServiceImplementation classes within the Spring Boot application.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes